GNOME Bugzilla – Bug 747397
crash in _geocode_read_nominatim_attributes geocode-reverse.c:130
Last modified: 2015-06-09 12:15:00 UTC
A crash here seems to have occurred regurarly for the last two fedora releases https://retrace.fedoraproject.org/faf/problems/353259/ Now in the still cooking f22 https://bugzilla.redhat.com/show_bug.cgi?id=1209040
Seems like we are missing a NULL check on the return from json_reader_list_members, will look into and fix when in front of a computer. Thanks for report!
Created attachment 301032 [details] [review] reverse: Check for NULL result in reverse_resolve()
Created attachment 301033 [details] [review] reverse: Check for NULL from json_reader_list_members()
I was not able to reproduce the crash in the report. But these two patches fixes issues in the reverse code. The first one will make geocode_reverse_resolve abort if there is no result. The second one checks for NULL return in the offending function from the report, so it might fix that issue. But I have no solid reproducer. Where exactly is the crash coming from? Would it be possible to contact someone who regularly sees it and could try the patch?
Correction, the first one fixes an issue where geocode crashed if there was no results. :)
Both patches are missing regression tests.
Created attachment 301421 [details] [review] test-gcglib: Add test for unsuccessful reverse
Created attachment 301422 [details] [review] reverse: Check for NULL result in reverse_resolve()
Created attachment 301423 [details] [review] reverse: Check for NULL from json_reader_list_members()
Well since I haven't been able to reproduce the crash from the bug report, I am not sure how to write a regression test for it. The patch in attachment 301423 [details] [review] fixes an issue on the line that the trace from the crash reported. The seccond one fixes an issues shown by the new test_rev_fail-test in attachtment 301421. Thanks!
Review of attachment 301423 [details] [review]: ::: geocode-glib/geocode-reverse.c @@ +128,3 @@ members = json_reader_list_members (reader); + for (i = 0; members != NULL && members[i] != NULL; i++) { Pretty sure that g_strfreev() doesn't like NULL being passed to it. Best would to do check for NULL before, so: if (members == NULL) { json_reader_end_member (reader); return; }
Review of attachment 301421 [details] [review]: ::: geocode-glib/test-gcglib.c @@ +152,3 @@ + + place = geocode_reverse_resolve (rev, &error); + g_assert (place == NULL); Looks fine, though do you want to make a g_assert_error() call as well?
Review of attachment 301422 [details] [review]: ::: geocode-glib/geocode-reverse.c @@ +515,3 @@ + if (!result) + return NULL; We don't propagate an error here? Don't we need a similar check in the async version?
Review of attachment 301423 [details] [review]: ::: geocode-glib/geocode-reverse.c @@ +128,3 @@ members = json_reader_list_members (reader); + for (i = 0; members != NULL && members[i] != NULL; i++) { Agreed.
Review of attachment 301422 [details] [review]: ::: geocode-glib/geocode-reverse.c @@ +515,3 @@ + if (!result) + return NULL; We do, we will get an error that is propagated from resolve_json, with the error message "Unable to geocode", the same you get from Nominatim web service if you supply lat/lon that does not give a result. The async version already checks for NULL from resolve_json, it was missing in the sync one.
Review of attachment 301421 [details] [review]: ::: geocode-glib/test-gcglib.c @@ +152,3 @@ + + place = geocode_reverse_resolve (rev, &error); + g_assert (place == NULL); Yes, will add.
Created attachment 301484 [details] [review] test-gcglib: Add test for unsuccessful reverse
Created attachment 301485 [details] [review] reverse: Check for NULL result in reverse_resolve()
Created attachment 301486 [details] [review] reverse: Check for NULL from json_reader_list_members()
Thanks for review!
Created attachment 301487 [details] [review] test-gcglib: Add test for unsuccessful reverse
Review of attachment 301485 [details] [review]: Looks good.
Review of attachment 301486 [details] [review]: Looks good.
Review of attachment 301487 [details] [review]: Looks good.
Attachment 301485 [details] pushed as a7d4ffc - reverse: Check for NULL result in reverse_resolve() Attachment 301486 [details] pushed as 8877b11 - reverse: Check for NULL from json_reader_list_members() Attachment 301487 [details] pushed as 31729af - test-gcglib: Add test for unsuccessful reverse
Should this go into 3.14?
(In reply to Jonas Danielsson from comment #26) > Should this go into 3.14? If it applies, definitely.