{"thread":{"id":"12261","subject":"[PATCH] hash: fix lookup_hash semantics","startedAt":"2008-02-22T19:47:27Z","lastAt":"2008-02-22T22:13:42Z","messageCount":4,"participants":["Jeff King","Linus Torvalds","Junio C Hamano","Pieter de Bie"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"69610","messageId":"20080222194726.GA24532@sigill.intra.peff.net","threadId":"12261","inReplyTo":null,"subject":"[PATCH] hash: fix lookup_hash semantics","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-02-22T19:47:27Z","receivedAt":"2008-02-22T19:47:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We were returning the _address of_ the stored item (or NULL)\ninstead of the item itself. While this sort of indirection\nis useful for insertion (since you can lookup and then\nmodify), it is unnecessary for read-only lookup. Since the\nhash code splits these functions between the internal\nlookup_hash_entry function and the public lookup_hash\nfunction, it makes sense for the latter to provide what\nusers of the library expect.\n\nThe result of this was that the index caching returned bogus\nresults on lookup. We unfortunately didn't catch this\nbecause we were returning a \"struct cache_entry **\" as a\n\"void *\", and accidentally assigning it to a \"struct\ncache_entry *\".\n\nAs it happens, this actually _worked_ most of the time,\nbecause the entries were defined as:\n\n struct cache_entry {\n\t struct cache_entry *next;\n\t ...\n };\n\nmeaning that interpreting a \"struct cache_entry **\" as a\n\"struct cache_entry *\" would yield an entry where all fields\nwere totally bogus _except_ for the next pointer, which\npointed to the actual cache entry. When walking the list, we\nwould look at the bogus \"name\" field, which was unlikely to\nmatch our lookup, and then proceed to the \"real\" entry.\n\nThe reading of bogus data was silently ignored most of the\ntime, but could cause a segfault for some data (which seems\nto be more common on OS X).\n\nSigned-off-by: Jeff King \n---\nThis can be applied to maint, but there aren't actually any\ncallers of lookup_hash until Linus' recent patch series, so it really\nonly makes sense on top of that. Alternatively, his patches could be\naltered to dereference the return from lookup_hash, but I think this\ncalling convention makes more sense (for the reasons I explained above).\n\nThis shuts up the valgrind errors I see under Linux; it would be nice to\nget confirmation from OS X people that this fixes their \"git status\"\nsegfaults.\n\n hash.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/hash.c b/hash.c\nindex 7b492d4..d9ec82f 100644\n--- a/hash.c\n+++ b/hash.c\n@@ -70,7 +70,7 @@ void *lookup_hash(unsigned int hash, struct hash_table *table)\n {\n \tif (!table->array)\n \t\treturn NULL;\n-\treturn &lookup_hash_entry(hash, table)->ptr;\n+\treturn lookup_hash_entry(hash, table)->ptr;\n }\n \n void **insert_hash(unsigned int hash, void *ptr, struct hash_table *table)\n-- \n1.5.4.2.262.g044a1.dirty\n"},{"id":"69614","messageId":"alpine.LFD.1.00.0802221233410.21332@woody.linux-foundation.org","threadId":"12261","inReplyTo":"20080222194726.GA24532@sigill.intra.peff.net","subject":"Re: [PATCH] hash: fix lookup_hash semantics","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-02-22T20:42:09Z","receivedAt":"2008-02-22T20:42:09Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Fri, 22 Feb 2008, Jeff King wrote:\n>\n> We were returning the _address of_ the stored item (or NULL)\n> instead of the item itself. While this sort of indirection\n> is useful for insertion (since you can lookup and then\n> modify), it is unnecessary for read-only lookup.\n\nGaah. That code returned the pointer-to-a-pointer exactly because of the \ninsert case, but then to clean it up I had split it up into that internal \n\"lookup_hash_entry()\" case, but left the broken semantics for \nlookup_hash().\n\nStupid.\n\nYour patch is obviously correct, and should probably go into maint just so \nthat no other code makes the mistake of using that broken interface.\n\n\t\tLinus\n"},{"id":"69616","messageId":"7v7igw90fz.fsf@gitster.siamese.dyndns.org","threadId":"12261","inReplyTo":"20080222194726.GA24532@sigill.intra.peff.net","subject":"Re: [PATCH] hash: fix lookup_hash semantics","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-22T20:54:24Z","receivedAt":"2008-02-22T20:54:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> We were returning the _address of_ the stored item (or NULL)\n> instead of the item itself. While this sort of indirection\n> is useful for insertion (since you can lookup and then\n> modify), it is unnecessary for read-only lookup.\n\nVery nicely spotted. Thanks.\n"},{"id":"69620","messageId":"2A7FA5AD-555E-480F-924F-68BB340E1753@ai.rug.nl","threadId":"12261","inReplyTo":"20080222194726.GA24532@sigill.intra.peff.net","subject":"Re: [PATCH] hash: fix lookup_hash semantics","fromName":"Pieter de Bie","fromEmail":"pdebie@ai.rug.nl","sentAt":"2008-02-22T22:13:42Z","receivedAt":"2008-02-22T22:13:42Z","isPatch":true,"sender":{"key":"pdebie@ai.rug.nl","avatar":null},"body":"\nOn Feb 22, 2008, at 8:47 PM, Jeff King wrote:\n\n> This shuts up the valgrind errors I see under Linux; it would be \n> nice to\n> get confirmation from OS X people that this fixes their \"git status\"\n> segfaults.\n\nI think my original message may not have come through as it was in \nHTML accidentally.\n\nAnyway, I tested the patch and it works. Thanks!\n\nTested-by: Pieter de Bie \n\n- Pieter\n"}]}