Re: [PATCH v2 29/29] read_packed_refs(): die if `packed-refs` contains bogus data

[Michael Haggerty <mhagger@alum.mit.edu>]

On 06/23/2017 09:58 PM, Jeff King wrote:
> On Fri, Jun 23, 2017 at 09:01:47AM +0200, Michael Haggerty wrote:
> 
>> The old code ignored any lines that it didn't understand. This is
>> dangerous. Instead, `die()` if the `packed-refs` file contains any
>> lines that we don't know how to handle.
> 
> This seems like a big improvement. Is it worth adding a test for a
> corrupted file?

That's a good idea. While I'm at it, I'll distinguish between
unterminated lines and lines that are invalid for other reasons, because
the error message should differ for these cases.

> I assume this isn't something you saw in the wild, but just a deficiency
> you noticed while reading the code.

Correct, I've never seen this problem in the wild. Though, since Git
would have covered up the problem while it existed and obliterated it at
the next `pack-refs`, it's the kind of thing that would be easy to
overlook and hard to prove afterwards.

> I suspect this laxness may have been what allowed us to add the optional
> peeled values long ago. But I think I'd rather see us be more strict and
> notice corruption or nonsense rather than quietly ignoring (especially
> because an operation like "git pack-refs" would then overwrite it,
> dropping whatever entries we didn't understand).

That's an interesting consideration. I suppose we could plan in some way
to extend the `packed-refs` format in a backwards-transparent fashion,
if there is ever an extension that old versions of git would be free to
ignore, while leaving the format strict enough that genuine corruption
would be unlikely to be overlooked. For example, we could reserve one or
more special characters which, when they appear at the beginning of a
line, mean that the line should be ignored.

But such an extension would only be practical if other Git
implementations are similarly lax. But libgit2 is currently strict about
rejecting unrecognized lines, so such an extension couldn't be
backwards-compatible with old versions of libgit2. So it doesn't seem
very useful.

Michael