Re: [PATCH 2/2] strbuf: allow to use preallocated memory

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

On 05/30/2016 02:13 PM, Johannes Schindelin wrote:
> [...]
>> @@ -38,7 +67,11 @@ char *strbuf_detach(struct strbuf *sb, size_t *sz)
>>  {
>>  	char *res;
>>  	strbuf_grow(sb, 0);
>> -	res = sb->buf;
>> +	if (sb->flags & STRBUF_OWNS_MEMORY)
>> +		res = sb->buf;
>> +	else
>> +		res = xmemdupz(sb->buf, sb->alloc - 1);
> 
> This looks like a usage to be avoided: if we plan to detach the buffer,
> anyway, there is no good reason to allocate it on the heap first. I would
> at least issue a warning here.

I think this last case should be changed to

    res = xmemdupz(sb->buf, sb->len);

Johannes, if this change is made then I think that there is a reasonable
use case for calling `strbuf_detach()` on a strbuf that wraps a
stack-allocated string, so I don't think that a warning is needed.

I think this change makes sense. After all, once a caller detaches a
string, it is probably not planning on growing/shrinking it anymore, so
any more space than that would probably be wasted. In fact, since the
caller has no way to ask how much extra space the detached string has
allocated, it is almost guaranteed that the space would be wasted.

Actually, that is not 100% certain. Theoretically, a caller might read
`sb->alloc` *before* calling `strbuf_detach()`, and assume that the
detached string has that allocated size. Or the caller might call
`strbuf_grow()` then assume that the detached string has at least that
much free space.

I sure hope that no callers actually do that. The docstring for
`strbuf_detach()` doesn't promise that it will work, and there is a
pretty stern warning [1] that should hopefully have dissuaded developers
from such a usage. But how could it be checked for sure?

* Audit the callers of strbuf_detach(). But given that there are nearly
200 callers, that would be a huge amount of work.

* On a test branch, change the existing implementation of
strbuf_detach() to return newly-allocated memory of size `sb->len + 1`
and free `sb->buf`, then run the test suite under valgrind. This would
flush out examples of this antipattern in the test suite.

It might seem like we don't have to worry about this, because existing
callers only deal with strbufs that wrap heap-allocated strings. But
such a caller might get a strbuf passed to it from a caller, and that
caller might someday be modified to use stack-allocated strings. So I
think that at least the valgrind test suggested above would be prudent.

Michael

[1]
https://github.com/git/git/blob/f3913c2d03abc660140678a9e14dac399f847647/strbuf.h#L20-L23