On Wed, 27.08.14 00:39, Zbigniew Jędrzejewski-Szmek (zbys...@in.waw.pl) wrote:
> mmap code crashes when attempting to map an object of zero size. > > https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=758392 > https://bugs.freedesktop.org/show_bug.cgi?id=82894 > --- > Oops, both my analysis and fix were totally borked. The code already > checked that object size is at least sizeof(ObjectHeader) in > journal_file_move_to_object(). This is enough to appease the mmap > code. But the object size field was not properly wrapped in le64toh(), > which I hope was the reason for the crash. > > 2/2 adds the check you suggested. But we actually need to check > the size of uncompressed payload, so my check on size was in the > wrong place anyway. > > I'm posting this again, in case anyone spots a different mistake. > > @Chris: I'm assuming that you're running in BE mode. It would be great > if you could check that this fixed the crash you were observing. > > src/journal/journal-file.h | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/src/journal/journal-file.h b/src/journal/journal-file.h > index 3d416820b0..da2ef3b795 100644 > --- a/src/journal/journal-file.h > +++ b/src/journal/journal-file.h > @@ -214,14 +214,15 @@ static unsigned type_to_context(int type) { > > static inline int journal_file_object_keep(JournalFile *f, Object *o, > uint64_t offset) { > unsigned context = type_to_context(o->object.type); > + uint64_t s = le64toh(o->object.size); > > return mmap_cache_get(f->mmap, f->fd, f->prot, context, true, > - offset, o->object.size, &f->last_stat, NULL); > + offset, s, &f->last_stat, NULL); > } > > static inline int journal_file_object_release(JournalFile *f, Object *o, > uint64_t offset) { > unsigned context = type_to_context(o->object.type); > + uint64_t s = le64toh(o->object.size); > > - return mmap_cache_release(f->mmap, f->fd, f->prot, context, > - offset, o->object.size); > + return mmap_cache_release(f->mmap, f->fd, f->prot, context, offset, > s); > } Looks good. We probably should rerun sparse on this, to figure out if we missed anything else... (That said, I'd probably not have bothered with first writing the result to a new variable "s", I'd just have invoked le64toh() directly as function parameter... Not that this matters in any way though...) Lennart -- Lennart Poettering, Red Hat _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel