Re: [PATCH 16/22] sequencer: prepare for rebase -i's GPG settings

From: Jakub Narębski <jnareb@gmail.com>
Date: Aug 31, 2016, 20:29 UTC
Message-ID: <a948ad4f-2145-bacb-0ec6-7edac14c4d4c@gmail.com>
In-Reply-To: <xmqqmvjszoia.fsf@gitster.mtv.corp.google.com>

W dniu 31.08.2016 o 22:12, Junio C Hamano pisze:

> Jakub Narębski <jnareb@gmail.com> writes:
>> Johannes Schindelin wrote:

Show 9 quoted lines

>>> +			else {
>>> +				opts->gpg_sign = buf.buf + 2;
>>> +				strbuf_detach(&buf, NULL);
>>
>> Wouldn't we leak 2 characters that got skipped?  Maybe xstrdup would
>> be better (if it is leaked, and not reattached)?
> 
> An attempt to avoid leaking by calling free(opts->gpg_sign) would
> make it crash, which would be even worse ;-).

 
Actually, from C standard:

"If ptr [in free(ptr)] does not point to a block of memory allocated
 with the above functions [malloc(), etc.], it causes undefined behavior."
                                                      ^^^^^^^^^

Which is even worse if it does not lead to crash.

Note that if the last line was

    +				sequencer_entrust(opts, strbuf_detach(&buf, NULL));

we can make it not leak.  A bit tricksy, though.

Though xstrdup(buf.buf + 2) followed by strbuf_release(&buf) would make free(opts->gpg_sign) possible without crash. That is we can work without *_entrust() mechanism at the cost of strdups.

-- 
Jakub Narębski