GNOME Bugzilla – Bug 705968
Fix use of uninitialized memory in ostree_builtin_checksum()
Last modified: 2013-08-18 11:21:55 UTC
Causes a critical warning: (ostree checksum:20281): GLib-CRITICAL **: g_main_loop_unref: assertion `g_atomic_int_get (&loop->ref_count) > 0' failed ostree checksum: A filename must be given
Created attachment 251576 [details] [review] Fix use of uninitialized memory in ostree_builtin_checksum()
Review of attachment 251576 [details] [review]: I'd prefer: memset (&data, 0, sizeof (data)); Since it's robust against other fields too. Feel free to push with that change, thanks!
Review of attachment 251576 [details] [review]: Will do, but FWIW = { 0, } is defined in C to set all fields to zero. Any fields not specified in the initializer will be set to zero.
Attachment 251576 [details] pushed as 349d795 - Fix use of uninitialized memory in ostree_builtin_checksum()
(In reply to comment #3) > Review of attachment 251576 [details] [review]: > > Will do, but FWIW = { 0, } is defined in C to set all fields to zero. Any > fields not specified in the initializer will be set to zero. Ok so... { 0, }; is one of those things that I somehow had it in my head "don't use this". When you filed this patch, I couldn't remember exactly why, but I thought of something that sounded plausible =) But the real reason, thinking about this more, is a while ago I was reading about kernel information leaks. While it's true that { 0, } will fill in the fields, it *doesn't* necessarily fill in holes in the structure for padding, and this is really bad for a kernel. We however are obviously not a kernel, and so we really should use { 0, } since it's significantly cleaner and more concise. Sorry about the brainfart!
Created attachment 252007 [details] [review] Use { 0, } for structure initialization rather than memset() It's cleaner, safer, and I had a totally wrong idea stuck in my head about why memset() should be used.
(In reply to comment #5) > But the real reason, thinking about this more, is a while ago I was reading > about kernel information leaks. While it's true that { 0, } will fill in the > fields, it *doesn't* necessarily fill in holes in the structure for padding, > and this is really bad for a kernel. Interesting, good to know.
(In reply to comment #7) > (In reply to comment #5) > > But the real reason, thinking about this more, is a while ago I was reading > > about kernel information leaks. While it's true that { 0, } will fill in the > > fields, it *doesn't* necessarily fill in holes in the structure for padding, > > and this is really bad for a kernel. > > Interesting, good to know. Can you review the patch? Not sure if you saw https://mail.gnome.org/archives/ostree-list/2013-August/msg00004.html
Comment on attachment 252007 [details] [review] Use { 0, } for structure initialization rather than memset() Looks good to me.
Attachment 252007 [details] pushed as eaee309 - Use { 0, } for structure initialization rather than memset()