GNOME Bugzilla – Bug 776115
Patches for spotted Coverity errors
Last modified: 2017-02-16 20:25:39 UTC
I'm attaching some patches fixing warnings I saw in Coverity. All of these patches (except the one fixing socket() retval checks) are quite unlikely to happen, still good to keep the code robust.
Created attachment 341975 [details] [review] Check socket() return value correctly socket() returns -1 in case of error, but that was being given as good, and passed to connect(). Spotted through Coverity.
Created attachment 341976 [details] [review] Fix "Dereference after NULL check" Coverity error in PIE handler The cur variable might end up NULL on the while loop above, and its contents used in later code without further checks.
Created attachment 341977 [details] [review] Fix "Dereference after NULL check" Coverity error in RSS handler The cur variable might end up NULL on the while loop above, and its contents used in later code without further checks.
Created attachment 341978 [details] [review] Fix "Dereference after NULL check" Coverity error in ATOM handler The cur variable might end up NULL on the while loop above, and its contents used in later code without further checks.
Created attachment 341979 [details] [review] Fix "Explicit NULL dereferenced" Coverity error The callback variable might end up NULL here, causing crashes on strcmp().
Created attachment 341980 [details] [review] Fix "Explicit NULL dereferenced" Coverity error The verify variable might end up NULL here, causing crashes on strcmp().
Created attachment 342341 [details] [review] src: Add missing NULL checks for XML elements If the loop to skip blank elements ends up reading the entire document, cur will be set to NULL, and the following code (which dereferences its elements) will crash. Avoid that by checking cur again. Coverity IDs: 1388544, 1388545, 1388548
Created attachment 342342 [details] [review] src: Remove some unnecessary NULL checks These were redundant, given that feed was dereferenced on every path up to them. Add a precondition check for NULL instead. Coverity ID: 1388550
Created attachment 342343 [details] [review] src: Fix binding between mode and parameters in publisher Previously, the code assumed that the correct parameters were provided for each mode, but that’s not necessarily the case. If incorrect parameters were provided (specifically, if the ones pertaining to the given mode were missing), some NULL pointer dereferences would happen and hence the code would crash. Fix that by checking for the parameters and returning error 400 if they are not present. Coverity IDs: 1388546, 1388547
Created attachment 342344 [details] [review] src: Fix check for return value from socket() Failure is indicated by a negative number, not by 0. 0 is a valid FD, although it’s normally used by stdio. Coverity ID: 1388549
I think the original patches here are partially incorrect, as they don’t set GErrors or the HTTP return status when they bail out in error cases. Here’s a set of patches which does. (I don’t know if they’re completely right though: this is just a drive-by patch dump.)
I wrote my patches before I new Carlos’ existed, so some are duplicates.
Review of attachment 341975 [details] [review]: lgtm
Review of attachment 342341 [details] [review]: lgtm
Review of attachment 342342 [details] [review]: ::: src/feed-atom-handler.c @@ +330,3 @@ gchar *alternate = NULL; + g_return_val_if_fail GRSS_IS_FEED_CHANNEL (feed), NULL); missing opening brace
Review of attachment 342343 [details] [review]: lgtm
Thanks for the reviews. Pushed with the missing bracket fixed. I accidentally pushed my version of attachment #341975 [details] rather than Carlos’, but they are identical. f47bef4 src: Fix check for return value from socket() 79842ad src: Fix binding between mode and parameters in publisher 7021e8c src: Remove some unnecessary NULL checks 9a9376c src: Add missing NULL checks for XML elements