[systemd-devel] [PATCH 1/2] sd-journal: properly convert object->size on big endian

[Lennart Poettering]

On Wed, 27.08.14 00:39, Zbigniew Jędrzejewski-Szmek (zbyszek at 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