{"thread":{"id":"45498","subject":"[PATCH 0/4] a few minor buffer cleanups in fast-import","startedAt":"2017-03-24T17:23:04Z","lastAt":"2017-03-24T17:33:15Z","messageCount":5,"participants":["Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"315278","messageId":"20170324172246.fy5drvhzqxghu44a@sigill.intra.peff.net","threadId":"45498","inReplyTo":null,"subject":"[PATCH 0/4] a few minor buffer cleanups in fast-import","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-24T17:22:46Z","receivedAt":"2017-03-24T17:23:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I don't think any of these is a triggerable bug. They're just cleanups\nto make it more obvious that the code is doing the right thing (and\nmaking it harder to do the wrong thing).\n\n  [1/4]: fast-import: use xsnprintf for writing sha1s\n  [2/4]: fast-import: use xsnprintf for formatting headers\n  [3/4]: encode_in_pack_object_header: respect output buffer length\n  [4/4]: pack.h: define largest possible encoded object size\n\n builtin/pack-objects.c | 12 ++++++++----\n bulk-checkin.c         |  2 +-\n fast-import.c          | 16 +++++++---------\n pack-write.c           |  5 ++++-\n pack.h                 |  9 ++++++++-\n 5 files changed, 28 insertions(+), 16 deletions(-)\n\n-Peff\n"},{"id":"315279","messageId":"20170324172522.l4sawf2u42mrnswk@sigill.intra.peff.net","threadId":"45498","inReplyTo":"20170324172246.fy5drvhzqxghu44a@sigill.intra.peff.net","subject":"[PATCH 1/4] fast-import: use xsnprintf for writing sha1s","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-24T17:25:22Z","receivedAt":"2017-03-24T17:25:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we have to write a sha1 with a newline, we do so by\ncopying both into a single buffer, so that we can issue a\nsingle write() call.\n\nWe use snprintf but don't bother to check the output, since\nwe know it will fit. However, we should use xsnprintf() in\nsuch a case so that we're notified if our assumption turns\nout to be wrong (and to make it easier to audit for\nunchecked snprintf calls).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis is ready for conversion to oid_to_hex, too, but I avoided that\nhere. The buffer would need to be allocated with the new GIT_MAX_HEXSZ,\nwhich is not yet available. So I figured it was better to leave it than\nhalf-convert it and leave brian wondering whether it's really supposed\nto be GIT_MAX_HEXSZ or GIT_SHA1_HEXSZ.\n\n fast-import.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 41a539f97..4e0f3f5dd 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -3003,7 +3003,7 @@ static void parse_get_mark(const char *p)\n \tif (!oe)\n \t\tdie(\"Unknown mark: %s\", command_buf.buf);\n \n-\tsnprintf(output, sizeof(output), \"%s\\n\", sha1_to_hex(oe->idx.sha1));\n+\txsnprintf(output, sizeof(output), \"%s\\n\", sha1_to_hex(oe->idx.sha1));\n \tcat_blob_write(output, 41);\n }\n \n-- \n2.12.1.843.g1937c56c2\n\n"},{"id":"315280","messageId":"20170324172650.x76jk4ahef6tvvjw@sigill.intra.peff.net","threadId":"45498","inReplyTo":"20170324172246.fy5drvhzqxghu44a@sigill.intra.peff.net","subject":"[PATCH 4/4] pack.h: define largest possible encoded object size","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-24T17:26:50Z","receivedAt":"2017-03-24T17:27:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Several callers use fixed buffers for storing the pack\nobject header, and they've picked 10 as a magic number. This\nis reasonable, since it handles objects up to 2^67. But\nlet's give them a constant so it's clear that the number\nisn't pulled out of thin air.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/pack-objects.c | 6 ++++--\n pack.h                 | 6 ++++++\n 2 files changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex cf3fc50a2..84af7c232 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -239,7 +239,8 @@ static unsigned long write_no_reuse_object(struct sha1file *f, struct object_ent\n \t\t\t\t\t   unsigned long limit, int usable_delta)\n {\n \tunsigned long size, datalen;\n-\tunsigned char header[10], dheader[10];\n+\tunsigned char header[MAX_PACK_OBJECT_HEADER],\n+\t\t      dheader[MAX_PACK_OBJECT_HEADER];\n \tunsigned hdrlen;\n \tenum object_type type;\n \tvoid *buf;\n@@ -353,7 +354,8 @@ static off_t write_reuse_object(struct sha1file *f, struct object_entry *entry,\n \toff_t offset;\n \tenum object_type type = entry->type;\n \toff_t datalen;\n-\tunsigned char header[10], dheader[10];\n+\tunsigned char header[MAX_PACK_OBJECT_HEADER],\n+\t\t      dheader[MAX_PACK_OBJECT_HEADER];\n \tunsigned hdrlen;\n \n \tif (entry->delta)\ndiff --git a/pack.h b/pack.h\nindex 87e456d5e..5c2158746 100644\n--- a/pack.h\n+++ b/pack.h\n@@ -84,6 +84,12 @@ extern int verify_pack(struct packed_git *, verify_fn fn, struct progress *, uin\n extern off_t write_pack_header(struct sha1file *f, uint32_t);\n extern void fixup_pack_header_footer(int, unsigned char *, const char *, uint32_t, unsigned char *, off_t);\n extern char *index_pack_lockfile(int fd);\n+\n+/*\n+ * The \"hdr\" output buffer should be at least this big, which will handle sizes\n+ * up to 2^67.\n+ */\n+#define MAX_PACK_OBJECT_HEADER 10\n extern int encode_in_pack_object_header(unsigned char *hdr, int hdr_len,\n \t\t\t\t\tenum object_type, uintmax_t);\n \n-- \n2.12.1.843.g1937c56c2\n"},{"id":"315281","messageId":"20170324172640.s6esvcbxfj2x63eb@sigill.intra.peff.net","threadId":"45498","inReplyTo":"20170324172246.fy5drvhzqxghu44a@sigill.intra.peff.net","subject":"[PATCH 3/4] encode_in_pack_object_header: respect output buffer length","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-24T17:26:40Z","receivedAt":"2017-03-24T17:27:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The encode_in_pack_object_header() writes a variable-length\nheader to an output buffer, but it doesn't actually know\nlong the buffer is. At first glance, this looks like it\nmight be possible to overflow.\n\nIn practice, this is probably impossible. The smallest\nbuffer we use is 10 bytes, which would hold the header for\nan object up to 2^67 bytes. Obviously we're not likely to\nsee such an object, but we might worry that an object could\nlie about its size (causing us to overflow before we realize\nit does not actually have that many bytes). But the argument\nis passed as a uintmax_t. Even on systems that have __int128\navailable, uintmax_t is typically restricted to 64-bit by\nthe ABI.\n\nSo it's unlikely that a system exists where this could be\nexploited. Still, it's easy enough to use a normal out/len\npair and make sure we don't write too far. That protects the\nhypothetical 128-bit system, makes it harder for callers to\naccidentally specify a too-small buffer, and makes the\nresulting code easier to audit.\n\nNote that the one caller in fast-import tried to catch such\na case, but did so _after_ the call (at which point we'd\nhave already overflowed!). This check can now go away.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/pack-objects.c |  6 ++++--\n bulk-checkin.c         |  2 +-\n fast-import.c          | 10 +++++-----\n pack-write.c           |  5 ++++-\n pack.h                 |  3 ++-\n 5 files changed, 16 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 16517f263..cf3fc50a2 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -286,7 +286,8 @@ static unsigned long write_no_reuse_object(struct sha1file *f, struct object_ent\n \t * The object header is a byte of 'type' followed by zero or\n \t * more bytes of length.\n \t */\n-\thdrlen = encode_in_pack_object_header(type, size, header);\n+\thdrlen = encode_in_pack_object_header(header, sizeof(header),\n+\t\t\t\t\t      type, size);\n \n \tif (type == OBJ_OFS_DELTA) {\n \t\t/*\n@@ -358,7 +359,8 @@ static off_t write_reuse_object(struct sha1file *f, struct object_entry *entry,\n \tif (entry->delta)\n \t\ttype = (allow_ofs_delta && entry->delta->idx.offset) ?\n \t\t\tOBJ_OFS_DELTA : OBJ_REF_DELTA;\n-\thdrlen = encode_in_pack_object_header(type, entry->size, header);\n+\thdrlen = encode_in_pack_object_header(header, sizeof(header),\n+\t\t\t\t\t      type, entry->size);\n \n \toffset = entry->in_pack_offset;\n \trevidx = find_pack_revindex(p, offset);\ndiff --git a/bulk-checkin.c b/bulk-checkin.c\nindex 991b4a13e..ddb6070c4 100644\n--- a/bulk-checkin.c\n+++ b/bulk-checkin.c\n@@ -105,7 +105,7 @@ static int stream_to_pack(struct bulk_checkin_state *state,\n \n \tgit_deflate_init(&s, pack_compression_level);\n \n-\thdrlen = encode_in_pack_object_header(type, size, obuf);\n+\thdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), type, size);\n \ts.next_out = obuf + hdrlen;\n \ts.avail_out = sizeof(obuf) - hdrlen;\n \ndiff --git a/fast-import.c b/fast-import.c\nindex 4a057e81f..f6f416f20 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1173,7 +1173,8 @@ static int store_object(\n \t\tdelta_count_by_type[type]++;\n \t\te->depth = last->depth + 1;\n \n-\t\thdrlen = encode_in_pack_object_header(OBJ_OFS_DELTA, deltalen, hdr);\n+\t\thdrlen = encode_in_pack_object_header(hdr, sizeof(hdr),\n+\t\t\t\t\t\t      OBJ_OFS_DELTA, deltalen);\n \t\tsha1write(pack_file, hdr, hdrlen);\n \t\tpack_size += hdrlen;\n \n@@ -1184,7 +1185,8 @@ static int store_object(\n \t\tpack_size += sizeof(hdr) - pos;\n \t} else {\n \t\te->depth = 0;\n-\t\thdrlen = encode_in_pack_object_header(type, dat->len, hdr);\n+\t\thdrlen = encode_in_pack_object_header(hdr, sizeof(hdr),\n+\t\t\t\t\t\t      type, dat->len);\n \t\tsha1write(pack_file, hdr, hdrlen);\n \t\tpack_size += hdrlen;\n \t}\n@@ -1246,9 +1248,7 @@ static void stream_blob(uintmax_t len, unsigned char *sha1out, uintmax_t mark)\n \n \tgit_deflate_init(&s, pack_compression_level);\n \n-\thdrlen = encode_in_pack_object_header(OBJ_BLOB, len, out_buf);\n-\tif (out_sz <= hdrlen)\n-\t\tdie(\"impossibly large object header\");\n+\thdrlen = encode_in_pack_object_header(out_buf, out_sz, OBJ_BLOB, len);\n \n \ts.next_out = out_buf + hdrlen;\n \ts.avail_out = out_sz - hdrlen;\ndiff --git a/pack-write.c b/pack-write.c\nindex 88bc7f9f7..c057513f1 100644\n--- a/pack-write.c\n+++ b/pack-write.c\n@@ -304,7 +304,8 @@ char *index_pack_lockfile(int ip_out)\n  *  - each byte afterwards: low seven bits are size continuation,\n  *    with the high bit being \"size continues\"\n  */\n-int encode_in_pack_object_header(enum object_type type, uintmax_t size, unsigned char *hdr)\n+int encode_in_pack_object_header(unsigned char *hdr, int hdr_len,\n+\t\t\t\t enum object_type type, uintmax_t size)\n {\n \tint n = 1;\n \tunsigned char c;\n@@ -315,6 +316,8 @@ int encode_in_pack_object_header(enum object_type type, uintmax_t size, unsigned\n \tc = (type << 4) | (size & 15);\n \tsize >>= 4;\n \twhile (size) {\n+\t\tif (n == hdr_len)\n+\t\t\tdie(\"object size is too enormous to format\");\n \t\t*hdr++ = c | 0x80;\n \t\tc = size & 0x7f;\n \t\tsize >>= 7;\ndiff --git a/pack.h b/pack.h\nindex 0e77429df..87e456d5e 100644\n--- a/pack.h\n+++ b/pack.h\n@@ -84,7 +84,8 @@ extern int verify_pack(struct packed_git *, verify_fn fn, struct progress *, uin\n extern off_t write_pack_header(struct sha1file *f, uint32_t);\n extern void fixup_pack_header_footer(int, unsigned char *, const char *, uint32_t, unsigned char *, off_t);\n extern char *index_pack_lockfile(int fd);\n-extern int encode_in_pack_object_header(enum object_type, uintmax_t, unsigned char *);\n+extern int encode_in_pack_object_header(unsigned char *hdr, int hdr_len,\n+\t\t\t\t\tenum object_type, uintmax_t);\n \n #define PH_ERROR_EOF\t\t(-1)\n #define PH_ERROR_PACK_SIGNATURE\t(-2)\n-- \n2.12.1.843.g1937c56c2\n\n"},{"id":"315283","messageId":"20170324172624.ddqquklvam2emn24@sigill.intra.peff.net","threadId":"45498","inReplyTo":"20170324172246.fy5drvhzqxghu44a@sigill.intra.peff.net","subject":"[PATCH 2/4] fast-import: use xsnprintf for formatting headers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-24T17:26:24Z","receivedAt":"2017-03-24T17:33:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The stream_blob() function checks the return value of\nsnprintf and dies. This is more simply done with\nxsnprintf (and matches the similar call in store_object).\n\nThe message the user would get is less specific, but since\nthe point is that this _shouldn't_ ever happen, that's OK.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n fast-import.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 4e0f3f5dd..4a057e81f 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1237,9 +1237,7 @@ static void stream_blob(uintmax_t len, unsigned char *sha1out, uintmax_t mark)\n \tsha1file_checkpoint(pack_file, &checkpoint);\n \toffset = checkpoint.offset;\n \n-\thdrlen = snprintf((char *)out_buf, out_sz, \"blob %\" PRIuMAX, len) + 1;\n-\tif (out_sz <= hdrlen)\n-\t\tdie(\"impossibly large object header\");\n+\thdrlen = xsnprintf((char *)out_buf, out_sz, \"blob %\" PRIuMAX, len) + 1;\n \n \tgit_SHA1_Init(&c);\n \tgit_SHA1_Update(&c, out_buf, hdrlen);\n-- \n2.12.1.843.g1937c56c2\n\n"}]}