GNOME Bugzilla – Bug 781771
Fix some issues found by Coverity Scan and clang
Last modified: 2018-09-21 16:31:36 UTC
Created attachment 350485 [details] [review] proposed patch The attached is a proposed patch which fixes issues reported by Coverity Scan and clang. If you are interested then I can share the full log with all defects (some false positives) which had been found.
Comment on attachment 350485 [details] [review] proposed patch >@@ -74,7 +74,7 @@ soup_auth_basic_get_protection_space (SoupAuth *auth, SoupURI *source_uri) > > /* Strip filename component */ > p = strrchr (space, '/'); >- if (p == space && p[1]) >+ if (p && p == space && p[1]) This is a false positive although coverity can't really prove it. (HTTP SoupURIs always have a non-NULL path, therefore g_strdup() can't return NULL, therefore if p==space then p!=NULL.) I feel like the "correction" obscures more than it helps. You could add an assert or return-if-fail, although in that case it really belongs in the soup_auth_get_protection_space() wrapper, not soup-auth-basic, and coverity probably isn't clever enough to take advantage of that... >@@ -268,7 +268,7 @@ soup_auth_negotiate_update_connection (SoupConnectionAuth *auth, SoupMessage *ms > } else { > /* FIXME: report further upward via > * soup_message_get_error_message */ >- g_warning ("gssapi step failed: %s", err->message); >+ g_warning ("gssapi step failed: %s", err ? err->message : "Unknown error"); Tracing through the code, the only way this could happen would be if GSSAPI authentication succeeded in 0 steps. (soup_gss_client_step() returns AUTH_GSS_COMPLETE and so soup_gss_build_response() returns FALSE with no error). That seems like a "can't happen" and maybe should be checked with a return-if-fail somewhere else. But checking here is also OK I guess. I might go with "Internal error". >+++ b/libsoup/soup-message-headers.c >@@ -1324,7 +1324,7 @@ content_type_setter (SoupMessageHeaders *hdrs, const char *value) > { > g_free (hdrs->content_type); > if (value) { >- char *content_type, *p; >+ char *content_type = NULL, *p; > > parse_content_foo (hdrs, "Content-Type", &content_type, NULL); > p = strpbrk (content_type, " /"); Another false positive coverity wouldn't be able to detect. But your fix should end up generating a warning of its own; if content_type didn't get set by parse_content_foo() then you'd end up calling strpbrk(NULL, " /"). Maybe add a "g_return_if_fail (content_type != NULL)" after the parse_content_foo()? >+++ b/libsoup/soup-xmlrpc.c >@@ -588,7 +588,7 @@ signature_get_next_complete_type (const char **signature) > > (*signature)++; > >- if ( (*signature)[0] == stack[stack_len - 1]) >+ if ( stack_len > 0 && (*signature)[0] == stack[stack_len - 1]) This is a false positive because the signature has already been determined to be valid. You could add a g_assert() if you want to pacify coverity. >+++ b/tests/test-utils.c >@@ -676,6 +678,8 @@ soup_test_request_read_all (SoupRequest *req, > > if (!SOUP_IS_SESSION_SYNC (soup_request_get_session (req))) > data.loop = g_main_loop_new (g_main_context_get_thread_default (), FALSE); >+ else >+ data.loop = NULL; > > do { > if (SOUP_IS_SESSION_SYNC (soup_request_get_session (req))) { >@@ -691,7 +695,7 @@ soup_test_request_read_all (SoupRequest *req, > } > } while (nread > 0); > >- if (!SOUP_IS_SESSION_SYNC (soup_request_get_session (req))) >+ if (data.loop) > g_main_loop_unref (data.loop); Ugh... Apparently coverity is assuming that the SOUP_IS_SESSION_SYNC check could be returning a different value at different times. Change the other SOUP_IS_SESSION_SYNC() check to check data.loop too then though, just for consistency.
(In reply to Dan Winship from comment #1) > Comment on attachment 350485 [details] [review] [review] > proposed patch > > >@@ -74,7 +74,7 @@ soup_auth_basic_get_protection_space (SoupAuth *auth, SoupURI *source_uri) > > > > /* Strip filename component */ > > p = strrchr (space, '/'); > >- if (p == space && p[1]) > >+ if (p && p == space && p[1]) > > This is a false positive although coverity can't really prove it. (HTTP > SoupURIs always have a non-NULL path, therefore g_strdup() can't return > NULL, therefore if p==space then p!=NULL.) I feel like the "correction" > obscures more than it helps. Right, it is a false positive. It even means that if 'space != NULL', then 'p == space' implies 'p != NULL'. Nonetheless, the 'else' also uses 'p && ...', thus I added it here as well. Coverity's logic behind the claim is: > libsoup-2.56.0/libsoup/soup-auth-basic.c:77: returned_null: "strrchr" returns null (checked 10 out of 10 times). > libsoup-2.56.0/examples/simple-httpd.c:87: example_assign: Example 1: Assigning: "slash" = return value from "strrchr(path, 47)". > libsoup-2.56.0/examples/simple-httpd.c:88: example_checked: Example 1 (cont.): "slash" has its value checked in "slash". > libsoup-2.56.0/libsoup/soup-auth-basic.c:77: example_assign: Example 2: Assigning: "p" = return value from "strrchr(space, 47)". > libsoup-2.56.0/libsoup/soup-auth-basic.c:80: example_checked: Example 2 (cont.): "p" has its value checked in "p". > libsoup-2.56.0/libsoup/soup-auth-digest.c:218: example_assign: Example 3: Assigning: "slash" = return value from "strrchr(dir, 47)". > libsoup-2.56.0/libsoup/soup-auth-digest.c:219: example_checked: Example 3 (cont.): "slash" has its value checked in "slash". > libsoup-2.56.0/libsoup/soup-auth-negotiate.c:158: example_assign: Example 4: Assigning: "p" = return value from "strrchr(space, 47)". > libsoup-2.56.0/libsoup/soup-auth-negotiate.c:159: example_checked: Example 4 (cont.): "p" has its value checked in "p". > libsoup-2.56.0/libsoup/soup-auth-ntlm.c:392: example_assign: Example 5: Assigning: "p" = return value from "strrchr(space, 47)". > libsoup-2.56.0/libsoup/soup-auth-ntlm.c:393: example_checked: Example 5 (cont.): "p" has its value checked in "p". > libsoup-2.56.0/libsoup/soup-auth-basic.c:77: var_assigned: Assigning: "p" = null return value from "strrchr". > libsoup-2.56.0/libsoup/soup-auth-basic.c:78: dereference: Dereferencing a null pointer "p". > You could add an assert or return-if-fail, although in that case it really > belongs in the soup_auth_get_protection_space() wrapper, not soup-auth-basic, > and coverity probably isn't clever enough to take advantage of that... If you do not agree with the 'p &&', then I'd rather skip it. > I might go with "Internal error". Okay, noted. > Another false positive coverity wouldn't be able to detect. But your fix > should end up generating a warning of its own It didn't, as far as I can tell. > Maybe add a "g_return_if_fail (content_type != NULL)" after the > parse_content_foo()? Sure, sounds good to me. > This is a false positive because the signature has already been determined > to be valid. You could add a g_assert() if you want to pacify coverity. I do not think so. The 'while' statement says 'while (stack_len > 0)', thus either the while invariant is wrong too or the stack_len can be set to 0 here, in which case Coverity claims: > libsoup-2.56.0/libsoup/soup-xmlrpc.c:591: overrun-local: Overrunning array "stack" of 256 bytes at byte offset 4294967295 using index "stack_len - 1U" (which evaluates to 4294967295). > Change the other SOUP_IS_SESSION_SYNC() check to check data.loop too then > though, just for consistency. Right, sure, I missed that one. Noted. I'm not updating the patch until we settle with the other parts of it. I'll add those 'Noted' parts to it, plus any additional changes, if needed.
Created attachment 371025 [details] [review] proposed patch ][ Tomas asked me to update the patch. This is done on top of libsoup 2.62.1, with addressed some of the Dan concerns from above. It shrinks the list of issues from initial 43 defects to 13 defects. Some are just over-pedantic or the analyser cannot catch the code construction. Some of the addressed issues are also kind of over-pendantic, but some are detected valid use-after-free issues, thus it makes sense to test the code from time to time.
Review of attachment 371025 [details] [review]: This is really hard to review all in one patch and without the coverity report. I would start by splitting it out into a bunch of separate, small patches, where the commit message includes the corresponding portion of the Coverity report. That way, we can review each change one at a time, alongside the problem report to see what Coverity thinks is wrong. With regards to any false-positives, my recommendation is to add g_assert() statements wherever needed to silence a false-positive. Other cases should be rare and can be handled on a case-by-case basis.
Created attachment 372744 [details] scan-results-before.html This is 2.62.2 before the patch (note the patch had been created against older version of libsoup), with this summary: 1 BUFFER_SIZE 1 BUFFER_SIZE_WARNING 1 CHECKED_RETURN 28 CLANG_WARNING 1 CONSTANT_EXPRESSION_RESULT 1 DEADCODE 3 FORWARD_NULL 2 INFINITE_LOOP 3 NO_EFFECT 2 NULL_RETURNS 1 OVERRUN 2 PW.INCLUDE_RECURSION 1 PW.PARAMETER_HIDDEN 2 TAINTED_SCALAR 1 TOCTOU 3 UNUSED_VALUE
Created attachment 372745 [details] scan-results-after.html and this is after the above patch, with this summary: 1 CHECKED_RETURN 1 CLANG_WARNING 1 CONSTANT_EXPRESSION_RESULT 1 DEADCODE 3 FORWARD_NULL 2 INFINITE_LOOP 3 NO_EFFECT 2 TAINTED_SCALAR 1 TOCTOU
I fully understand that you want to see the report itself, it makes perfect sense, but I hesitate to split the patch into smaller pieces. When you look only on the summaries, what the pieces should look like? And how many is too many? There are 16 sections, and even not all of them fully covered it would been like 16 patches? I hope you can imagine it was a pain to walk through all the issues and figure out what to do to make the static analysers happy. Splitting the patch would be even more pain. (In reply to Michael Catanzaro from comment #4) > With regards to any false-positives, my recommendation is to add g_assert() > statements wherever needed to silence a false-positive. Other cases should > be rare and can be handled on a case-by-case basis. These are different kinds of false positives, not talking that g_assert() doesn't always help (analysers sometimes claim anyway) and having it in the production code is kind of bad idea, especially without having a reproducer and be 100% sure the case cannot happen. And even then the crash of an application is the last thing you want to do (graceful error handling, anyone?).
I understand that splitting the patch is work, but it's too hard for me to review in its current form, sorry. You can either split the patch for me, or try to get Dan to approve it, or wait a couple months for Claudio to return (he is on leave). (In reply to Milan Crha from comment #7) > These are different kinds of false positives, not talking that g_assert() > doesn't always help (analysers sometimes claim anyway) and having it in the > production code is kind of bad idea, especially without having a reproducer > and be 100% sure the case cannot happen. And even then the crash of an > application is the last thing you want to do (graceful error handling, > anyone?). Um, I strongly disagree. If Coverity has made an invalid assumption, then g_assert() is the right way to tell it that. "Graceful error handling" is not a goal here, because if we are wrong in thinking that Coverity is wrong, then we have a security bug and remote attackers can try to gain control of our users' computers. In this case, using g_assert() means we have only a denial of service vulnerability (still worth a CVE) rather than a code execution vulnerability. But yes, adding g_assert() should ideally only be done if the assertion is correct and not going to be hit. :P
Do you have recommended "algorithm" for the split, please? There are false positives like those INFINITE_LOOP (search for them in the after-patch log). Coverity cannot catch such without some guidance, while g_assert() is no way to do it. Of course, you can do what looks the best. I agree a crash is better than execution, still I hesitate to use g_assert(). It's only my opinion and kind of habit and it's understood that others may or may not agree with it. It's all fine.
(In reply to Milan Crha from comment #9) > Do you have recommended "algorithm" for the split, please? How about one patch per problem? > There are false positives like those INFINITE_LOOP (search for them in the > after-patch log). Coverity cannot catch such without some guidance, while > g_assert() is no way to do it. Yeah, that's true. In those cases, it's up to you whether you prefer to just leave the code unchanged, or try to modify it to avoid the false positive. If I were the one reading the Coverity report, I would want to eliminate all the complaints, or silence them somehow, but that's totally up to you. > Of course, you can do what looks the best. I agree a crash is better than > execution, still I hesitate to use g_assert(). It's only my opinion and kind > of habit and it's understood that others may or may not agree with it. It's > all fine. OK, as you prefer. The actual bugs are more important, anyway.
Created attachment 372756 [details] scan-results-after.html Updated final log. I tried to mute some of the issues which had been more or less useless, but only one worked, the other didn't and I do not know how to properly address the rest of the issues. The summary for this log is: 1 CLANG_WARNING 1 CONSTANT_EXPRESSION_RESULT 1 DEADCODE 2 FORWARD_NULL 2 INFINITE_LOOP 1 NO_EFFECT 2 TAINTED_SCALAR
The 15 (!) patches I'm going to attach are split per issue. I'm not going to copy all of the reports there, I do not know if you noticed, but the HTML file has more than 355KB and even it's HTML, it would be quite hard to read in plain text anyway (it's already harder to read when it's fancy colorized with HTML, the worse to decipher it in plain text). There is one exception, 10-coverity-scan-issues-init-var.patch, for which I do not have a reference in the scan-results-before.html. I added it anyway, because it makes sure the variable is initialized (maybe newer Coverity Scan version stopped showing it, I do not know).
Created attachment 372758 [details] [review] 01-coverity-scan-issues-mute.patch
Created attachment 372759 [details] [review] 02-coverity-scan-issues-addr.patch
Created attachment 372760 [details] [review] 03-coverity-scan-issues-null-returns.patch
Created attachment 372761 [details] [review] 04-coverity-scan-issues-unused-value.patch
Created attachment 372762 [details] [review] 05-coverity-scan-issues-null-deref-err.patch
Created attachment 372763 [details] [review] 06-coverity-scan-issues-buffer-size.patch
Created attachment 372764 [details] [review] 07-coverity-scan-issues-use-after-free.patch If nothing, then at least this one is important.
Created attachment 372765 [details] [review] 08-coverity-scan-issues-nonnull-param.patch
Created attachment 372766 [details] [review] 09-coverity-scan-issues-forward-null.patch
Created attachment 372767 [details] [review] 10-coverity-scan-issues-init-var.patch
Created attachment 372768 [details] [review] 11-coverity-scan-issues-checked-return.patch
Created attachment 372769 [details] [review] 12-coverity-scan-issues-include-recursion.patch
Created attachment 372770 [details] [review] 13-coverity-scan-issues-overrun.patch
Created attachment 372771 [details] [review] 14-coverity-scan-issues-uninit-value.patch
Created attachment 372772 [details] [review] 15-coverity-scan-issues-null-deref-gclass.patch
Created attachment 372773 [details] all-in-one.patch Just in case you'd like to also build the patches, here's a one large patch, which has all of those 15 (!) patches in one file (working with so many patches is painful, both on command line and in bugzilla; and no, GitLab interface won't help at all with it; my opinion).
Review of attachment 372759 [details] [review]: Good
Review of attachment 372760 [details] [review]: ::: libsoup-2.62.2/libsoup/soup-auth-basic.c.coverity-scan-issues @@ +75,3 @@ /* Strip filename component */ p = strrchr (space, '/'); + if (p && p == space && p[1]) I would leave this unchanged, since (a) as Dan noted, it is a false positive, and (b) since it's redundant with the p == space check (given space is guaranteed to be nonnull). ::: libsoup-2.62.2/libsoup/soup-cache.c.coverity-scan-issues @@ +823,3 @@ time_t request_time, response_time; + g_return_val_if_fail (SOUP_IS_CACHE (cache), NULL); Does this fix a Coverity trace? This is iffy. I believe libsoup generally follows the convention that g_return_if_fail/g_return_val_if_fail are not used for private functions, but there are clearly some exceptions in the code. I don't think I would add it. Coverity has gotten a bit confused here; it realizes that there might not be a SoupCache, but it fails to realize that this function is never called if so. Since I am not the maintainer of libsoup -- just playing one for the next month or two while Claudio is on leave -- I don't want to make the judgment call to allow these or not, so please don't add them. (At least not unless Dan likes them.) @@ +1080,3 @@ + g_return_val_if_fail (SOUP_IS_CACHE (cache), SOUP_CACHE_RESPONSE_STALE); + g_return_val_if_fail (SOUP_IS_MESSAGE (msg), SOUP_CACHE_RESPONSE_STALE); These ones are good. This is a public API function, so they should have been here anyway. @@ +1416,3 @@ SoupCacheEntry *entry; + g_return_if_fail (SOUP_IS_CACHE (cache)); Does this fix a Coverity trace? Please don't add these. @@ +1432,3 @@ + SoupCacheEntry *entry; + + g_return_if_fail (SOUP_IS_CACHE (cache)); Ditto. At least I see Coverity explains what it thinks is wrong here: it thinks the cache parameter could be non-null because soup_session_get_feature() can be non-null, failing to realize that this function would never be called in that case.
Review of attachment 372758 [details] [review]: I don't want to make the judgment call as to whether to allow coverity suppression annotations in libsoup, since I'm not the maintainer. So I'll use the "reviewed" status to note that I am neither accepting nor rejecting the patch.
Review of attachment 372761 [details] [review]: Oh wow. I think coverity has found a serious bug: libsoup is writing too much data into the output stream. I see the code you are removing is currently unused, but I don't think removing it is the solution. I think you should move len = strlen (buf); up above the again: label so that it doesn't overwrite len every time. Pretty sure that was what Dan must have intended when he added this code in c0414594. I wonder what all this is going to fix!
Review of attachment 372762 [details] [review]: Surely soup_gss_build_response() should always set its error if it returns FALSE.
Review of attachment 372763 [details] [review]: ::: libsoup-2.62.2/libsoup/soup-auth-digest.c.coverity-scan-issues @@ -344,3 +344,3 @@ g_checksum_update (checksum, (guchar *)":", 1); g_checksum_update (checksum, (guchar *)uri, strlen (uri)); - strncpy (hex_a2, g_checksum_get_string (checksum), 33); + memcpy (hex_a2, g_checksum_get_string (checksum), sizeof (char) * 33); OK, I agree this is a reasonable way to avoid the warning, even if it's a false-positive (as we know g_checksum_get_string will return 32 bytes). But you should do the same with the strncpy at the bottom of the function, as well. coverity failed to notice that because it was too smart for its own good: it knew the compiler would ignore the buffer size hint in the parameter list, so it ignored it and so didn't realize that buffer is also 33 bytes. FWIW, I usually fix these by either zeroing the buffer, or manually assigning the trailing = '\0' after calling strncpy, but memcpy is fine too, it just has the same problem as strncpy in that if g_checksum_get_string ever returns the wrong result, the resulting hex_a2 will improperly lack the trailing nul.
Review of attachment 372763 [details] [review]: ::: libsoup-2.62.2/libsoup/soup-auth-ntlm.c.coverity-scan-issues @@ +812,2 @@ out = g_malloc (((offset + 3) * 4) / 3 + 6); + memcpy (out, "NTLM ", 5); Good
Review of attachment 372764 [details] [review]: ::: libsoup-2.62.2/libsoup/soup-auth.c.coverity-scan-issues @@ -109,3 @@ case PROP_REALM: - if (auth->realm) - g_free (auth->realm); Wow again! Score +2 for coverity. These are both security vulnerabilities.
(In reply to Michael Catanzaro from comment #32) > Review of attachment 372761 [details] [review] [review]: > > Oh wow. I think coverity has found a serious bug: libsoup is writing too > much data into the output stream. I see the code you are removing is > currently unused, but I don't think removing it is the solution. I think you > should move len = strlen (buf); up above the again: label so that it doesn't > overwrite len every time. Pretty sure that was what Dan must have intended > when he added this code in c0414594. > > I wonder what all this is going to fix! Sorry, I misread the code. Of course your patch is fine.
Review of attachment 372761 [details] [review]: Changing this to accepted.
Review of attachment 372765 [details] [review]: ::: libsoup-2.62.2/libsoup/soup-content-sniffer-stream.c.coverity-scan-issues @@ +168,3 @@ if (sniffer->priv->buffer) { nread = MIN (count, sniffer->priv->buffer_nread); + if (buffer) Yes ::: libsoup-2.62.2/libsoup/soup-uri.c.coverity-scan-issues @@ +1349,3 @@ } + if (aliases[0] && !aliases[1] && !strcmp (aliases[0], "*")) Yes When investigating this, I noticed that the documentation of SoupServer:http-aliases needs to be updated: """The default value is an array containing the single element <literal>"*"</literal>, a special value which means that any scheme except "https" is considered to be an alias for "http".""" It should also mention wss: """The default value is an array containing the single element <literal>"*"</literal>, a special value which means that any scheme except "https" or "wss" is considered to be an alias for "http".""" Since you're going to be landing a bunch of patches, might as well fix that too.
Review of attachment 372766 [details] [review]: ::: libsoup-2.62.2/libsoup/soup-path-map.c.coverity-scan-issues @@ +186,3 @@ + if (!map) + return NULL; No, the bug here is in SoupAuthManager, lookup_auth(). It should not call a method of SoupPathMap with a NULL SoupPathMap.
Review of attachment 372767 [details] [review]: ::: libsoup-2.62.2/libsoup/soup-message-headers.c.coverity-scan-issues @@ +1325,3 @@ g_free (hdrs->content_type); if (value) { + char *content_type = NULL, *p; Dan suggested you use g_return_if_fail() here instead. (I suppose that indicates that it is OK to use in the earlier places where I had rejected it.)
Review of attachment 372768 [details] [review]: ::: libsoup-2.62.2/libsoup/soup-socket.c.coverity-scan-issues @@ +1315,3 @@ fd = g_socket_get_fd (priv->gsock); v6_only = TRUE; + g_warn_if_fail (setsockopt (fd, IPPROTO_IPV6, IPV6_V6ONLY, I don't like this, I would instead set the error parameter if the call fails.
Review of attachment 372769 [details] [review]: Yes
Review of attachment 372770 [details] [review]: ::: libsoup-2.62.2/libsoup/soup-xmlrpc.c.coverity-scan-issues @@ +589,3 @@ (*signature)++; + if (stack_len > 0 && (*signature)[0] == stack[stack_len - 1]) Dan requested you use g_assert() instead (comment #1).
Review of attachment 372771 [details] [review]: Dan has already approved this, so you're free to commit if your goal is to silence the coverity warnings, but personally I think it was more readable before, so I'll use the "reviewed" status.
Review of attachment 372772 [details] [review]: Ugh, I don't know what I think of this patch. ::: libsoup-2.62.2/libsoup/soup-auth.c.coverity-scan-issues @@ +350,3 @@ g_return_val_if_fail (SOUP_IS_AUTH (auth), FALSE); + priv = soup_auth_get_instance_private (auth); On the one hand, yes, I see that the g_return_val_if_fail does not work very well if it comes after the call to _get_instance_private(). So this change makes sense everywhere, even if it's a bit annoying. @@ +370,3 @@ g_return_val_if_fail (SOUP_IS_AUTH (auth), NULL); + klass = SOUP_AUTH_GET_CLASS (auth); But this should not be. A SoupAuth's class struct is never going to be NULL. Checking for this all over the place is unnecessary and messy. I don't know how to teach coverity this, though.
Whew, that was a lot of reviews. Thanks a bunch for working on this, Milan: static analysis is pretty important for an HTTP library. We really need to get our own set up....
(In reply to Michael Catanzaro from comment #48) > But this should not be. A SoupAuth's class struct is never going to be NULL. > Checking for this all over the place is unnecessary and messy. I don't know > how to teach coverity this, though. The macro can return NULL, like when you pass incorrect object type to the function. Coverity claims it clearly and it's correct. That means the class struct can be NULL, thus it's unsafe to dereference it without checking first. Yes, these things are overpedantic and discovers rather coder's error than code's error, but these claims are valid regardless. (In reply to Michael Catanzaro from comment #42) > I don't like this, I would instead set the error parameter if the call fails. The current code expects the function call always succeeds. The idea behind the change is to verify it is truly right. I wanted to do it in a conservative way. (In reply to Michael Catanzaro from comment #49) > We really need to get our own set up.... You can do that here: https://scan.coverity.com/
Comment on attachment 372764 [details] [review] 07-coverity-scan-issues-use-after-free.patch Created commit 0e3b9413 in ls master (2.63.3+) Created commit 9fc0ae2d in ls gnome-3-28 (2.62.3+)
Comment on attachment 372759 [details] [review] 02-coverity-scan-issues-addr.patch Created commit e01e1b74 in ls master (2.63.3+) Created commit cb9f5544 in ls gnome-3-28 (2.62.3+)
Comment on attachment 372761 [details] [review] 04-coverity-scan-issues-unused-value.patch Created commit 3c98f0bf in ls master (2.63.3+) Created commit 99e95ef1 in ls gnome-3-28 (2.62.3+)
Comment on attachment 372765 [details] [review] 08-coverity-scan-issues-nonnull-param.patch Created commit 9ec22e3b in ls master (2.63.3+) Created commit 0ce455be in ls gnome-3-28 (2.62.3+)
Comment on attachment 372769 [details] [review] 12-coverity-scan-issues-include-recursion.patch Created commit 87a624bf in ls master (2.63.3+) Created commit fa90ffe8 in ls gnome-3-28 (2.62.3+)
Review of attachment 372762 [details] [review]: Unless the call of gss_display_status() (called by soup_gss_error()) fails, in which case the 'err' is left untouched, thus NULL.
Created attachment 372814 [details] [review] 06-coverity-scan-issues-buffer-size.patch ][ Updated patch. You are right, I missed the second call. I think Coverity missed it, because the 'response' buffer wasn't stack-allocated in that function, but I can be wrong. In any case, better to replace both calls. By the way, the change in NTLM code is already committed in master, but not in the gnome-3-28 branch, thus I include it anyway.
(In reply to Milan Crha from comment #50) > The macro can return NULL, like when you pass incorrect object type to the > function. Coverity claims it clearly and it's correct. That means the class > struct can be NULL, thus it's unsafe to dereference it without checking > first. Yes, these things are overpedantic and discovers rather coder's error > than code's error, but these claims are valid regardless. I strongly disagree... it would only return NULL if the macro is misused. There's no value in addition conditions to the code that will never be hit, it just makes the code harder to reason about. Do you really add such clutter to the Evolution codebase? > (In reply to Michael Catanzaro from comment #42) > > I don't like this, I would instead set the error parameter if the call fails. > > The current code expects the function call always succeeds. The idea behind > the change is to verify it is truly right. I wanted to do it in a > conservative way. That's not true, there is already an error parameter, it's public API, and it's checked when called from SoupServer. The only place it's not checked is in the non-full soup_socket_listen(), which you presumably only want to call if you don't care about the error. So no code changes elsewhere are required, you can just set the error and move on. In the extremely unlikely event this change breaks any existing code, then that would reveal a serious bug, since the ipv6only property would have been dishonored. Failing here is valuable to ensure IPv4 connections are never made when the ipv6only property is enabled. > (In reply to Michael Catanzaro from comment #49) > > We really need to get our own set up.... > > You can do that here: https://scan.coverity.com/ I suggest open source static analysis (i.e. scan-build, not Coverity), and running on gitlab.gnome.org using our own CI infrastructure. But anyway, that's not for me to decide. Thanks again for working on this; it's valuable work. P.S. I was going to backport only the use-after-free fix to the stable branch, since that one looked serious. I see you backported them all, which is OK now that it's been done, but I'd prefer to not do this for the other patches to avoid the risk of accidentally breaking anything.
Review of attachment 372814 [details] [review]: OK
Created attachment 372815 [details] [review] 09-coverity-scan-issues-forward-null.patch ][ Sure, it's the other way around.
Created attachment 372816 [details] [review] 10-coverity-scan-issues-init-var.patch ][ Okay.
Review of attachment 372770 [details] [review]: If I read it correctly, then this is on user-provided data. With g_assert(), you'd have denial of service, which is something you surely do not want. The g_assert() can be "disabled" in compile time too. Thus I'd prefer to left the change as is.
Comment on attachment 372771 [details] [review] 14-coverity-scan-issues-uninit-value.patch Created commit 8f77b47d in ls master (2.63.3+) Created commit 4cf3395d in ls gnome-3-28 (2.62.3+)
(In reply to Michael Catanzaro from comment #58) > > You can do that here: https://scan.coverity.com/ > > I suggest open source static analysis (i.e. scan-build, not Coverity)... They provide it for open source projects for free. There is for example libical, from those I get in touch most often. Anyway, it's only an option. > P.S. I was going to backport only the use-after-free fix to the stable > branch, since that one looked serious. Oh, okay, since I noticed it, I'll use only the master branch now.
Comment on attachment 372814 [details] [review] 06-coverity-scan-issues-buffer-size.patch ][ Created commit 2d4efd22 in ls master (2.63.3+)
(In reply to Milan Crha from comment #62) > Review of attachment 372770 [details] [review] [review]: > > If I read it correctly, then this is on user-provided data. With g_assert(), > you'd have denial of service, which is something you surely do not want. The > g_assert() can be "disabled" in compile time too. Thus I'd prefer to left > the change as is. Hm, I don't know about this one. There is a comment saying that the data has already been validated, which is probably why Dan said to use g_assert(), but I don't immediately see where that has taken place, so I'm not sure.
(In reply to Milan Crha from comment #63) > Comment on attachment 372771 [details] [review] [review] > 14-coverity-scan-issues-uninit-value.patch > > Created commit 8f77b47d in ls master (2.63.3+) > Created commit 4cf3395d in ls gnome-3-28 (2.62.3+) Again, please stop pushing code changes that don't fix anything to the stable branch. It would also have been good had you mentioned that this was a false positive, and the change is just to placate Coverity. People reading the commit will think there is an actual uninitialized value read here. I think I'll revert most of them (except the double free), just to be extra cautious about what we put onto the stable branch when Claudio is away. Your changes are all still fine for master, of course.
(In reply to Michael Catanzaro from comment #67) > I think I'll revert most of them (except the double free), I wound up reverting only this most recent one (and only on the stable branch), due to the confusing commit message which might mislead distributors into thinking it is important to include as a downstream patch.
Review of attachment 372815 [details] [review]: ::: libsoup/soup-auth-manager.c @@ +480,3 @@ return NULL; + if (!host->auth_realms) Regardless of whether the input has already been sanitized/validated, I think you're right that a g_assert() would be improper here, since otherwise host->auth_realms would not be checked in the condition up above. But that also shows why you don't need this conditional here. You could simply change the previous conditional to || instead.
(In reply to Michael Catanzaro from comment #69) > Review of attachment 372815 [details] [review] [review]: > > ::: libsoup/soup-auth-manager.c > @@ +480,3 @@ > return NULL; > > + if (!host->auth_realms) > > Regardless of whether the input has already been sanitized/validated Sorry, I got this confused with the other issue, but the rest of my comment stands. :)
Review of attachment 372816 [details] [review]: OK
Again, please commit the rest of these only to master!
Review of attachment 372815 [details] [review]: Yeah, I've been thinking to make it ||, but it's wrong, because make_auto_ntlm_auth() can assign the value to host->auth_realms, thus the || is wrong. This change is to "mute" another sort-of false-positive reported by Coverity Scan (the make_auto_ntlm_auth() returns FALSE only if it did no assign the host->auth_realms value), thus the host->auth_realms is not NULL after the call to make_auto_ntlm_auth() when it returns TRUE.
Comment on attachment 372816 [details] [review] 10-coverity-scan-issues-init-var.patch ][ Created commit ef10fc84 in ls master (2.63.3+)
(In reply to Milan Crha from comment #73) > Review of attachment 372815 [details] [review] [review]: > > Yeah, I've been thinking to make it ||, but it's wrong, because > make_auto_ntlm_auth() can assign the value to host->auth_realms, thus the || > is wrong. This change is to "mute" another sort-of false-positive reported > by Coverity Scan (the make_auto_ntlm_auth() returns FALSE only if it did no > assign the host->auth_realms value), thus the host->auth_realms is not NULL > after the call to make_auto_ntlm_auth() when it returns TRUE. OK, I see, then your patch is fine if you add a comment... because the way it is written now, someone else is going to come along and "simplify" it down to the || I suggested if there's no warning comment there. Tricky business!
Comment on attachment 372815 [details] [review] 09-coverity-scan-issues-forward-null.patch ][ You are right, it's tricky there. It's so tricky that Coverity didn't realize it. I made a lengthy comment there with a little explanation. I also call it to workaround the false-positive, not a real fix of anything. Created commit 171a461f in ls master (2.63.3+)
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/libsoup/issues/103.