{"thread":{"id":"39773","subject":"[PATCH] Change strbuf_read_file() to return ssize_t","startedAt":"2015-07-03T13:59:32Z","lastAt":"2015-07-07T14:33:50Z","messageCount":2,"participants":["Michael Haggerty","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"265458","messageId":"74f6ccbe74486b1d5589e953c4cf6d39c82c5c05.1435931485.git.mhagger@alum.mit.edu","threadId":"39773","inReplyTo":null,"subject":"[PATCH] Change strbuf_read_file() to return ssize_t","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-07-03T13:59:32Z","receivedAt":"2015-07-03T13:59:32Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It is currently declared to return int, which could overflow for large\nfiles.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\nThis patch is against maint, but it also rebases against master\nwithout conflict.\n\nI couldn't find any way to exploit this bug. Most callers only use\nthis function for locally-generated files in the first place. And the\ncorrect length of the file is available in strbuf::len, so most\ncallers only use the return value for a \"< 0\" check. And they don't do\nanything risky on the error path.\n\n strbuf.c | 5 +++--\n strbuf.h | 2 +-\n 2 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex 88cafd4..b4da9f5 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -481,9 +481,10 @@ int strbuf_getwholeline_fd(struct strbuf *sb, int fd, int term)\n \treturn 0;\n }\n \n-int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint)\n+ssize_t strbuf_read_file(struct strbuf *sb, const char *path, size_t hint)\n {\n-\tint fd, len;\n+\tint fd;\n+\tssize_t len;\n \n \tfd = open(path, O_RDONLY);\n \tif (fd < 0)\ndiff --git a/strbuf.h b/strbuf.h\nindex 1883494..1ea9d0b 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -364,7 +364,7 @@ extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);\n  * Read the contents of a file, specified by its path. The third argument\n  * can be used to give a hint about the file size, to avoid reallocs.\n  */\n-extern int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint);\n+extern ssize_t strbuf_read_file(struct strbuf *sb, const char *path, size_t hint);\n \n /**\n  * Read the target of a symbolic link, specified by its path.  The third\n-- \n2.1.4\n"},{"id":"265733","messageId":"20150707143350.GB629@peff.net","threadId":"39773","inReplyTo":"74f6ccbe74486b1d5589e953c4cf6d39c82c5c05.1435931485.git.mhagger@alum.mit.edu","subject":"Re: [PATCH] Change strbuf_read_file() to return ssize_t","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-07-07T14:33:50Z","receivedAt":"2015-07-07T14:33:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 03, 2015 at 03:59:32PM +0200, Michael Haggerty wrote:\n\n> It is currently declared to return int, which could overflow for large\n> files.\n> \n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n> ---\n> This patch is against maint, but it also rebases against master\n> without conflict.\n> \n> I couldn't find any way to exploit this bug. Most callers only use\n> this function for locally-generated files in the first place. And the\n> correct length of the file is available in strbuf::len, so most\n> callers only use the return value for a \"< 0\" check. And they don't do\n> anything risky on the error path.\n\nFWIW, I also looked for problem areas, but couldn't find anything\ninteresting. But this seems like an obviously good thing to be doing\nanyway.\n\nI also wondered if any callers needed to adjust their storage for the\nreturn type to ssize_t (i.e., are we just moving the truncation up one\nassignment). But there is only a single caller that assigns the result,\nand it uses an ssize_t already.\n\n-Peff\n"}]}