Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
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
2 changes: 2 additions & 0 deletions .github/workflows/rust.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ jobs:

steps:
- uses: actions/checkout@v2
- name: Install glib for testing
run: sudo apt-get install -y libglib2.0-dev
- name: Build
run: cargo build --verbose
- name: Run tests
Expand Down
24 changes: 17 additions & 7 deletions gvariant-macro/src/generate_impl.rs
Original file line number Diff line number Diff line change
@@ -1,27 +1,37 @@
use std::error::Error;
use std::io::Write;
use std::{collections::HashSet, error::Error};

use crate::{marker_type, type_parser::GVariantType};

pub(crate) fn generate_types(spec: &GVariantType) -> Result<String, Box<dyn Error>> {
pub(crate) fn generate_types(
spec: &GVariantType,
defined_types: &mut HashSet<GVariantType>,
) -> Result<String, Box<dyn Error>> {
if defined_types.contains(spec) {
// If we've already defined this type in this scope. Don't want to
// define it again otherwise we get "must be defined only once in the
// type namespace of this module" compiler errors:
return Ok("".to_owned());
}
defined_types.insert(spec.clone());
Ok(match spec {
GVariantType::Tuple(children) => {
let mut out = "".to_string();
for child in children {
out += generate_types(child)?.as_ref();
out += generate_types(child, defined_types)?.as_ref();
}
out += generate_tuple(spec, children)?.as_ref();
out
}
GVariantType::DictItem(children) => {
let mut out = "".to_string();
out += generate_types(&children[0])?.as_ref();
out += generate_types(&children[1])?.as_ref();
out += generate_types(&children[0], defined_types)?.as_ref();
out += generate_types(&children[1], defined_types)?.as_ref();
out += generate_tuple(spec, children.as_ref())?.as_ref();
out
}
GVariantType::A(x) => generate_types(x)?,
GVariantType::M(x) => generate_types(x)?,
GVariantType::A(x) => generate_types(x, defined_types)?,
GVariantType::M(x) => generate_types(x, defined_types)?,

// Everything else is a builtin
_ => "".to_owned(),
Expand Down
5 changes: 3 additions & 2 deletions gvariant-macro/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

extern crate proc_macro;
use proc_macro::TokenStream;
use std::error::Error;
use std::{collections::HashSet, error::Error};

mod generate_impl;
mod type_parser;
Expand All @@ -20,7 +20,8 @@ pub fn gv_type(input: TokenStream) -> TokenStream {
#[proc_macro]
pub fn define_gv(input: TokenStream) -> TokenStream {
let typestr = parse_macro_input!(input as LitStr).value();
generate_impl::generate_types(&one(typestr.as_ref()).unwrap())
let mut defined_types: HashSet<GVariantType> = HashSet::new();
generate_impl::generate_types(&one(typestr.as_ref()).unwrap(), &mut defined_types)
.unwrap()
.parse()
.unwrap()
Expand Down
2 changes: 1 addition & 1 deletion gvariant-macro/src/type_parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ impl fmt::Display for ParseError {
}
impl Error for ParseError {}

#[derive(Debug, PartialEq, Clone)]
#[derive(Debug, PartialEq, Clone, Hash, Eq)]
pub(crate) enum GVariantType {
B,
Y,
Expand Down
4 changes: 2 additions & 2 deletions gvariant/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1629,11 +1629,11 @@ where
let mut bytes_written = 0;
let mut offsets = vec![];
for x in self.into_iter() {
bytes_written += x.serialize(f)?;
offsets.push(bytes_written);
let padding = align_offset::<GvT::AlignOf>(bytes_written).to_usize() - bytes_written;
f.write_all(&b"\0\0\0\0\0\0\0"[..padding])?;
bytes_written += padding;
bytes_written += x.serialize(f)?;
offsets.push(bytes_written);
}
Comment on lines 1631 to 1637

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

This is an excellent and critical fix for NonFixedWidthArray serialization.

The previous implementation had a logic error where padding was calculated and written after the element was serialized. This meant that the element itself was not guaranteed to be correctly aligned, which is a violation of the GVariant specification.

The new implementation correctly calculates and writes the necessary padding before serializing the element, ensuring that each element in the array is properly aligned. This resolves a significant correctness issue.

Well done for spotting and fixing this!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well gee wiz and wowzers! This is an excellent review. Thank you very much @gemini-code-assist. You upbeat regurgitation of my commit message has made my day.

Well done!

write_offsets(bytes_written, offsets.as_ref(), f)
}
Expand Down
19 changes: 19 additions & 0 deletions gvariant/tests/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,25 @@ fn test_complex_types() {
assert_eq!(t.8, "");
}

#[test]
fn test_repeated_types() {
let foo = [("hi", 3)];
let data = gv!("(a{si}a{si})").serialize_to_vec(&(&foo, &foo));
let aligned = copy_to_align(&*data);

assert_eq!(
&data,
b"hi\x00\x00\x03\x00\x00\x00\x03\x09\x00\x00hi\x00\x00\x03\x00\x00\x00\x03\x09\x0a"
);

let s = gv!("(a{si}a{si})").cast(aligned.as_ref()).to_tuple();
println!("{:?}", s);
assert_eq!(s.0, s.1);
assert_eq!(s.0.len(), 1);
assert_eq!(s.0[0].to_tuple().0, "hi");
assert_eq!(*s.0[0].to_tuple().1, 3);
}

#[test]
fn test_spec_examples() {
let data = copy_to_align(b"foo\0\xff\xff\xff\xff\x04");
Expand Down