GNOME Bugzilla – Bug 658794
Reading value of entry->mtime in meta_journal_iterate fails on 64-bit sparc system (gentoo/linux)
Last modified: 2014-04-16 15:48:27 UTC
After installation of linux/gentoo (2.6.39-gentoo-r3) on a Sun-Blade-1500 (sun4u TI UltraSparc IIIi (Jalapeno)) several gnome desktop applications and tools (nautilus, gedit, evince, gvfs-info) are crashing on line 1306 of gvfs-1.6.7/metadata/metatree.c in function static char * meta_journal_iterate (MetaJournal *journal, const char *path, journal_key_callback key_callback, journal_path_callback path_callback, gpointer user_data) { when attempting to read guint64 mtime = GUINT64_FROM_BE (entry->mtime); and the corresponding memory section is not 64-bit-aligned. The memory section for the journal is created in line 1172 data = mmap (NULL, statbuf.st_size, mmap_prot, MAP_SHARED, fd, 0); It starts with a 20-byte header (MetaJournalHeader) followed by 2x4 byte section for guint32 entry_size; guint32 crc32; Hence the value of guint64 mtime starts for the first entry of the journal at the offset 20 + 4 + 4 = 28 byte which is clearly not 64-bit-aligned. Testing a read operation at offset 0 in contrast (which is on my system always 64-bit-aligned) is successful. A simple and short patch for this problem which consists in reading two times a guint32 value and combining the results for a guint64 value is: diff metatree.c.orig metatree.c 1306c1306,1312 < mtime = GUINT64_FROM_BE (entry->mtime); --- > guint32* big_part = (guint32*)((char*) entry + 8); > guint64 big_part_64 = *big_part; > big_part_64 <<= 32; > guint32* little_part = (guint32*)((char*) entry + 12); > guint64 big_little = (big_part_64 | ((guint64)(*little_part))); > mtime = GUINT64_FROM_BE (big_little); > With this patch (could be shortened) the problem for nautilus e.a. has gone.
Writing dissimilar structs to a flat buffer without padding only works on x86 and amd64 - and even there will offer suboptimal performance. On RISC architectures it results in crashes. IMHO, the correct solution would be to write all structs to memaligned addresses in the first place, for instance by using "+ sizeof (struct foo) % G_MEM_ALIGN" constructions.
By the way, this issue is present in all versions of gvfs, including git master (1.9.x).
(In reply to comment #1) > > IMHO, the correct solution would be to write all structs to memaligned > addresses in the first place, for instance by using "+ sizeof (struct foo) % > G_MEM_ALIGN" constructions. Unfortunately G_MEM_ALIGN isn't enough: http://mail.gnome.org/archives/gtk-devel-list/2004-December/msg00091.html On many platforms, it's 4, but minimum alignment required is 8 for 8 byte types.
I guess this could be moved to a "CONFIRMED" status and version set to trunk, no? Thanks a lot :)
No objections. Each time I upgrade my system I have to reapply the above patch.
Can anybody from upstream review this patch? Thanks
(marking for me to look at it before 3.4 is out)
Looks like finally wasn't possible for 3.4 cycle, could at least be solved in 1.12 branch to get it fixed in next gvfs bump? Thanks :)
Looks like this got overlooked again for 3.6 cycle :( Could you please tell us if there is a problem with this patch at least ?
(In reply to comment #9) > Looks like this got overlooked again for 3.6 cycle :( > Could you please tell us if there is a problem with this patch at least ? The patch probably got lost because nobody actually attached a patch to the bug, and the patch as it was given is unappliable other than by hand. Create a git-formatted patch and attach it to this bug, and I'm sure it'll get more feedback.
Created attachment 227397 [details] [review] Patch for discussed wrong memory alignment in metatree.c for sparc systems Attaching a formatted patch taken from my overlay.
(In reply to comment #11) > Created an attachment (id=227397) [details] [review] > Patch for discussed wrong memory alignment in metatree.c for sparc systems > > Attaching a formatted patch taken from my overlay. That looks way too scary. Wouldn't be easier to add `__attribute__ ((aligned (8)))` to the `MetaJournal` definition?
(In reply to comment #12) > (In reply to comment #11) > > Created an attachment (id=227397) [details] [review] [details] [review] > > Patch for discussed wrong memory alignment in metatree.c for sparc systems > > > > Attaching a formatted patch taken from my overlay. > > That looks way too scary. It looks scary indeed and I'm not going to compute bits. > Wouldn't be easier to add `__attribute__ ((aligned (8)))` to the `MetaJournal` > definition? I guess that would break existing data structures on the disk. I'm willing to push the patch ifdef'ed, what would be the define to check please? Would "defined(__sparc__)" be enough?
Comment on attachment 227397 [details] [review] Patch for discussed wrong memory alignment in metatree.c for sparc systems Setting needs-work status as per comment 13.
Closing this bug due to inactivity and because there is another bug with "nicer" patch to solve it. *** This bug has been marked as a duplicate of bug 726456 ***