Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🎁 Populate nwritten_out when errors occur in config-store::get or dictionary::get #389

Merged
merged 2 commits into from
Jun 24, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/compute-at-edge-abi/compute-at-edge.witx
Original file line number Diff line number Diff line change
Expand Up @@ -638,7 +638,8 @@
(param $key string)
(param $value (@witx pointer (@witx char8)))
(param $value_max_len (@witx usize))
(result $err (expected $num_bytes (error $fastly_status)))
(param $nwritten_out (@witx pointer (@witx usize)))
(result $err (expected (error $fastly_status)))
)
)

Expand Down
3 changes: 2 additions & 1 deletion lib/compute-at-edge-abi/config-store.witx
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
(param $key string)
(param $value (@witx pointer (@witx char8)))
(param $value_max_len (@witx usize))
(result $err (expected $num_bytes (error $fastly_status)))
(param $nwritten_out (@witx pointer (@witx usize)))
(result $err (expected (error $fastly_status)))
)
)
8 changes: 5 additions & 3 deletions lib/src/wiggle_abi/config_store.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
use super::{
fastly_config_store::FastlyConfigStore,
fastly_dictionary::FastlyDictionary,
types::{ConfigStoreHandle, DictionaryHandle},
};
use crate::{session::Session, wiggle_abi::fastly_dictionary::FastlyDictionary, Error};
use crate::{session::Session, Error};
use wiggle::GuestPtr;

impl FastlyConfigStore for Session {
Expand All @@ -17,8 +18,9 @@ impl FastlyConfigStore for Session {
key: &GuestPtr<str>,
buf: &GuestPtr<u8>,
buf_len: u32,
) -> Result<u32, Error> {
nwritten_out: &GuestPtr<u32>,
) -> Result<(), Error> {
let dict_handle = DictionaryHandle::from(unsafe { config_store.inner() });
<Self as FastlyDictionary>::get(self, dict_handle, key, buf, buf_len)
<Self as FastlyDictionary>::get(self, dict_handle, key, buf, buf_len, nwritten_out)
}
}
18 changes: 13 additions & 5 deletions lib/src/wiggle_abi/dictionary_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ impl FastlyDictionary for Session {
key: &GuestPtr<str>,
buf: &GuestPtr<u8>,
buf_len: u32,
) -> Result<u32, Error> {
nwritten_out: &GuestPtr<u32>,
) -> Result<(), Error> {
let dict = &self.dictionary(dictionary)?.contents;

let item_bytes = {
Expand All @@ -54,16 +55,23 @@ impl FastlyDictionary for Session {
.as_bytes()
};

if item_bytes.len() > buf_len as usize {
if item_bytes.len() > usize::try_from(buf_len).expect("buf_len must fit in usize") {
// Write out the number of bytes necessary to fit this item, or zero on overflow to
// signal an error condition. This is probably unnecessary, as config store entries
// may be at most 8000 utf-8 characters large.
nwritten_out.write(u32::try_from(item_bytes.len()).unwrap_or(0))?;
return Err(Error::BufferLengthError {
buf: "dictionary_item",
len: "dictionary_item_max_len",
});
}
let item_len = u32::try_from(item_bytes.len())
.expect("smaller than dictionary_item_max_len means it must fit");

// We know the conversion of item_bytes.len() to u32 will succeed, as it's <= buf_len.
let item_len = u32::try_from(item_bytes.len()).unwrap();

nwritten_out.write(item_len)?;
buf.as_array(item_len).copy_from_slice(item_bytes)?;
Ok(item_len)

Ok(())
}
}
61 changes: 55 additions & 6 deletions test-fixtures/src/bin/config-store-lookup.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,59 @@
//! A guest program to test that dictionary lookups work properly.
//! A guest program to test that config-store lookups work properly.

use fastly::ConfigStore;
use fastly_shared::FastlyStatus;

fn main() {
let animals = ConfigStore::open("animals");
assert_eq!(animals.get("dog").unwrap(), "woof");
assert_eq!(animals.get("cat").unwrap(), "meow");
assert_eq!(animals.get("lamp"), None);
let animals = unsafe {
let mut dict_handle = fastly_shared::INVALID_CONFIG_STORE_HANDLE;
let res = fastly_sys::fastly_config_store::open(
"animals".as_ptr(),
"animals".len(),
&mut dict_handle as *mut _,
);
assert_eq!(res, FastlyStatus::OK, "Failed to open config-store");
dict_handle
};

#[derive(Debug, PartialEq, Eq)]
enum LookupResult {
Success(String),
Missing,
BufTooSmall(usize),
Err(FastlyStatus),
}

let get = |key: &str, buf_len: usize| unsafe {
let mut value = Vec::with_capacity(buf_len);
let mut nwritten = 0;
let res = fastly_sys::fastly_config_store::get(
animals,
key.as_ptr(),
key.len(),
value.as_mut_ptr(),
buf_len,
&mut nwritten as *mut _,
);

if res != FastlyStatus::OK {
if res == FastlyStatus::NONE {
return LookupResult::Missing;
}

if res == FastlyStatus::BUFLEN {
assert!(nwritten > 0 && buf_len < nwritten);
return LookupResult::BufTooSmall(nwritten);
}

return LookupResult::Err(res);
}

value.set_len(nwritten);
value.shrink_to(nwritten);
LookupResult::Success(String::from_utf8(value).unwrap())
};

assert_eq!(get("dog", 4), LookupResult::Success(String::from("woof")));
assert_eq!(get("cat", 4), LookupResult::Success(String::from("meow")));
assert_eq!(get("cat", 2), LookupResult::BufTooSmall(4));
assert_eq!(get("lamp", 4), LookupResult::Missing);
}
28 changes: 21 additions & 7 deletions test-fixtures/src/bin/dictionary-lookup.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,18 @@ fn main() {
"animals".len(),
&mut dict_handle as *mut _,
);
assert_eq!(res, FastlyStatus::OK, "Failed to open dictionary");
assert_eq!(res, FastlyStatus::OK, "Failed to open config-store");
elliottt marked this conversation as resolved.
Show resolved Hide resolved
dict_handle
};

#[derive(Debug, PartialEq, Eq)]
enum LookupResult {
Success(String),
Missing,
BufTooSmall(usize),
Err(FastlyStatus),
}

let get = |key: &str, buf_len: usize| unsafe {
let mut value = Vec::with_capacity(buf_len);
let mut nwritten = 0;
Expand All @@ -28,18 +36,24 @@ fn main() {

if res != FastlyStatus::OK {
if res == FastlyStatus::NONE {
return Ok(None);
return LookupResult::Missing;
}

if res == FastlyStatus::BUFLEN {
assert!(nwritten > 0 && buf_len < nwritten);
return LookupResult::BufTooSmall(nwritten);
}

return Err(res);
return LookupResult::Err(res);
}

value.set_len(nwritten);
value.shrink_to(nwritten);
Ok(Some(String::from_utf8(value).unwrap()))
LookupResult::Success(String::from_utf8(value).unwrap())
};

assert_eq!(get("dog", 4).unwrap().unwrap(), "woof");
assert_eq!(get("cat", 4).unwrap().unwrap(), "meow");
assert_eq!(get("lamp", 4), Ok(None));
assert_eq!(get("dog", 4), LookupResult::Success(String::from("woof")));
assert_eq!(get("cat", 4), LookupResult::Success(String::from("meow")));
assert_eq!(get("cat", 2), LookupResult::BufTooSmall(4));
assert_eq!(get("lamp", 4), LookupResult::Missing);
}
Loading