threads / patch / 47401

patchstrbuf: release memory on read error in strbuf_read_once()

Subject: [PATCH] strbuf: release memory on read error in strbuf_read_once()

## tl;dr

2 messages between Dec 7, 2017 and Dec 7, 2017. Diffs are folded; open one to read it.

replies: 1people: 2as markdown or json

René Scharfe· Dec 7, 2017, 20:51 UTC · lore

If other strbuf add functions cause the first allocation and subsequently encounter an error then they release the memory, restoring the pristine state of the strbuf. That simplifies error handling for callers.

Do the same in strbuf_read_once(), and do it also in case no bytes were read -- which may or may not be an error as well, depending on the caller.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 strbuf.c | 3 +++
 1 file changed, 3 insertions(+)

Show changes to strbuf.c +3 −0

diff --git a/strbuf.c b/strbuf.c
index 323c49ceb3..ac5a7ab62d 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -386,12 +386,15 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)
 
 ssize_t strbuf_read_once(struct strbuf *sb, int fd, size_t hint)
 {
+	size_t oldalloc = sb->alloc;
 	ssize_t cnt;
 
 	strbuf_grow(sb, hint ? hint : 8192);
 	cnt = xread(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);
 	if (cnt > 0)
 		strbuf_setlen(sb, sb->len + cnt);
+	else if (oldalloc == 0)
+		strbuf_release(sb);
 	return cnt;
 }

-- 
2.15.1

Jeff King· Dec 7, 2017, 21:28 UTC · re: René Scharfe · lore

Re: [PATCH] strbuf: release memory on read error in strbuf_read_once()

On Thu, Dec 07, 2017 at 09:51:26PM +0100, René Scharfe wrote:

Show 8 quoted lines

> If other strbuf add functions cause the first allocation and
> subsequently encounter an error then they release the memory, restoring
> the pristine state of the strbuf.  That simplifies error handling for
> callers.
> 
> Do the same in strbuf_read_once(), and do it also in case no bytes were
> read -- which may or may not be an error as well, depending on the
> caller.

Makes sense, and the patch is delightfully simple.

For the "0" case nobody should be negatively impacted by dropping the allocation, as they get a sane 0-length string from the slopbuf (and anybody who relies on sb->buf being allocated without calling detach or similar is already doing it wrong).

-Peff