GNOME Bugzilla – Bug 792238
[Patch] PotentialMatch: don't rely on null-terminated string
Last modified: 2018-01-06 15:42:10 UTC
Created attachment 366372 [details] [review] [Patch] PotentialMatch: fix end-of-string check PotentialMatch: fix end-of-string check. Since we're not dealing with a `\0`-terminated string anymore, we can't use that to check the end of the string. Just compare with the length instead. Attached patch should fix this.
Review of attachment 366372 [details] [review]: Why are we not dealing with a nul-terminated string any more? From a quick look through the code I can’t see how we could lose the nul terminator. ::: folks/potential-match.vala @@ +673,1 @@ + while (idx < pos + max_dist && idx < haystack_len) If you’re going to make a change like this, I’d prefer to keep the old check (`(ch = haystack[idx]) != 0`) in addition to adding the new one (`idx < haystack_len`). Then we can still handle nul-terminated strings. Additionally, any function which takes input unichar arrays which might not be nul-terminated should be clearly documented as such.
Also, are you using git-bz to submit patches to Bugzilla and push them to git? If not, you should: it makes things a lot easier. http://git.fishsoup.net/cgit/git-bz/ In particular, it adds a link to the Bugzilla bug in your commit message (automatically), which I find very useful for doing archaeology on the commit history later on (“what was the discussion which prompted this change?”). Even if you don’t use git-bz, please do start adding a link back to the bug report in your commit messages. Thanks. :-)
(In reply to Philip Withnall from comment #1) > Why are we not dealing with a nul-terminated string any more? From a quick > look through the code I can’t see how we could lose the nul terminator. I think it happens when using `s.get_next_char()` in `_strip_string()`, as it will never return the string terminator. Even if it would, it would then be stripped out in `_stripped_char()` (as its UnicodeType is UnicodeType.CONTROL). But I agree that the code should still handle null-terminated strings. Will adjust the patch to that (In reply to Philip Withnall from comment #2) > In particular, it adds a link to the Bugzilla bug in your commit message > (automatically), which I find very useful for doing archaeology on the > commit history later on (“what was the discussion which prompted this > change?”). Even if you don’t use git-bz, please do start adding a link back > to the bug report in your commit messages. Thanks. :-) Ah yes, sorry about that. I normally do this, but I was a bit distracted. My bad!
Created attachment 366377 [details] [review] [Patch] PotentialMatch: fix end-of-string check (v2) Edited patch.
Review of attachment 366377 [details] [review]: ++
Comment on attachment 366377 [details] [review] [Patch] PotentialMatch: fix end-of-string check (v2) Landed on master as commit 9c4e87e.
Created attachment 366414 [details] [review] [Patch] PotentialMatch: fix end-of-string check, part 2 Reopening this since it's basically the same issue as the one described here, but the forgotten check now being on the first string rather than the second one.
This patch fixes a long standing bug (3+ years) on OpenBSD where gnome-contacts would segfault pretty much all the time when selecting a contact. Thank you very much!
Hey, don't forget it's also thanks to you giving the feedback I needed to solve this :-)
Review of attachment 366414 [details] [review]: Sure, though it’s getting to the point where I’d *really* like to see some regression tests for this code (and tests of other odd situations) in bug #792201. When it gets to the fourth minor patch for the same bit of code, that’s a bit of a sign that writing some unit tests is due. Are you planning on working on bug #792201?
Fix landed as of commit eaaa6fa on master. Sent in a patch for bug 792201 as well. Thanks for the quick review again Philip!