> On April 28, 2015, 2:32 p.m., Rafael Schloming wrote: > > proton-c/src/object/map.c, line 266 > > <https://reviews.apache.org/r/33623/diff/2/?file=944061#file944061line266> > > > > This isn't related to this line, I just don't know where to put general > > comments... > > > > It occurs to me that with this sort of thing it is extremely valuable > > to capture an actual test case that reproduces the corruption. There are a > > number of existing tests for maps in proton-c/src/tests/object.c, but > > obviously they don't hit this delete scenario. It would be good to get some > > coverage of this case. If it would be helpful I can throw together a test > > class that lets you create objects consisting of an explicitly specified > > hashcode and value. That way you should be able to deterministically > > recreate the collisions resulting in this bug. > > Gordon Sim wrote: > I agree. I'll have a look at the existing tests and see if I can create > one demonstrating the problem with the current code.
"If it would be helpful I can throw together a test class that lets you create objects consisting of an explicitly specified hashcode and value. That way you should be able to deterministically recreate the collisions resulting in this bug." This would indeed be helpful. I tried using pn_hash_t, but can't seem tocorrectly free that. - Gordon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33623/#review81816 ----------------------------------------------------------- On April 28, 2015, 6:35 p.m., Gordon Sim wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33623/ > ----------------------------------------------------------- > > (Updated April 28, 2015, 6:35 p.m.) > > > Review request for qpid, Alan Conway, Andrew Stitcher, and Rafael Schloming. > > > Bugs: PROTON-858 > https://issues.apache.org/jira/browse/PROTON-858 > > > Repository: qpid-proton-git > > > Description > ------- > > The internal map implementation for proton-c uses coalesced hashing, but the > current deletion algorithm does not take account of this. On deletion of > entries, the map can lose its ability to locate other entries by key. > > This patch is the simplest possible fix. It frees the deleted entry, makes > any previous entry that points to this tha tail of its chain, and then > relocates any entries in a chain following the deleted entry. Though I > believe this to be correct and safe, it does involve more work on some > deletions. The approach can be optimised further, either be reducing the > amount of chain that needs relocated, or by marking records as deleted in the > first instance and only rehashing later on some criteria. > > (I started off trying to do the latter, but it is more complicated and I > still haven't got my patch fully correct. When I get that working I'll post > that also. It is a more involved change though, so perhaps we may want to > consider this simpler approach for 0.9.1). > > > Diffs > ----- > > proton-c/src/object/map.c 1a758a1 > proton-c/src/tests/object.c 2b213c5 > > Diff: https://reviews.apache.org/r/33623/diff/ > > > Testing > ------- > > > Thanks, > > Gordon Sim > >
