# [PATCH] Change strbuf_read_file() to return ssize_t

2 messages from 2015-07-03 to 2015-07-07. Participants: Michael Haggerty, Jeff King.
Thread: https://gitlist.dev/t/39773

## Michael Haggerty, 2015-07-03 13:59

Subject: [PATCH] Change strbuf_read_file() to return ssize_t
Message-ID: <74f6ccbe74486b1d5589e953c4cf6d39c82c5c05.1435931485.git.mhagger@alum.mit.edu>
URL: https://gitlist.dev/e/74f6ccbe74486b1d5589e953c4cf6d39c82c5c05.1435931485.git.mhagger%40alum.mit.edu

```
It is currently declared to return int, which could overflow for large
files.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
This patch is against maint, but it also rebases against master
without conflict.

I couldn't find any way to exploit this bug. Most callers only use
this function for locally-generated files in the first place. And the
correct length of the file is available in strbuf::len, so most
callers only use the return value for a "< 0" check. And they don't do
anything risky on the error path.

 strbuf.c | 5 +++--
 strbuf.h | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/strbuf.c b/strbuf.c
index 88cafd4..b4da9f5 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -481,9 +481,10 @@ int strbuf_getwholeline_fd(struct strbuf *sb, int fd, int term)
 	return 0;
 }
 
-int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint)
+ssize_t strbuf_read_file(struct strbuf *sb, const char *path, size_t hint)
 {
-	int fd, len;
+	int fd;
+	ssize_t len;
 
 	fd = open(path, O_RDONLY);
 	if (fd < 0)
diff --git a/strbuf.h b/strbuf.h
index 1883494..1ea9d0b 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -364,7 +364,7 @@ extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);
  * Read the contents of a file, specified by its path. The third argument
  * can be used to give a hint about the file size, to avoid reallocs.
  */
-extern int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint);
+extern ssize_t strbuf_read_file(struct strbuf *sb, const char *path, size_t hint);
 
 /**
  * Read the target of a symbolic link, specified by its path.  The third
-- 
2.1.4

```

## Jeff King, 2015-07-07 14:33

Subject: Re: [PATCH] Change strbuf_read_file() to return ssize_t
Message-ID: <20150707143350.GB629@peff.net>
URL: https://gitlist.dev/e/20150707143350.GB629%40peff.net
In-Reply-To: <74f6ccbe74486b1d5589e953c4cf6d39c82c5c05.1435931485.git.mhagger@alum.mit.edu>

```
On Fri, Jul 03, 2015 at 03:59:32PM +0200, Michael Haggerty wrote:

> It is currently declared to return int, which could overflow for large
> files.
> 
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
> This patch is against maint, but it also rebases against master
> without conflict.
> 
> I couldn't find any way to exploit this bug. Most callers only use
> this function for locally-generated files in the first place. And the
> correct length of the file is available in strbuf::len, so most
> callers only use the return value for a "< 0" check. And they don't do
> anything risky on the error path.

FWIW, I also looked for problem areas, but couldn't find anything
interesting. But this seems like an obviously good thing to be doing
anyway.

I also wondered if any callers needed to adjust their storage for the
return type to ssize_t (i.e., are we just moving the truncation up one
assignment). But there is only a single caller that assigns the result,
and it uses an ssize_t already.

-Peff

```