GNOME Bugzilla – Bug 790030
GResource/GVariant fails to load from non-pointer aligned memory
Last modified: 2017-11-14 14:59:35 UTC
Created attachment 363162 [details] test-gresource.c See attached code. Changing > #define OFFSET 1 to 0 makes it work again. Without proper alignment, you get > GLib:ERROR:../../../../glib/gvariant-serialiser.c:167:g_variant_serialised_check: assertion failed (alignment & (gsize) serialised.data == 0): (1 == 0) This doesn't seem to be documented anywhere and is generally a big trap just waiting to happen. There's nothing that requires a GBytes to be aligned in any way, and the GResource API accepts any kind of GBytes. This was first noticed in the Rust bindings: https://github.com/gtk-rs/glib/issues/242
We could do the memcpy() internally if we detect it's not aligned, but it's clearly better to update the docs to suggest callers ensure data is aligned, and to change the Rust bindings to encourage that?
Does #[repr(align = "8")] help?
It doesn't, no. It would be good if it would memcpy if needed and maybe print a warning, instead of just killing the application. And to have that documented :) Also it would be difficult to ensure this, as GBytes can have data of any alignment. The best thing that the Rust bindings could do here is the same as GLib: panic, or internally memcpy.
(In reply to Sebastian Dröge (slomo) from comment #3) > It would be good if it would memcpy if needed and maybe print a warning, > instead of just killing the application. Internally memcpy()ing into a new, correctly-aligned GBytes is probably the right approach here. I’m not sure about printing a warning, since it’s not really a runtime error: either non-aligned GBytes are allowed, and we deal with them; or they’re not allowed (a programmer error), and we should abort. I’d definitely be in favour of printing a debug message, to make the performance problems which will potentially arise from the memcpy() more obvious to any developer trying to debug them. > And to have that documented :) A documentation patch is very welcome as a high priority. Please keep it separate from any other patch, so we can backport the documentation notice about providing aligned GBytes to glib-2-54.
Created attachment 363506 [details] [review] GResource – Add note to documentation that the memory must be at least pointer aligned
Created attachment 363507 [details] [review] GResource – Create an internal copy of the GBytes if it is not pointer aligned
Review of attachment 363506 [details] [review]: ++
Review of attachment 363507 [details] [review]: Looks good to me, thanks.
Can you also add a unit test (could be based on test-gresource.c) to check that it works with non-aligned input please?
Created attachment 363563 [details] [review] GResource – Create an internal copy of the GBytes if it is not pointer aligned
Now with test :)
Review of attachment 363563 [details] [review]: Great, thanks.
Attachment 363506 [details] pushed as 3c9c01e - GResource – Add note to documentation that the memory must be at least pointer aligned Attachment 363563 [details] pushed as 7b60708 - GResource – Create an internal copy of the GBytes if it is not pointer aligned
Thanks for the review. You'll backport the docs commit?
(In reply to Sebastian Dröge (slomo) from comment #14) > Thanks for the review. You'll backport the docs commit? Pushed to glib-2-54 as 23373d7f7e0b4041fc4bf18f1e9fffe666eac9c0.