{"thread":{"id":"48945","subject":"[PATCH 0/6] use size_t in iconv/strbuf","startedAt":"2018-07-24T10:48:55Z","lastAt":"2018-07-26T06:09:55Z","messageCount":9,"participants":["Jeff King","Torsten Bögershausen"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"353456","messageId":"20180724104852.GA14638@sigill.intra.peff.net","threadId":"48945","inReplyTo":null,"subject":"[PATCH 0/6] use size_t in iconv/strbuf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-24T10:48:52Z","receivedAt":"2018-07-24T10:48:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This series is primarily about the first two patches to convert our\niconv helpers to use size_t consistently. I posted them to the\ngit-security list a while back, wondering if there was something sneaky\nyou could do here. But after some discussion, the consensus was no, you\ncan't.\n\nThe other four patches are just semi-related cleanups I saw while poking\naround the strbuf code. I doubt any of them fixes a user-visible bug,\nbut I think they're worth doing (and they don't seem to conflict with\nanything on pu).\n\n  [1/6]: reencode_string: use st_add/st_mult helpers\n  [2/6]: reencode_string: use size_t for string lengths\n  [3/6]: strbuf: use size_t for length in intermediate variables\n  [4/6]: strbuf_readlink: use ssize_t\n  [5/6]: pass st.st_size as hint for strbuf_readlink()\n  [6/6]: strbuf_humanise: use unsigned variables\n\n builtin/init-db.c    |  3 ++-\n convert.c            |  6 +++---\n pretty.c             |  2 +-\n refs/files-backend.c |  2 +-\n strbuf.c             | 20 ++++++++++----------\n utf8.c               | 10 +++++-----\n utf8.h               | 10 +++++-----\n 7 files changed, 27 insertions(+), 26 deletions(-)\n\n-Peff\n"},{"id":"353457","messageId":"20180724105009.GA17165@sigill.intra.peff.net","threadId":"48945","inReplyTo":"20180724104852.GA14638@sigill.intra.peff.net","subject":"[PATCH 1/6] reencode_string: use st_add/st_mult helpers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-24T10:50:10Z","receivedAt":"2018-07-24T10:50:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When converting a string with iconv, if the output buffer\nisn't big enough, we grow it. But our growth is done without\nany concern for integer overflow. So when we add:\n\n  outalloc = sofar + insz * 2 + 32;\n\nwe may end up wrapping outalloc (which is a size_t), and\nallocating a too-small buffer. We then manipulate it\nfurther:\n\n  outsz = outalloc - sofar - 1;\n\nand feed outsz back to iconv. If outalloc is wrapped and\nsmaller than sofar, we'll end up with a small allocation but\nfeed a very large outsz to iconv, which could result in it\noverflowing the buffer.\n\nCan we use this to construct an attack wherein the victim\nclones a repository with a very large commit object with an\nencoding header, and running \"git log\" reencodes it into\nutf8, causing an overflow?\n\nAn attack of this sort is likely impossible in practice.\n\"sofar\" is how many output bytes we've written total, and\n\"insz\" is the number of input bytes remaining. Imagine our\ninput doubles in size as we output it (which is easy to do\nby converting latin1 to utf8, for example), and that we\nstart with N input bytes. Our initial output buffer also\nstarts at N bytes, so after the first call we'd have N/2\ninput bytes remaining (insz), and have written N bytes\n(sofar). That means our next allocation will be\n(N + N/2 * 2 + 32) bytes, or (2N + 32).\n\nWe can therefore overflow a 32-bit size_t with a commit\nmessage that's just under 2^31 bytes, assuming it consists\nmostly of \"doubling\" sequences (e.g., latin1 0xe1 which\nbecomes utf8 0xc3 0xa1).\n\nBut we'll never make it that far with such a message. We'll\nbe spending 2^31 bytes on the original string. And our\ninitial output buffer will also be 2^31 bytes. Which is not\ngoing to succeed on a system with a 32-bit size_t, since\nthere will be other things using the address space, too. The\ninitial malloc will fail.\n\nIf we imagine instead that we can triple the size when\nconverting, then our second allocation becomes\n(N + 2/3N * 2 + 32), or (7/3N + 32). That still requires two\nallocations of 3/7 of our address space (6/7 of the total)\nto succeed.\n\nIf we imagine we can quadruple, it becomes (5/2N + 32); we\nneed to be able to allocate 4/5 of the address space to\nsucceed.\n\nThis might start to get plausible. But is it possible to get\na 4-to-1 increase in size? Probably if you're converting to\nsome obscure encoding. But since git defaults to utf8 for\nits output, that's the likely destination encoding for an\nattack. And while there are 4-character utf8 sequences, it's\nunlikely that you'd be able find a single-byte source\nsequence in any encoding.\n\nSo this is certainly buggy code which should be fixed, but\nit is probably not a useful attack vector.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n utf8.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/utf8.c b/utf8.c\nindex d55e20c641..a2fd24c70a 100644\n--- a/utf8.c\n+++ b/utf8.c\n@@ -477,7 +477,7 @@ char *reencode_string_iconv(const char *in, size_t insz, iconv_t conv, int *outs\n \ticonv_ibp cp;\n \n \toutsz = insz;\n-\toutalloc = outsz + 1; /* for terminating NUL */\n+\toutalloc = st_add(outsz, 1); /* for terminating NUL */\n \tout = xmalloc(outalloc);\n \toutpos = out;\n \tcp = (iconv_ibp)in;\n@@ -497,7 +497,7 @@ char *reencode_string_iconv(const char *in, size_t insz, iconv_t conv, int *outs\n \t\t\t * converting the rest.\n \t\t\t */\n \t\t\tsofar = outpos - out;\n-\t\t\toutalloc = sofar + insz * 2 + 32;\n+\t\t\toutalloc = st_add3(sofar, st_mult(insz, 2), 32);\n \t\t\tout = xrealloc(out, outalloc);\n \t\t\toutpos = out + sofar;\n \t\t\toutsz = outalloc - sofar - 1;\n-- \n2.18.0.542.g2bf2fc4f7e\n\n"},{"id":"353458","messageId":"20180724105032.GB17165@sigill.intra.peff.net","threadId":"48945","inReplyTo":"20180724104852.GA14638@sigill.intra.peff.net","subject":"[PATCH 2/6] reencode_string: use size_t for string lengths","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-24T10:50:33Z","receivedAt":"2018-07-24T10:50:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The iconv interface takes a size_t, which is the appropriate\ntype for an in-memory buffer. But our reencode_string_*\nfunctions use integers, meaning we may get confusing results\nwhen the sizes exceed INT_MAX. Let's use size_t\nconsistently.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n convert.c |  6 +++---\n pretty.c  |  2 +-\n strbuf.c  |  2 +-\n utf8.c    |  6 +++---\n utf8.h    | 10 +++++-----\n 5 files changed, 13 insertions(+), 13 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 56cfe31ec5..530a43cf63 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -390,7 +390,7 @@ static int encode_to_git(const char *path, const char *src, size_t src_len,\n \t\t\t struct strbuf *buf, const char *enc, int conv_flags)\n {\n \tchar *dst;\n-\tint dst_len;\n+\tsize_t dst_len;\n \tint die_on_error = conv_flags & CONV_WRITE_OBJECT;\n \n \t/*\n@@ -453,7 +453,7 @@ static int encode_to_git(const char *path, const char *src, size_t src_len,\n \t */\n \tif (die_on_error && check_roundtrip(enc)) {\n \t\tchar *re_src;\n-\t\tint re_src_len;\n+\t\tsize_t re_src_len;\n \n \t\tre_src = reencode_string_len(dst, dst_len,\n \t\t\t\t\t     enc, default_encoding,\n@@ -481,7 +481,7 @@ static int encode_to_worktree(const char *path, const char *src, size_t src_len,\n \t\t\t      struct strbuf *buf, const char *enc)\n {\n \tchar *dst;\n-\tint dst_len;\n+\tsize_t dst_len;\n \n \t/*\n \t * No encoding is specified or there is nothing to encode.\ndiff --git a/pretty.c b/pretty.c\nindex 703fa6ff7b..e1e4060243 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1538,7 +1538,7 @@ void format_commit_message(const struct commit *commit,\n \t}\n \n \tif (output_enc) {\n-\t\tint outsz;\n+\t\tsize_t outsz;\n \t\tchar *out = reencode_string_len(sb->buf, sb->len,\n \t\t\t\t\t\toutput_enc, utf8, &outsz);\n \t\tif (out)\ndiff --git a/strbuf.c b/strbuf.c\nindex b0716ac585..e79758b942 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -134,7 +134,7 @@ void strbuf_ltrim(struct strbuf *sb)\n int strbuf_reencode(struct strbuf *sb, const char *from, const char *to)\n {\n \tchar *out;\n-\tint len;\n+\tsize_t len;\n \n \tif (same_encoding(from, to))\n \t\treturn 0;\ndiff --git a/utf8.c b/utf8.c\nindex a2fd24c70a..edcd1e835a 100644\n--- a/utf8.c\n+++ b/utf8.c\n@@ -470,7 +470,7 @@ int utf8_fprintf(FILE *stream, const char *format, ...)\n #else\n \ttypedef char * iconv_ibp;\n #endif\n-char *reencode_string_iconv(const char *in, size_t insz, iconv_t conv, int *outsz_p)\n+char *reencode_string_iconv(const char *in, size_t insz, iconv_t conv, size_t *outsz_p)\n {\n \tsize_t outsz, outalloc;\n \tchar *out, *outpos;\n@@ -534,9 +534,9 @@ static const char *fallback_encoding(const char *name)\n \treturn name;\n }\n \n-char *reencode_string_len(const char *in, int insz,\n+char *reencode_string_len(const char *in, size_t insz,\n \t\t\t  const char *out_encoding, const char *in_encoding,\n-\t\t\t  int *outsz)\n+\t\t\t  size_t *outsz)\n {\n \ticonv_t conv;\n \tchar *out;\ndiff --git a/utf8.h b/utf8.h\nindex db73a2d8d3..ce1c2696e0 100644\n--- a/utf8.h\n+++ b/utf8.h\n@@ -25,14 +25,14 @@ void strbuf_utf8_replace(struct strbuf *sb, int pos, int width,\n \n #ifndef NO_ICONV\n char *reencode_string_iconv(const char *in, size_t insz,\n-\t\t\t    iconv_t conv, int *outsz);\n-char *reencode_string_len(const char *in, int insz,\n+\t\t\t    iconv_t conv, size_t *outsz);\n+char *reencode_string_len(const char *in, size_t insz,\n \t\t\t  const char *out_encoding,\n \t\t\t  const char *in_encoding,\n-\t\t\t  int *outsz);\n+\t\t\t  size_t *outsz);\n #else\n-static inline char *reencode_string_len(const char *a, int b,\n-\t\t\t\t\tconst char *c, const char *d, int *e)\n+static inline char *reencode_string_len(const char *a, size_t b,\n+\t\t\t\t\tconst char *c, const char *d, size_t *e)\n { if (e) *e = 0; return NULL; }\n #endif\n \n-- \n2.18.0.542.g2bf2fc4f7e\n\n"},{"id":"353459","messageId":"20180724105108.GC17165@sigill.intra.peff.net","threadId":"48945","inReplyTo":"20180724104852.GA14638@sigill.intra.peff.net","subject":"[PATCH 3/6] strbuf: use size_t for length in intermediate variables","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-24T10:51:08Z","receivedAt":"2018-07-24T10:51:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"A few strbuf functions store the length of a strbuf in a\ntemporary variable. We should always use size_t for this, as\nit's possible for a strbuf to exceed an \"int\" (e.g., a 2GB\nstring on a 64-bit system). This is unlikely in practice,\nbut we should try to behave sensibly on silly or malicious\ninput.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n strbuf.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex e79758b942..6ff1f80129 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -209,7 +209,7 @@ void strbuf_list_free(struct strbuf **sbs)\n \n int strbuf_cmp(const struct strbuf *a, const struct strbuf *b)\n {\n-\tint len = a->len < b->len ? a->len: b->len;\n+\tsize_t len = a->len < b->len ? a->len: b->len;\n \tint cmp = memcmp(a->buf, b->buf, len);\n \tif (cmp)\n \t\treturn cmp;\n@@ -389,7 +389,7 @@ size_t strbuf_expand_dict_cb(struct strbuf *sb, const char *placeholder,\n \n void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *src)\n {\n-\tint i, len = src->len;\n+\tsize_t i, len = src->len;\n \n \tfor (i = 0; i < len; i++) {\n \t\tif (src->buf[i] == '%')\n@@ -960,7 +960,7 @@ static size_t cleanup(char *line, size_t len)\n  */\n void strbuf_stripspace(struct strbuf *sb, int skip_comments)\n {\n-\tint empties = 0;\n+\tsize_t empties = 0;\n \tsize_t i, j, len, newlen;\n \tchar *eol;\n \n-- \n2.18.0.542.g2bf2fc4f7e\n\n"},{"id":"353460","messageId":"20180724105125.GD17165@sigill.intra.peff.net","threadId":"48945","inReplyTo":"20180724104852.GA14638@sigill.intra.peff.net","subject":"[PATCH 4/6] strbuf_readlink: use ssize_t","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-24T10:51:25Z","receivedAt":"2018-07-24T10:51:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The return type of readlink() is ssize_t, not int. This\nprobably doesn't matter in practice, as it would require a\n2GB symlink destination, but it doesn't hurt to be careful.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n strbuf.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex 6ff1f80129..db9069c937 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -469,7 +469,7 @@ int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)\n \t\thint = 32;\n \n \twhile (hint < STRBUF_MAXLINK) {\n-\t\tint len;\n+\t\tssize_t len;\n \n \t\tstrbuf_grow(sb, hint);\n \t\tlen = readlink(path, sb->buf, hint);\n-- \n2.18.0.542.g2bf2fc4f7e\n\n"},{"id":"353461","messageId":"20180724105139.GE17165@sigill.intra.peff.net","threadId":"48945","inReplyTo":"20180724104852.GA14638@sigill.intra.peff.net","subject":"[PATCH 5/6] pass st.st_size as hint for strbuf_readlink()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-24T10:51:39Z","receivedAt":"2018-07-24T10:51:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we initially added the strbuf_readlink() function in\nb11b7e13f4 (Add generic 'strbuf_readlink()' helper function,\n2008-12-17), the point was that we generally have a _guess_\nas to the correct size based on the stat information, but we\ncan't necessarily trust it.\n\nOver the years, a few callers have grown up that simply pass\nin 0, even though they have the stat information. Let's have\nthem pass in their hint for consistency (and in theory\nefficiency, since it may avoid an extra resize/syscall loop,\nbut neither location is probably performance critical).\n\nNote that st.st_size is actually an off_t, so in theory we\nneed xsize_t() here. But none of the other callsites use it,\nand since this is just a hint, it doesn't matter either way\n(if we wrap we'll simply start with a too-small hint and\nthen eventually complain when we cannot allocate the\nmemory).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/init-db.c    | 3 ++-\n refs/files-backend.c | 2 +-\n 2 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex 4ecf909368..12ddda7e7b 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -73,7 +73,8 @@ static void copy_templates_1(struct strbuf *path, struct strbuf *template_path,\n \t\t\tcontinue;\n \t\telse if (S_ISLNK(st_template.st_mode)) {\n \t\t\tstruct strbuf lnk = STRBUF_INIT;\n-\t\t\tif (strbuf_readlink(&lnk, template_path->buf, 0) < 0)\n+\t\t\tif (strbuf_readlink(&lnk, template_path->buf,\n+\t\t\t\t\t    st_template.st_size) < 0)\n \t\t\t\tdie_errno(_(\"cannot readlink '%s'\"), template_path->buf);\n \t\t\tif (symlink(lnk.buf, path->buf))\n \t\t\t\tdie_errno(_(\"cannot symlink '%s' '%s'\"),\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex a9a066dcfb..c110c2520c 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -363,7 +363,7 @@ static int files_read_raw_ref(struct ref_store *ref_store,\n \t/* Follow \"normalized\" - ie \"refs/..\" symlinks by hand */\n \tif (S_ISLNK(st.st_mode)) {\n \t\tstrbuf_reset(&sb_contents);\n-\t\tif (strbuf_readlink(&sb_contents, path, 0) < 0) {\n+\t\tif (strbuf_readlink(&sb_contents, path, st.st_size) < 0) {\n \t\t\tif (errno == ENOENT || errno == EINVAL)\n \t\t\t\t/* inconsistent with lstat; retry */\n \t\t\t\tgoto stat_ref;\n-- \n2.18.0.542.g2bf2fc4f7e\n\n"},{"id":"353462","messageId":"20180724105229.GF17165@sigill.intra.peff.net","threadId":"48945","inReplyTo":"20180724104852.GA14638@sigill.intra.peff.net","subject":"[PATCH 6/6] strbuf_humanise: use unsigned variables","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-24T10:52:29Z","receivedAt":"2018-07-24T10:52:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"All of the numeric formatting done by this function uses\n\"%u\", but we pass in a signed \"int\". The actual range\ndoesn't matter here, since the conditional makes sure we're\nalways showing reasonably small numbers. And even gcc's\nformat-checker does not seem to mind. But it's potentially\nconfusing to a reader of the code to see the mismatch.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n strbuf.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex db9069c937..54f29bbb23 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -734,18 +734,18 @@ void strbuf_humanise_bytes(struct strbuf *buf, off_t bytes)\n {\n \tif (bytes > 1 << 30) {\n \t\tstrbuf_addf(buf, \"%u.%2.2u GiB\",\n-\t\t\t    (int)(bytes >> 30),\n-\t\t\t    (int)(bytes & ((1 << 30) - 1)) / 10737419);\n+\t\t\t    (unsigned)(bytes >> 30),\n+\t\t\t    (unsigned)(bytes & ((1 << 30) - 1)) / 10737419);\n \t} else if (bytes > 1 << 20) {\n-\t\tint x = bytes + 5243;  /* for rounding */\n+\t\tunsigned x = bytes + 5243;  /* for rounding */\n \t\tstrbuf_addf(buf, \"%u.%2.2u MiB\",\n \t\t\t    x >> 20, ((x & ((1 << 20) - 1)) * 100) >> 20);\n \t} else if (bytes > 1 << 10) {\n-\t\tint x = bytes + 5;  /* for rounding */\n+\t\tunsigned x = bytes + 5;  /* for rounding */\n \t\tstrbuf_addf(buf, \"%u.%2.2u KiB\",\n \t\t\t    x >> 10, ((x & ((1 << 10) - 1)) * 100) >> 10);\n \t} else {\n-\t\tstrbuf_addf(buf, \"%u bytes\", (int)bytes);\n+\t\tstrbuf_addf(buf, \"%u bytes\", (unsigned)bytes);\n \t}\n }\n \n-- \n2.18.0.542.g2bf2fc4f7e\n"},{"id":"353582","messageId":"20180725184100.GA30961@tor.lan","threadId":"48945","inReplyTo":"20180724105139.GE17165@sigill.intra.peff.net","subject":"Re: [PATCH 5/6] pass st.st_size as hint for strbuf_readlink()","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2018-07-25T18:41:00Z","receivedAt":"2018-07-25T18:41:06Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Tue, Jul 24, 2018 at 06:51:39AM -0400, Jeff King wrote:\n> When we initially added the strbuf_readlink() function in\n> b11b7e13f4 (Add generic 'strbuf_readlink()' helper function,\n> 2008-12-17), the point was that we generally have a _guess_\n> as to the correct size based on the stat information, but we\n> can't necessarily trust it.\n> \n> Over the years, a few callers have grown up that simply pass\n> in 0, even though they have the stat information. Let's have\n> them pass in their hint for consistency (and in theory\n> efficiency, since it may avoid an extra resize/syscall loop,\n> but neither location is probably performance critical).\n> \n> Note that st.st_size is actually an off_t, so in theory we\n> need xsize_t() here. But none of the other callsites use it,\n> and since this is just a hint, it doesn't matter either way\n> (if we wrap we'll simply start with a too-small hint and\n> then eventually complain when we cannot allocate the\n> memory).\n\nThanks a lot for the series.\n\n For the last paragraph I would actually vote the other way around -\n how about something like this ?\n Note that st.st_size is actually an off_t, so we should use\n xsize_t() here. In pratise we don't expect links to be large like that,\n but let's give a good example in the source code and use xsize_t()\n whenever an off_t is converted into size_t.\n This will make live easier whenever someones diggs into 32/64 bit\n \"conversion safetyness\"\n\n"},{"id":"353614","messageId":"20180726060951.GA25386@sigill.intra.peff.net","threadId":"48945","inReplyTo":"20180725184100.GA30961@tor.lan","subject":"Re: [PATCH 5/6] pass st.st_size as hint for strbuf_readlink()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-26T06:09:51Z","receivedAt":"2018-07-26T06:09:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 25, 2018 at 08:41:00PM +0200, Torsten Bögershausen wrote:\n\n> On Tue, Jul 24, 2018 at 06:51:39AM -0400, Jeff King wrote:\n> > When we initially added the strbuf_readlink() function in\n> > b11b7e13f4 (Add generic 'strbuf_readlink()' helper function,\n> > 2008-12-17), the point was that we generally have a _guess_\n> > as to the correct size based on the stat information, but we\n> > can't necessarily trust it.\n> > \n> > Over the years, a few callers have grown up that simply pass\n> > in 0, even though they have the stat information. Let's have\n> > them pass in their hint for consistency (and in theory\n> > efficiency, since it may avoid an extra resize/syscall loop,\n> > but neither location is probably performance critical).\n> > \n> > Note that st.st_size is actually an off_t, so in theory we\n> > need xsize_t() here. But none of the other callsites use it,\n> > and since this is just a hint, it doesn't matter either way\n> > (if we wrap we'll simply start with a too-small hint and\n> > then eventually complain when we cannot allocate the\n> > memory).\n> \n> Thanks a lot for the series.\n> \n>  For the last paragraph I would actually vote the other way around -\n>  how about something like this ?\n>  Note that st.st_size is actually an off_t, so we should use\n>  xsize_t() here. In pratise we don't expect links to be large like that,\n>  but let's give a good example in the source code and use xsize_t()\n>  whenever an off_t is converted into size_t.\n>  This will make live easier whenever someones diggs into 32/64 bit\n>  \"conversion safetyness\"\n\nI actually don't mind using xsize_t(), but if we're going into I think\nwe should do it consistently. I.e., as a patch on top with that\nexplanation.\n\n-Peff\n"}]}