{"thread":{"id":"46559","subject":"[PATCH 3/4] Convert zlib.c to size_t","startedAt":"2017-08-10T18:13:28Z","lastAt":"2017-08-11T18:50:28Z","messageCount":6,"participants":["Martin Koegler","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"326049","messageId":"1502388789-5775-1-git-send-email-martin@mail.zuhause","threadId":"46559","inReplyTo":null,"subject":"[PATCH 3/4] Convert zlib.c to size_t","fromName":"Martin Koegler","fromEmail":"martin.koegler@chello.at","sentAt":"2017-08-10T18:13:08Z","receivedAt":"2017-08-10T18:13:28Z","isPatch":true,"sender":{"key":"martin.koegler@chello.at","avatar":"https://avatars.githubusercontent.com/u/1859030?v=4"},"body":"From: Martin Koegler \n\nSigned-off-by: Martin Koegler \n---\n builtin/pack-objects.c | 10 +++++-----\n cache.h | 12 ++++++------\n pack-check.c | 6 +++---\n pack.h | 2 +-\n sha1_file.c | 6 +++---\n wrapper.c | 8 ++++----\n zlib.c | 8 ++++----\n 7 files changed, 26 insertions(+), 26 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex d94fd17..aa70f80 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -222,15 +222,15 @@ static void copy_pack_data(struct sha1file *f,\n \t\tstruct packed_git *p,\n \t\tstruct pack_window **w_curs,\n \t\toff_t offset,\n-\t\toff_t len)\n+\t\tsize_t len)\n {\n \tunsigned char *in;\n-\tunsigned long avail;\n+\tsize_t avail;\n \n \twhile (len) {\n \t\tin = use_pack(p, w_curs, offset, &avail);\n \t\tif (avail > len)\n-\t\t\tavail = (unsigned long)len;\n+\t\t\tavail = len;\n \t\tsha1write(f, in, avail);\n \t\toffset += avail;\n \t\tlen -= avail;\n@@ -1388,8 +1388,8 @@ static void check_object(struct object_entry *entry)\n \t\tstruct pack_window *w_curs = NULL;\n \t\tconst unsigned char *base_ref = NULL;\n \t\tstruct object_entry *base_entry;\n-\t\tunsigned long used, used_0;\n-\t\tunsigned long avail;\n+\t\tsize_t used, used_0;\n+\t\tsize_t avail;\n \t\toff_t ofs;\n \t\tunsigned char *buf, c;\n \ndiff --git a/cache.h b/cache.h\nindex 26a3eaa..9185763 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -42,10 +42,10 @@\n #include \n typedef struct git_zstream {\n \tz_stream z;\n-\tunsigned long avail_in;\n-\tunsigned long avail_out;\n-\tunsigned long total_in;\n-\tunsigned long total_out;\n+\tsize_t avail_in;\n+\tsize_t avail_out;\n+\tsize_t total_in;\n+\tsize_t total_out;\n \tunsigned char *next_in;\n \tunsigned char *next_out;\n } git_zstream;\n@@ -62,7 +62,7 @@ void git_deflate_end(git_zstream *);\n int git_deflate_abort(git_zstream *);\n int git_deflate_end_gently(git_zstream *);\n int git_deflate(git_zstream *, int flush);\n-unsigned long git_deflate_bound(git_zstream *, unsigned long);\n+size_t git_deflate_bound(git_zstream *, size_t);\n \n /* The length in bytes and in hex digits of an object name (SHA-1 value). */\n #define GIT_SHA1_RAWSZ 20\n@@ -1678,7 +1678,7 @@ extern int open_pack_index(struct packed_git *);\n */\n extern void close_pack_index(struct packed_git *);\n \n-extern unsigned char *use_pack(struct packed_git *, struct pack_window **, off_t, unsigned long *);\n+extern unsigned char *use_pack(struct packed_git *, struct pack_window **, off_t, size_t *);\n extern void close_pack_windows(struct packed_git *);\n extern void close_all_packs(void);\n extern void unuse_pack(struct pack_window **);\ndiff --git a/pack-check.c b/pack-check.c\nindex 6f7714f..8cc2364 100644\n--- a/pack-check.c\n+++ b/pack-check.c\n@@ -24,13 +24,13 @@ static int compare_entries(const void *e1, const void *e2)\n }\n \n int check_pack_crc(struct packed_git *p, struct pack_window **w_curs,\n-\t\t off_t offset, off_t len, unsigned int nr)\n+\t\t off_t offset, size_t len, unsigned int nr)\n {\n \tconst uint32_t *index_crc;\n \tuint32_t data_crc = crc32(0, NULL, 0);\n \n \tdo {\n-\t\tunsigned long avail;\n+\t\tsize_t avail;\n \t\tvoid *data = use_pack(p, w_curs, offset, &avail);\n \t\tif (avail > len)\n \t\t\tavail = len;\n@@ -65,7 +65,7 @@ static int verify_packfile(struct packed_git *p,\n \n \tgit_SHA1_Init(&ctx);\n \tdo {\n-\t\tunsigned long remaining;\n+\t\tsize_t remaining;\n \t\tunsigned char *in = use_pack(p, w_curs, offset, &remaining);\n \t\toffset += remaining;\n \t\tif (!pack_sig_ofs)\ndiff --git a/pack.h b/pack.h\nindex 8294341..ca89ad0 100644\n--- a/pack.h\n+++ b/pack.h\n@@ -78,7 +78,7 @@ struct progress;\n typedef int (*verify_fn)(const struct object_id *, enum object_type, unsigned long, void*, int*);\n \n extern const char *write_idx_file(const char *index_name, struct pack_idx_entry **objects, int nr_objects, const struct pack_idx_option *, const unsigned char *sha1);\n-extern int check_pack_crc(struct packed_git *p, struct pack_window **w_curs, off_t offset, off_t len, unsigned int nr);\n+extern int check_pack_crc(struct packed_git *p, struct pack_window **w_curs, off_t offset, size_t len, unsigned int nr);\n extern int verify_pack_index(struct packed_git *);\n extern int verify_pack(struct packed_git *, verify_fn fn, struct progress *, uint32_t);\n extern off_t write_pack_header(struct sha1file *f, uint32_t);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 97b39b0..3428172 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1228,7 +1228,7 @@ static int in_window(struct pack_window *win, off_t offset)\n unsigned char *use_pack(struct packed_git *p,\n \t\tstruct pack_window **w_cursor,\n \t\toff_t offset,\n-\t\tunsigned long *left)\n+\t\tsize_t *left)\n {\n \tstruct pack_window *win = *w_cursor;\n \n@@ -2122,8 +2122,8 @@ int unpack_object_header(struct packed_git *p,\n \t\t\t size_t *sizep)\n {\n \tunsigned char *base;\n-\tunsigned long left;\n-\tunsigned long used;\n+\tsize_t left;\n+\tsize_t used;\n \tenum object_type type;\n \n \t/* use_pack() assures us we have [base, base + 20) available\ndiff --git a/wrapper.c b/wrapper.c\nindex 36630e5..c4253f7 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -67,11 +67,11 @@ static void *do_xmalloc(size_t size, int gentle)\n \t\t\tret = malloc(1);\n \t\tif (!ret) {\n \t\t\tif (!gentle)\n-\t\t\t\tdie(\"Out of memory, malloc failed (tried to allocate %lu bytes)\",\n-\t\t\t\t (unsigned long)size);\n+\t\t\t\tdie(\"Out of memory, malloc failed (tried to allocate %\" PRIuMAX \" bytes)\",\n+\t\t\t\t (uintmax_t)size);\n \t\t\telse {\n-\t\t\t\terror(\"Out of memory, malloc failed (tried to allocate %lu bytes)\",\n-\t\t\t\t (unsigned long)size);\n+\t\t\t\terror(\"Out of memory, malloc failed (tried to allocate %\" PRIuMAX \" bytes)\",\n+\t\t\t\t (uintmax_t)size);\n \t\t\t\treturn NULL;\n \t\t\t}\n \t\t}\ndiff --git a/zlib.c b/zlib.c\nindex 4223f1a..e98e499 100644\n--- a/zlib.c\n+++ b/zlib.c\n@@ -29,7 +29,7 @@ static const char *zerr_to_string(int status)\n */\n /* #define ZLIB_BUF_MAX ((uInt)-1) */\n #define ZLIB_BUF_MAX ((uInt) 1024 * 1024 * 1024) /* 1GB */\n-static inline uInt zlib_buf_cap(unsigned long len)\n+static inline uInt zlib_buf_cap(size_t len)\n {\n \treturn (ZLIB_BUF_MAX < len) ? ZLIB_BUF_MAX : len;\n }\n@@ -46,8 +46,8 @@ static void zlib_pre_call(git_zstream *s)\n \n static void zlib_post_call(git_zstream *s)\n {\n-\tunsigned long bytes_consumed;\n-\tunsigned long bytes_produced;\n+\tsize_t bytes_consumed;\n+\tsize_t bytes_produced;\n \n \tbytes_consumed = s->z.next_in - s->next_in;\n \tbytes_produced = s->z.next_out - s->next_out;\n@@ -150,7 +150,7 @@ int git_inflate(git_zstream *strm, int flush)\n #define deflateBound(c,s) ((s) + (((s) + 7) >> 3) + (((s) + 63) >> 6) + 11)\n #endif\n \n-unsigned long git_deflate_bound(git_zstream *strm, unsigned long size)\n+size_t git_deflate_bound(git_zstream *strm, size_t size)\n {\n \treturn deflateBound(&strm->z, size);\n }\n-- \n2.1.4\n\n"},{"id":"326050","messageId":"1502388789-5775-2-git-send-email-martin@mail.zuhause","threadId":"46559","inReplyTo":"1502388789-5775-1-git-send-email-martin@mail.zuhause","subject":"[PATCH 4/4] Fix delta offset overflow","fromName":"Martin Koegler","fromEmail":"martin.koegler@chello.at","sentAt":"2017-08-10T18:13:09Z","receivedAt":"2017-08-10T18:13:30Z","isPatch":true,"sender":{"key":"martin.koegler@chello.at","avatar":"https://avatars.githubusercontent.com/u/1859030?v=4"},"body":"From: Martin Koegler \n\nPrevent generating delta offsets beyond 4G.\n\nSigned-off-by: Martin Koegler \n---\n diff-delta.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/diff-delta.c b/diff-delta.c\nindex 3d5e1ef..633883e 100644\n--- a/diff-delta.c\n+++ b/diff-delta.c\n@@ -454,6 +454,9 @@ create_delta(const struct delta_index *index,\n \t\t\tmoff += msize;\n \t\t\tmsize = left;\n \n+\t\t\tif (moff > 0xffffffff)\n+\t\t\t\tmsize = 0;\n+\n \t\t\tif (msize < 4096) {\n \t\t\t\tint j;\n \t\t\t\tval = 0;\n-- \n2.1.4\n\n"},{"id":"326094","messageId":"xmqqbmnn2lnv.fsf@gitster.mtv.corp.google.com","threadId":"46559","inReplyTo":"1502388789-5775-2-git-send-email-martin@mail.zuhause","subject":"Re: [PATCH 4/4] Fix delta offset overflow","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-10T20:49:24Z","receivedAt":"2017-08-10T20:49:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Koegler writes:\n\n> From: Martin Koegler \n>\n> Prevent generating delta offsets beyond 4G.\n>\n> Signed-off-by: Martin Koegler \n> ---\n> diff-delta.c | 3 +++\n> 1 file changed, 3 insertions(+)\n>\n> diff --git a/diff-delta.c b/diff-delta.c\n> index 3d5e1ef..633883e 100644\n> --- a/diff-delta.c\n> +++ b/diff-delta.c\n> @@ -454,6 +454,9 @@ create_delta(const struct delta_index *index,\n> \t\t\tmoff += msize;\n> \t\t\tmsize = left;\n> \n> +\t\t\tif (moff > 0xffffffff)\n> +\t\t\t\tmsize = 0;\n> +\n\nThe lower 4-byte of moff (before incrementing it with msize) were\nalready encoded to the output stream before this hunk. Shouldn't\nwe be checking if moff would fit in uint32_t _before_ that happens?\n\nIOW, in a much earlier part of that \"while (data < top)\" loop, there\nis a code that tries to find a match that gives us a large msize by\niterating over index->hash[], and it sets msize and moff as a potential\nlocation that we may want to use. If moff is already big there, then\nwe shouldn't consider it a match because we cannot encode its location\nusing 4-byte anyway, no?\n\nCutting it off at here by resetting msize to 0 might help the next\niteration (I didn't check, but is the effect of it is to corrupt the\n\"val\" rolling checksum and make it unlikely that the hash\ncomputation would not find a correct match?) but it somehow feels\nlike closing the barn door after the horse has already bolted...\n\n> \t\t\tif (msize < 4096) {\n> \t\t\t\tint j;\n> \t\t\t\tval = 0;\n"},{"id":"326106","messageId":"xmqqy3qr1462.fsf@gitster.mtv.corp.google.com","threadId":"46559","inReplyTo":"1502388789-5775-1-git-send-email-martin@mail.zuhause","subject":"Re: [PATCH 3/4] Convert zlib.c to size_t","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-10T21:52:37Z","receivedAt":"2017-08-10T21:52:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Koegler writes:\n\n> From: Martin Koegler \n>\n> Signed-off-by: Martin Koegler \n> ---\n\nThanks. I haven't thought things through but this looks sensible.\nWill queue.\n"},{"id":"326128","messageId":"20170811065732.GA15128@mail.zuhause","threadId":"46559","inReplyTo":"xmqqbmnn2lnv.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/4] Fix delta offset overflow","fromName":"Martin Koegler","fromEmail":"martin.koegler@chello.at","sentAt":"2017-08-11T06:57:32Z","receivedAt":"2017-08-11T06:58:06Z","isPatch":true,"sender":{"key":"martin.koegler@chello.at","avatar":"https://avatars.githubusercontent.com/u/1859030?v=4"},"body":"On Thu, Aug 10, 2017 at 01:49:24PM -0700, Junio C Hamano wrote:\n> The lower 4-byte of moff (before incrementing it with msize) were\n> already encoded to the output stream before this hunk. Shouldn't\n> we be checking if moff would fit in uint32_t _before_ that happens?\n\nmoff is otherwise only decremented or assigned with an offset generated by\ncreate_delta_index. These offsets are limited by 4GB.\n\nAny larger offets would be a programming bug - so qualify for just a \"assert\".\n \n> IOW, in a much earlier part of that \"while (data < top)\" loop, there\n> is a code that tries to find a match that gives us a large msize by\n> iterating over index->hash[], and it sets msize and moff as a potential\n> location that we may want to use. If moff is already big there, then\n> we shouldn't consider it a match because we cannot encode its location\n> using 4-byte anyway, no?\n\nRecovering from an incorrect moff would add more complex code for a case, that\nshould not happen. A bailout would be sufficient.\n \n> Cutting it off at here by resetting msize to 0 might help the next\n> iteration (I didn't check, but is the effect of it is to corrupt the\n> \"val\" rolling checksum and make it unlikely that the hash\n> computation would not find a correct match?) but it somehow feels\n> like closing the barn door after the horse has already bolted...\n\nThe current code produces incorrect deltas - its not just a checksum issue.\n\nBy the way: \n\nSomebody interested in JGIT should also look at these two bugs:\n\nhttps://github.com/eclipse/jgit/blob/005e5feb4ecd08c4e4d141a38b9e7942accb3212/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/DeltaEncoder.java\ncopy would also encode beyond 4GB - producing truncated delta offset.\n\nhttps://github.com/eclipse/jgit/blob/005e5feb4ecd08c4e4d141a38b9e7942accb3212/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/BinaryDelta.java\napply uses int for decoding length values.\n\nRegards,\nMartin\n"},{"id":"326159","messageId":"xmqqtw1eym56.fsf@gitster.mtv.corp.google.com","threadId":"46559","inReplyTo":"20170811065732.GA15128@mail.zuhause","subject":"Re: [PATCH 4/4] Fix delta offset overflow","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-11T18:50:13Z","receivedAt":"2017-08-11T18:50:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Koegler writes:\n\n> On Thu, Aug 10, 2017 at 01:49:24PM -0700, Junio C Hamano wrote:\n>> The lower 4-byte of moff (before incrementing it with msize) were\n>> already encoded to the output stream before this hunk. Shouldn't\n>> we be checking if moff would fit in uint32_t _before_ that happens?\n>\n> moff is otherwise only decremented or assigned with an offset generated by\n> create_delta_index. These offsets are limited by 4GB.\n>\n> Any larger offets would be a programming bug - so qualify for just a \"assert\".\n\nOK, in that case, I agree that a check before encoding moff into\n(upto) 4 output bytes is unnecessary. Sorry, I didn't read the\nfunction that populates index->hash[] before responding, and I admit\nthat I haven't read it for a while.\n\n>> Cutting it off at here by resetting msize to 0 might help the next\n>> iteration (I didn't check, but is the effect of it is to corrupt the\n>> \"val\" rolling checksum and make it unlikely that the hash\n>> computation would not find a correct match?) but it somehow feels\n>> like closing the barn door after the horse has already bolted...\n>\n> The current code produces incorrect deltas - its not just a checksum issue.\n\nAgain, I mis-read what role msize was playing in the original (or in\nyour update). I'd need to re-read that part of the code to make\nsure I get how your change will fix the issue.\n\nThanks.\n\n> By the way: \n>\n> Somebody interested in JGIT should also look at these two bugs:\n>\n> https://github.com/eclipse/jgit/blob/005e5feb4ecd08c4e4d141a38b9e7942accb3212/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/DeltaEncoder.java\n> copy would also encode beyond 4GB - producing truncated delta offset.\n>\n> https://github.com/eclipse/jgit/blob/005e5feb4ecd08c4e4d141a38b9e7942accb3212/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/BinaryDelta.java\n> apply uses int for decoding length values.\n\nI'll cc: an obvious suspect; thanks for the note.\n"}]}