{"thread":{"id":"16720","subject":"[PATCH] git-fast-import possible memory corruption problem","startedAt":"2008-12-14T02:08:22Z","lastAt":"2008-12-14T10:45:34Z","messageCount":4,"participants":["YONETANI Tomokazu","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"97837","messageId":"20081214020822.GB4121@les.ath.cx","threadId":"16720","inReplyTo":null,"subject":"[PATCH] git-fast-import possible memory corruption problem","fromName":"YONETANI Tomokazu","fromEmail":"qhwt+git@les.ath.cx","sentAt":"2008-12-14T02:08:22Z","receivedAt":"2008-12-14T02:08:22Z","isPatch":true,"sender":{"key":"y0n3t4n1@gmail.com","avatar":null},"body":"Hello.\nWhile trying to convert NetBSD CVS repository to Git, I've been\nexperiencing 100% reproducible crash of git-fast-import.  After\npoking here and there and I noticed a dubious code fragment in\npool_alloc():\n\t:\n\n        r = p->next_free;\n        /* round out to a 'uintmax_t' alignment */\n        if (len & (sizeof(uintmax_t) - 1))\n                len += sizeof(uintmax_t) - (len & (sizeof(uintmax_t) - 1));\n        p->next_free += len;\n        return r;\n\nAs the `round out' takes place AFTER it found the room in the mem_pool,\nthere's a small chance of p->next_free being set outside of the chosen\narea, up to (sizeof(uintmax_t) - 1) bytes.  pool_strdup() is one of the\nfunctions which can trigger the problem, when pool_alloc() finds a room\nat the end of a pool entry and the requested length is not multiple of\nsize(uintmax_t).  I believe attached patch addresses this problem.\n\nBest regards,\nYONETANI Tomokazu.\n\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 3c035a5..fb4367a 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -549,11 +549,15 @@ static unsigned int hc_str(const char *s, size_t len)\n \treturn r;\n }\n \n-static void *pool_alloc(size_t len)\n+static void *_pool_alloc(size_t len, int zero)\n {\n \tstruct mem_pool *p;\n \tvoid *r;\n \n+\t/* round out to a 'uintmax_t' alignment */\n+\tif (len & (sizeof(uintmax_t) - 1))\n+\t\tlen += sizeof(uintmax_t) - (len & (sizeof(uintmax_t) - 1));\n+\n \tfor (p = mem_pool; p; p = p->next_pool)\n \t\tif ((p->end - p->next_free >= len))\n \t\t\tbreak;\n@@ -561,7 +565,8 @@ static void *pool_alloc(size_t len)\n \tif (!p) {\n \t\tif (len >= (mem_pool_alloc/2)) {\n \t\t\ttotal_allocd += len;\n-\t\t\treturn xmalloc(len);\n+\t\t\tr = xmalloc(len);\n+\t\t\tgoto out;\n \t\t}\n \t\ttotal_allocd += sizeof(struct mem_pool) + mem_pool_alloc;\n \t\tp = xmalloc(sizeof(struct mem_pool) + mem_pool_alloc);\n@@ -572,19 +577,22 @@ static void *pool_alloc(size_t len)\n \t}\n \n \tr = p->next_free;\n-\t/* round out to a 'uintmax_t' alignment */\n-\tif (len & (sizeof(uintmax_t) - 1))\n-\t\tlen += sizeof(uintmax_t) - (len & (sizeof(uintmax_t) - 1));\n \tp->next_free += len;\n+out:\n+\tif (zero)\n+\t\tmemset(r, 0, len);\n \treturn r;\n }\n \n+static void *pool_alloc(size_t len)\n+{\n+\treturn _pool_alloc(len, 0);\n+}\n+\n static void *pool_calloc(size_t count, size_t size)\n {\n \tsize_t len = count * size;\n-\tvoid *r = pool_alloc(len);\n-\tmemset(r, 0, len);\n-\treturn r;\n+\treturn _pool_alloc(len, 1);\n }\n \n static char *pool_strdup(const char *s)\n"},{"id":"97841","messageId":"7vd4fv4e3u.fsf@gitster.siamese.dyndns.org","threadId":"16720","inReplyTo":"20081214020822.GB4121@les.ath.cx","subject":"Re: [PATCH] git-fast-import possible memory corruption problem","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-14T03:42:13Z","receivedAt":"2008-12-14T03:42:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"YONETANI Tomokazu <qhwt+git@les.ath.cx> writes:\n\n> While trying to convert NetBSD CVS repository to Git, I've been\n> experiencing 100% reproducible crash of git-fast-import.  After\n> poking here and there and I noticed a dubious code fragment in\n> pool_alloc():\n> \t:\n>\n>         r = p->next_free;\n>         /* round out to a 'uintmax_t' alignment */\n>         if (len & (sizeof(uintmax_t) - 1))\n>                 len += sizeof(uintmax_t) - (len & (sizeof(uintmax_t) - 1));\n>         p->next_free += len;\n>         return r;\n>\n> As the `round out' takes place AFTER it found the room in the mem_pool,\n> there's a small chance of p->next_free being set outside of the chosen\n> area, up to (sizeof(uintmax_t) - 1) bytes.  pool_strdup() is one of the\n> functions which can trigger the problem, when pool_alloc() finds a room\n> at the end of a pool entry and the requested length is not multiple of\n> size(uintmax_t).  I believe attached patch addresses this problem.\n\nThanks -- do you mean your reproducible crash does not reproduce with the\npatch anymore?\n\nI think your change to move the \"round up\" logic up in the codepath makes\nperfect sense.  But your patch seems to conflate totally unrelated change\nto move memzero from the caller to callee into it, and I do not see the\nreason why it should be that way.  If the caller asked 10 bytes to calloc\nfrom the pool, and the underlying pool allocator gives you a 16-byte\nblock, you only have to guarantee that the first 10 bytes are cleared, and\ncan leave the trailing padding 6 bytes at the end untouched.\n"},{"id":"97844","messageId":"7vskor2tfw.fsf@gitster.siamese.dyndns.org","threadId":"16720","inReplyTo":"7vd4fv4e3u.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] git-fast-import possible memory corruption problem","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-14T05:53:55Z","receivedAt":"2008-12-14T05:53:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> As the `round out' takes place AFTER it found the room in the mem_pool,\n>> there's a small chance of p->next_free being set outside of the chosen\n>> area, up to (sizeof(uintmax_t) - 1) bytes.  pool_strdup() is one of the\n>> functions which can trigger the problem, when pool_alloc() finds a room\n>> at the end of a pool entry and the requested length is not multiple of\n>> size(uintmax_t).  I believe attached patch addresses this problem.\n>\n> Thanks -- do you mean your reproducible crash does not reproduce with the\n> patch anymore?\n>\n> I think your change to move the \"round up\" logic up in the codepath makes\n> perfect sense.  But your patch seems to conflate totally unrelated change\n> to move memzero from the caller to callee into it, and I do not see the\n> reason why it should be that way.  If the caller asked 10 bytes to calloc\n> from the pool, and the underlying pool allocator gives you a 16-byte\n> block, you only have to guarantee that the first 10 bytes are cleared, and\n> can leave the trailing padding 6 bytes at the end untouched.\n\nThat is, something like this...\n\n fast-import.c |    7 ++++---\n 1 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git c/fast-import.c w/fast-import.c\nindex 3c035a5..3276d5d 100644\n--- c/fast-import.c\n+++ w/fast-import.c\n@@ -554,6 +554,10 @@ static void *pool_alloc(size_t len)\n \tstruct mem_pool *p;\n \tvoid *r;\n \n+\t/* round up to a 'uintmax_t' alignment */\n+\tif (len & (sizeof(uintmax_t) - 1))\n+\t\tlen += sizeof(uintmax_t) - (len & (sizeof(uintmax_t) - 1));\n+\n \tfor (p = mem_pool; p; p = p->next_pool)\n \t\tif ((p->end - p->next_free >= len))\n \t\t\tbreak;\n@@ -572,9 +576,6 @@ static void *pool_alloc(size_t len)\n \t}\n \n \tr = p->next_free;\n-\t/* round out to a 'uintmax_t' alignment */\n-\tif (len & (sizeof(uintmax_t) - 1))\n-\t\tlen += sizeof(uintmax_t) - (len & (sizeof(uintmax_t) - 1));\n \tp->next_free += len;\n \treturn r;\n }\n"},{"id":"97855","messageId":"20081214104534.GA72082@les.ath.cx","threadId":"16720","inReplyTo":"7vskor2tfw.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] git-fast-import possible memory corruption problem","fromName":"YONETANI Tomokazu","fromEmail":"qhwt+git@les.ath.cx","sentAt":"2008-12-14T10:45:34Z","receivedAt":"2008-12-14T10:45:34Z","isPatch":true,"sender":{"key":"y0n3t4n1@gmail.com","avatar":null},"body":"On Sat, Dec 13, 2008 at 09:53:55PM -0800, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> >> As the `round out' takes place AFTER it found the room in the mem_pool,\n> >> there's a small chance of p->next_free being set outside of the chosen\n> >> area, up to (sizeof(uintmax_t) - 1) bytes.  pool_strdup() is one of the\n> >> functions which can trigger the problem, when pool_alloc() finds a room\n> >> at the end of a pool entry and the requested length is not multiple of\n> >> size(uintmax_t).  I believe attached patch addresses this problem.\n> >\n> > Thanks -- do you mean your reproducible crash does not reproduce with the\n> > patch anymore?\n> >\n> > I think your change to move the \"round up\" logic up in the codepath makes\n> > perfect sense.  But your patch seems to conflate totally unrelated change\n> > to move memzero from the caller to callee into it, and I do not see the\n> > reason why it should be that way.  If the caller asked 10 bytes to calloc\n> > from the pool, and the underlying pool allocator gives you a 16-byte\n> > block, you only have to guarantee that the first 10 bytes are cleared, and\n> > can leave the trailing padding 6 bytes at the end untouched.\n> \n> That is, something like this...\n> \n>  fast-import.c |    7 ++++---\n>  1 files changed, 4 insertions(+), 3 deletions(-)\n> \n> diff --git c/fast-import.c w/fast-import.c\n> index 3c035a5..3276d5d 100644\n[snip]\nYes, that was what it originally looked like, but after fixing the crash\nI encountered another problem, and thought that I had to clear rounded up\narea too.  It turned out that the second problem has nothing to do with\n[the pool allocator, but the data fed to git-fast-import.  After fixing\nthe data, the git-fast-import with above patch finished successfully,\nno more crashes.\n"}]}