After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 790030 - GResource/GVariant fails to load from non-pointer aligned memory
GResource/GVariant fails to load from non-pointer aligned memory
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gvariant
unspecified
Other Linux
: Normal normal
: 2.56
Assigned To: Allison Karlitskaya (desrt)
gtkdev
Depends on:
Blocks:
 
 
Reported: 2017-11-07 17:41 UTC by Sebastian Dröge (slomo)
Modified: 2017-11-14 14:59 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
test-gresource.c (40.20 KB, text/x-csrc)
2017-11-07 17:41 UTC, Sebastian Dröge (slomo)
  Details
GResource – Add note to documentation that the memory must be at least pointer aligned (1.09 KB, patch)
2017-11-13 13:42 UTC, Sebastian Dröge (slomo)
committed Details | Review
GResource – Create an internal copy of the GBytes if it is not pointer aligned (1.91 KB, patch)
2017-11-13 13:42 UTC, Sebastian Dröge (slomo)
none Details | Review
GResource – Create an internal copy of the GBytes if it is not pointer aligned (3.56 KB, patch)
2017-11-14 08:40 UTC, Sebastian Dröge (slomo)
committed Details | Review

Description Sebastian Dröge (slomo) 2017-11-07 17:41:34 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
Comment 1 Colin Walters 2017-11-07 18:21:23 UTC
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?
Comment 2 Colin Walters 2017-11-07 18:23:46 UTC
Does #[repr(align = "8")] help?
Comment 3 Sebastian Dröge (slomo) 2017-11-07 18:41:37 UTC
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.
Comment 4 Philip Withnall 2017-11-08 12:46:01 UTC
(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.
Comment 5 Sebastian Dröge (slomo) 2017-11-13 13:42:23 UTC
Created attachment 363506 [details] [review]
GResource – Add note to documentation that the memory must be at least pointer aligned
Comment 6 Sebastian Dröge (slomo) 2017-11-13 13:42:33 UTC
Created attachment 363507 [details] [review]
GResource – Create an internal copy of the GBytes if it is not pointer aligned
Comment 7 Philip Withnall 2017-11-14 00:11:58 UTC
Review of attachment 363506 [details] [review]:

++
Comment 8 Philip Withnall 2017-11-14 00:18:04 UTC
Review of attachment 363507 [details] [review]:

Looks good to me, thanks.
Comment 9 Philip Withnall 2017-11-14 00:19:35 UTC
Can you also add a unit test (could be based on test-gresource.c) to check that it works with non-aligned input please?
Comment 10 Sebastian Dröge (slomo) 2017-11-14 08:40:07 UTC
Created attachment 363563 [details] [review]
GResource – Create an internal copy of the GBytes if it is not pointer aligned
Comment 11 Sebastian Dröge (slomo) 2017-11-14 08:40:27 UTC
Now with test :)
Comment 12 Philip Withnall 2017-11-14 10:49:58 UTC
Review of attachment 363563 [details] [review]:

Great, thanks.
Comment 13 Sebastian Dröge (slomo) 2017-11-14 14:31:20 UTC
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
Comment 14 Sebastian Dröge (slomo) 2017-11-14 14:33:32 UTC
Thanks for the review. You'll backport the docs commit?
Comment 15 Philip Withnall 2017-11-14 14:59:35 UTC
(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.