Re: [PATCH 2/5] revert: make commit subjects in insn sheet optional

From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Dec 9, 2011, 05:57 UTC
Message-ID: <CALkWK0nkPB5WptJ9nSkSOif5_0R_f39Dg_HR3Rmg02hG_4Q1Tg@mail.gmail.com>
In-Reply-To: <20111207070223.GC11737@elie.hsd1.il.comcast.net>

Hi again,

Jonathan Nieder wrote:

Show 12 quoted lines

> Ramkumar Ramachandra wrote:
>
>> [...]
>> While at it, also fix a bug: currently, we use a commit-id-shaped
>> buffer to store the word after "pick" in '.git/sequencer/todo'.  This
>> is both wasteful and wrong because it places an artificial limit on
>> the line length.  Eliminate the need for the buffer altogether, and
>> add a test demonstrating this.
>
> Reading the above does not make it at all obvious that I should want
> to apply this patch because otherwise my prankster friend can cause my
> copy of git to crash or run arbitrary code by putting a long commit

Working backwards:
get_sha1() is what will finally misbehave: how?  It uses strlen() and
let's assume that the number returned by it is too big to fit in a
size_t.  Surely, this means that we should only use get_sha1() on
something whose length is bounded.  So, do we ever try to get to the
end of the line?  Yes!  Let's assume that the problem starts when
end_of_object_name calls strcspn which returns something too big to
fit in a size_t.  From the manpage it has no standard way of reporting
failure.  I'm not sure what to do; I think I have two choices:
1. Implement the strscpn() using two strchrnul() calls.
2. Drop this patch and use strbuf to replace the fixed-size buffer.

I think I'll go with the second option.  What do you think?

-- Ram