Re: [PATCH 2/2] strbuf: allow to use preallocated memory
- From
Michael Haggerty <mhagger@alum.mit.edu>
- Date
- May 31, 2016, 03:05 UTC
- Message-ID
- <574CFF75.3090805@alum.mit.edu>
- In-Reply-To
- <alpine.DEB.2.20.1605301326530.4449@virtualbox>
On 05/30/2016 02:13 PM, Johannes Schindelin wrote:
Show 14 quoted lines
> [...]
>> @@ -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