{"thread":{"id":"11334","subject":"[PATCH] Fix segfault in diff-delta.c when FLEX_ARRAY is 1","startedAt":"2007-12-18T01:39:57Z","lastAt":"2007-12-18T14:46:37Z","messageCount":7,"participants":["Pierre Habouzit","Linus Torvalds","Junio C Hamano","David Kastrup","Nicolas Pitre"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"63543","messageId":"1197941997-11421-1-git-send-email-madcoder@debian.org","threadId":"11334","inReplyTo":null,"subject":"[PATCH] Fix segfault in diff-delta.c when FLEX_ARRAY is 1","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-12-18T01:39:57Z","receivedAt":"2007-12-18T01:39:57Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"aka don't do pointer arithmetics on structs that have a FLEX_ARRAY member,\nor you'll end up believing your array is 1 cell off its real address.\n\nSigned-off-by: Pierre Habouzit <madcoder@debian.org>\n---\n diff-delta.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/diff-delta.c b/diff-delta.c\nindex 9e440a9..601b49e 100644\n--- a/diff-delta.c\n+++ b/diff-delta.c\n@@ -264,7 +264,7 @@ struct delta_index * create_delta_index(const void *buf, unsigned long bufsize)\n \tindex->src_size = bufsize;\n \tindex->hash_mask = hmask;\n \n-\tmem = index + 1;\n+\tmem = index->hash;\n \tpacked_hash = mem;\n \tmem = packed_hash + (hsize+1);\n \tpacked_entry = mem;\n-- \n1.5.4.rc0.1153.gb1b3d-dirty\n"},{"id":"63546","messageId":"20071218014455.GB14981@artemis.madism.org","threadId":"11334","inReplyTo":"1197941997-11421-1-git-send-email-madcoder@debian.org","subject":"Re: [PATCH] Fix segfault in diff-delta.c when FLEX_ARRAY is 1","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-12-18T01:44:55Z","receivedAt":"2007-12-18T01:44:55Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Tue, Dec 18, 2007 at 01:39:57AM +0000, Pierre Habouzit wrote:\n> aka don't do pointer arithmetics on structs that have a FLEX_ARRAY member,\n> or you'll end up believing your array is 1 cell off its real address.\n\n  I wonder if we could teach sparse to prevent us from using pointer\narithmetics on some types… because I obviously didn't read all the git\ncode, and I wouldn't be surprised an instance of this still remains\nsomehwere.\n\n-- \n·O·  Pierre Habouzit\n··O                                                madcoder@debian.org\nOOO                                                http://www.madism.org\n"},{"id":"63557","messageId":"alpine.LFD.0.9999.0712172032090.21557@woody.linux-foundation.org","threadId":"11334","inReplyTo":"20071218014455.GB14981@artemis.madism.org","subject":"Re: [PATCH] Fix segfault in diff-delta.c when FLEX_ARRAY is 1","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-12-18T04:46:55Z","receivedAt":"2007-12-18T04:46:55Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 18 Dec 2007, Pierre Habouzit wrote:\n> \n>   I wonder if we could teach sparse to prevent us from using pointer\n> arithmetics on some types… because I obviously didn't read all the git\n> code, and I wouldn't be surprised an instance of this still remains\n> somehwere.\n\nThis should do it.\n\nWhat this does is:\n - make flex structures not have a size at all (so \"sizeof()\" will fail)\n - add warnings for trying to add or subtract unsized pointers\n\nso now you can try it on git with\n\n\tmake CC=cgcc\n\nand while it finds a fair number of \"sizeof(..)\" things and complains \nabout them, the only invalid pointer arithmetic it finds is the \n\n\tmem = index + 1;\n\nline in diff-delta.c.\n\nWhether it is worth fixing all the \"sizeof()\" calls too, I dunno. They \nresult in a slight waste of memory (ie we allocate too much memory), but \nI guess they should be harmless.\n\nHowever, one indication that there may still be something wrong is that if \nyou re-make git with FLEX_ARRAY set to some big insane value (say, 1234), \nthen git will still fail the test-suite. So maybe there's a \"sizeof()\" \nthat isn't just used for allocation sizes. I didn't check them all, \nthere's something like 44 complaints like\n\n\tbuiltin-fetch.c:306:21: error: cannot size expression\n\nfrom sparse with this patch.\n\n\t\tLinus\n\n---\n evaluate.c |    8 ++++++++\n symbol.c   |    2 ++\n 2 files changed, 10 insertions(+), 0 deletions(-)\n\ndiff --git a/evaluate.c b/evaluate.c\nindex 54fcd3f..cd816a8 100644\n--- a/evaluate.c\n+++ b/evaluate.c\n@@ -576,6 +576,10 @@ static struct symbol *evaluate_ptr_add(struct expression *expr, struct symbol *i\n \t\texpression_error(expr, \"arithmetics on pointers to functions\");\n \t\treturn NULL;\n \t}\n+\tif (base->bit_size & 7) {\n+\t\texpression_error(expr, \"arithmetic on unsized pointers\");\n+\t\treturn NULL;\n+\t}\n \n \t/* Get the size of whatever the pointer points to */\n \tmultiply = base->bit_size >> 3;\n@@ -820,6 +824,10 @@ static struct symbol *evaluate_ptr_sub(struct expression *expr)\n \t\texpression_error(expr, \"subtraction of functions? Share your drugs\");\n \t\treturn NULL;\n \t}\n+\tif (lbase->bit_size & 7) {\n+\t\texpression_error(expr, \"subtracting unsized pointers\");\n+\t\treturn NULL;\n+\t}\n \n \texpr->ctype = ssize_t_ctype;\n \tif (lbase->bit_size > bits_in_char) {\ndiff --git a/symbol.c b/symbol.c\nindex 7539817..8b390ac 100644\n--- a/symbol.c\n+++ b/symbol.c\n@@ -124,8 +124,10 @@ static void lay_out_struct(struct symbol *sym, struct struct_union_info *info)\n \t * structure size\n \t */\n \tif (base_size < 0) {\n+\t\tinfo->bit_size = -1;\n \t\tinfo->align_size = 0;\n \t\tbase_size = 0;\n+\t\treturn;\n \t}\n \n \talign_bit_mask = (sym->ctype.alignment << 3) - 1;\n"},{"id":"63562","messageId":"alpine.LFD.0.9999.0712172146070.21557@woody.linux-foundation.org","threadId":"11334","inReplyTo":"alpine.LFD.0.9999.0712172032090.21557@woody.linux-foundation.org","subject":"Re: [PATCH] Fix segfault in diff-delta.c when FLEX_ARRAY is 1","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-12-18T06:12:03Z","receivedAt":"2007-12-18T06:12:03Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 17 Dec 2007, Linus Torvalds wrote:\n> \n> However, one indication that there may still be something wrong is that if \n> you re-make git with FLEX_ARRAY set to some big insane value (say, 1234), \n> then git will still fail the test-suite. So maybe there's a \"sizeof()\" \n> that isn't just used for allocation sizes.\n\nNo, that's a different thing. In at least unpack-trees.c (line 593), we do\n\n\t..\n\tif (same(old, merge)) {\n\t\t*merge = *old;\n\t} else {\n\t..\n\nand that \"merge\" is a cache_entry pointer. If we have a non-zero \nFLEX_ARRAY size, it will cause us to copy the first few bytes of the name \ntoo.\n\nThat is technically wrong even for FLEX_ARRAY being 1, but you'll never \nnotice, since the names are always at least one byte, and the filenames \nshould basically always be the same. But if we do the same thing for a \nrename, we'd be screwed.\n\nSo we probably should look out for those things too. A quick hack to \nsparse shows that that was the only such occurrence in the current tree, \nthough.\n\nAnyway, with this patch to current git, it passes all the test-suite with \nFLEX_ARRAY artificially set to 123, and the only complaints from my \nhacked-up sparse are about \"sizeof()\" calls (ie no pointer arithmetic, and \nno assignments to flex-array-structures).\n\n*Most* of the remaining sizeof() uses, in turn, are allocation-size \nrelated, ie passed in to calloc() and friends, and while I didn't check \nthem all, such uses of sizeof() is fine (even if it causes some \nunnecessarily big allocations).\n\nBut there's a few that aren't obviously allocations (this is a list done \nwith grep and sparse, I didn't look at whether the values used are then \nall allocation-related):\n\n - builtin-blame.c:128     memset(o, 0, sizeof(*o));\n - diff-delta.c:250        memsize = sizeof(*index)\n - object-refs.c:23        size_t size = sizeof(*refs) + count*sizeof(struct object *);\n - object-refs.c:61        size_t size = sizeof(*refs) + j*sizeof(struct object *);\n - attr.c:220              sizeof(*res) +\n - remote.c:467            memset(ret, 0, sizeof(struct ref) + namelen);\n - remote.c:474            memcpy(ret, ref, sizeof(struct ref) + strlen(ref->name) + 1);\n - transport.c:491         memset(ref, 0, sizeof(struct ref));\n\nmaybe somebody else can check them out (I've not sent out the hacky patch \nto 'sparse' that found that assignment in unpack-trees.c, but since that \none was the only one it found, nobody should care - and it really was \npretty hacky).\n\n\t\tLinus\n\n---\n diff-delta.c   |    2 +-\n unpack-trees.c |    2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/diff-delta.c b/diff-delta.c\nindex 9e440a9..601b49e 100644\n--- a/diff-delta.c\n+++ b/diff-delta.c\n@@ -264,7 +264,7 @@ struct delta_index * create_delta_index(const void *buf, unsigned long bufsize)\n \tindex->src_size = bufsize;\n \tindex->hash_mask = hmask;\n \n-\tmem = index + 1;\n+\tmem = index->hash;\n \tpacked_hash = mem;\n \tmem = packed_hash + (hsize+1);\n \tpacked_entry = mem;\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex e9eb795..aa2513e 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -590,7 +590,7 @@ static int merged_entry(struct cache_entry *merge, struct cache_entry *old,\n \t\t * a match.\n \t\t */\n \t\tif (same(old, merge)) {\n-\t\t\t*merge = *old;\n+\t\t\tmemcpy(merge, old, offsetof(struct cache_entry, name));\n \t\t} else {\n \t\t\tverify_uptodate(old, o);\n \t\t\tinvalidate_ce_path(old);\n"},{"id":"63571","messageId":"7vwsrc1idm.fsf@gitster.siamese.dyndns.org","threadId":"11334","inReplyTo":"alpine.LFD.0.9999.0712172146070.21557@woody.linux-foundation.org","subject":"Re: [PATCH] Fix segfault in diff-delta.c when FLEX_ARRAY is 1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-18T09:07:01Z","receivedAt":"2007-12-18T09:07:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> But there's a few that aren't obviously allocations (this is a list done \n> with grep and sparse, I didn't look at whether the values used are then \n> all allocation-related):\n>\n>  - builtin-blame.c:128     memset(o, 0, sizeof(*o));\n\nThis is harmless and in fact unnecessary clearing, immediately before\ncalling free(3).\n\n>  - diff-delta.c:250        memsize = sizeof(*index)\n\nI haven't studied this codepath.\n\n>  - object-refs.c:23        size_t size = sizeof(*refs) + count*sizeof(struct object *);\n\nOverallocation to have at least \"count\" pointers to \"struct object\".\n\n>  - object-refs.c:61        size_t size = sizeof(*refs) + j*sizeof(struct object *);\n\nDitto for \"j\" pointers.\n\n>  - attr.c:220              sizeof(*res) +\n\nOverallocation to have at least \"num_attr\" instances of \"struct\nattr_state\" (plus name string if needed, which is stored using location\npast state[num_attr]).\n\n>  - remote.c:467            memset(ret, 0, sizeof(struct ref) + namelen);\n\nClearing an arena that was overallocated to have at least namelen\nelements of char[] on the line immediately before this, with matching\nsize.  All callers pass namelen = strlen(name) + 1 so we are Ok even\nwhen FLEX_ARRAY gives no extra space.\n\n>  - remote.c:474            memcpy(ret, ref, sizeof(struct ref) + strlen(ref->name) + 1);\n\nDitto.\n\n>  - transport.c:491         memset(ref, 0, sizeof(struct ref));\n\nA line above overallocates to have enough room for strlen(ref_name) plus\nterminating NUL, and after this lines clears the non-flex part, name is\ncopied.  So this overclears a bit, but is harmless.\n\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index e9eb795..aa2513e 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -590,7 +590,7 @@ static int merged_entry(struct cache_entry *merge, struct cache_entry *old,\n>  \t\t * a match.\n>  \t\t */\n>  \t\tif (same(old, merge)) {\n> -\t\t\t*merge = *old;\n> +\t\t\tmemcpy(merge, old, offsetof(struct cache_entry, name));\n>  \t\t} else {\n>  \t\t\tverify_uptodate(old, o);\n>  \t\t\tinvalidate_ce_path(old);\n\nPortability of offsetof() is slightly worrisome, but giving a\ncompatibility macro is trivial if this turns out to be problematic for\nsome people.\n"},{"id":"63591","messageId":"85ir2wjltz.fsf@lola.goethe.zz","threadId":"11334","inReplyTo":"7vwsrc1idm.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Fix segfault in diff-delta.c when FLEX_ARRAY is 1","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2007-12-18T11:15:04Z","receivedAt":"2007-12-18T11:15:04Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Linus Torvalds <torvalds@linux-foundation.org> writes:\n>\n>>  - diff-delta.c:250        memsize = sizeof(*index)\n>\n> I haven't studied this codepath.\n\nMy proposed patch should have addressed this as well.\n\n-- \nDavid Kastrup, Kriemhildstr. 15, 44793 Bochum\n"},{"id":"63615","messageId":"alpine.LFD.0.999999.0712180941250.8467@xanadu.home","threadId":"11334","inReplyTo":"7vwsrc1idm.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Fix segfault in diff-delta.c when FLEX_ARRAY is 1","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-12-18T14:46:37Z","receivedAt":"2007-12-18T14:46:37Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 18 Dec 2007, Junio C Hamano wrote:\n\n> Linus Torvalds <torvalds@linux-foundation.org> writes:\n> \n> > But there's a few that aren't obviously allocations (this is a list done \n> > with grep and sparse, I didn't look at whether the values used are then \n> > all allocation-related):\n> >\n> >  - diff-delta.c:250        memsize = sizeof(*index)\n> \n> I haven't studied this codepath.\n\nHarmless overallocation.  Nothing to worry about.\n\n\nNicolas\n"}]}