Re: [PATCH] tree-walk: don't parse incorrect entries

[Junio C Hamano <gitster@pobox.com>]

Martin Koegler <mkoegler@auto.tuwien.ac.at> writes:

> The current code can access memory outside of the tree
> buffer in the case of malformed tree entries.
>
> This patch prevent this by:
> * The rest of the buffer must be at least 24 bytes
>   (at least 1 byte mode, 1 blank, at least one byte path name,
>    1 zero, 20 bytes sha1).

Good ("zero" in this context is better spelled as "NUL").

> * Check that the last zero (21 bytes before the end) is present.
>   This ensurse, that strlen and get_mode stay within the buffer.

Good (ditto).

> * The mode may not be empty. We have only to reject a blank at the begin,
>   as the rest is handled by if (c < '0' || c > '7').

I initially thought this was slightly iffy as tree objects were
written with padding for mode bits, but that was padding with
zero ('0') to the left and not with SP, so it is a good change,
I think (I am saying this primarily so that others can say "you
are wrong, there are valid old trees with SP there" to stop me).

> * The blank is ensured by get_mode.

Ok.

> * The start of the path may not be after the last zero (21 bytes before the end).

How can that be possible?

 - you know end points at NUL and buf < end;

 - get_mode() starts scanning from buf, stops at the first SP if
   returns a non NULL pointer; anything non octal digit before
   it sees that SP results in a NULL return;

 - the return value of get_mode() is the beginning of the path.

The second point above means when get_mode() scans buf, it would
never go beyond end which you already made sure is NUL (which is
not SP and not an octal digit).  If it hits end, you would get NULL
pointer back, wouldn't you?

Rejecting an empty path may be sensible (i.e. checking "!*path"
instead), though.