Re: [PATCH v2 2/5] sha1_file: remove a buggy value setting

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 6, 2011, 16:26 UTC
Message-ID: <7vpqjdab76.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1315304645-12009-3-git-send-email-Hui.Wang@windriver.com>

Wang Hui <Hui.Wang@windriver.com> writes:

Show 7 quoted lines

> From: Hui Wang <Hui.Wang@windriver.com>
>
> The ent->base[] is a character array, it has pfxlen characters from
> position 0 to (pfxlen-1) to contain an alt object dir name, the
> position pfxlen should be the string terminating character '\0' and
> is deliberately set to '\0' at the previous code line. The position
> (pfxlen+1) is given to ent->name.

Correct. Do you understand why?

We temporarily NUL terminate the ent->base[] so that we can give it to is_directory() to see if that is a directory, but the invariants for a alternate_object_database instance after it is properly initialized by this function are to have:

 - the directory name followed by a slash in the base[] array;
 - the name pointer pointing at one byte beyond the slash;
 - name[2] filled with a slash; and
 - name[41] terminated with NUL.

Later, has_loose_object_nonlocal() calls fill_sha1_path() with the name pointer to fill name[0..1, 3..40] with the hexadecimal representation of the object name, which would result in base[] array to have the pathname for a loose object found in that alternate. The same thing happens in open_sha1_file() to read from a loose object in an alternate.

And you are breaking one of the above invariants by removing that slash after the directory name. These callers of fill_sha1_path() will see the directory name, your NUL, two hex, slash, and 38 hex in base[].

How would the code even work with your patch?