GNOME Bugzilla – Bug 792196
PotentialMatch: fix array indexing if first string is longer than second
Last modified: 2018-01-04 13:10:25 UTC
Created attachment 366274 [details] [review] [Patch] Check for `pos` before doing trying to use it as index. In the `_matches` method of PotentialMatch, we iterate over the 1st string and try to find a match for the character on the same position in the 2nd string. If it's not the same, we also perform the same check within a margin of `max_dist`, *with* some boundary checks. On the original position however, it only does this check *after* already using it as index (potential-match.vala:668). If the 1st string is longer than the 2nd string, it will hence try to an out-of-bounds access on the 2nd string. The attached patch fixes this by returning -1 in `_contains()` if the index is too large. On a side note: I was curious why the `match-name.vala` is situated in the tests for the Tracker backend (which is disabled by default and which I can't even get building locally without some tinkering) rather than a general test.
Review of attachment 366274 [details] [review]: I don’t think this is quite right, since it discounts a match within the bounds of pos ± max_dist. For example, if pos = haystack.length, max_dist = 5, and haystack[haystack.length-1] = c, then -1 will be returned incorrectly.
(In reply to Niels De Graef from comment #0) > On a side note: I was curious why the `match-name.vala` is situated in the > tests for the Tracker backend (which is disabled by default and which I > can't even get building locally without some tinkering) rather than a > general test. Probably because it was written at the same time as the Tracker backend, I think. I agree, it’s a bit of a weird place for the test. Moving it to tests/dummy/match-name.vala would be better, but it would need a bit of rework to use dummy personas rather than Tracker personas to provide the test data. That should be fairly straightforward (see how search-view.vala does it). I’d be really happy to review a patch for it if you want to open a separate bug about it. :-)
Created attachment 366277 [details] [review] [Patch] Check for `pos` before doing trying to use it as index. (In reply to Philip Withnall from comment #2) > (In reply to Niels De Graef from comment #0) > > On a side note: I was curious why the `match-name.vala` is situated in the > > tests for the Tracker backend (which is disabled by default and which I > > can't even get building locally without some tinkering) rather than a > > general test. > > Probably because it was written at the same time as the Tracker backend, I > think. > > I agree, it’s a bit of a weird place for the test. Moving it to > tests/dummy/match-name.vala would be better, but it would need a bit of > rework to use dummy personas rather than Tracker personas to provide the > test data. That should be fairly straightforward (see how search-view.vala > does it). I’d be really happy to review a patch for it if you want to open a > separate bug about it. :-) Indeed, you're right. Great find! The attached patch should fix this.
Review of attachment 366277 [details] [review]: Looks good. If you do end up moving the match-name.vala tests, it would be good to add some regression tests to it for this bug (and the other PotentialMatch) bug.
Comment on attachment 366277 [details] [review] [Patch] Check for `pos` before doing trying to use it as index. Landed on master as commit 7a71777.
Also made bug 792201 for the test migration. Thanks for the quick feedback again!