Fix some issues revealed by Coverty Scan, improve documentation of parseHttpRequest

Address issues for the defect classes:
- Array compared against 0
- Copy into fixed size buffer
- Dereference after null check
- Dereference before null check

diff --git a/lib/smblib/smblib.c b/lib/smblib/smblib.c
index a83c978671b0a02c428ec78b905d6265606b9f99..79de3d61436f01bad8aed9c4dc403571fa5b5c9b 100644 (file)
--- a/lib/smblib/smblib.c
+++ b/lib/smblib/smblib.c
@@ -252,7 +252,8 @@ SMB_Handle_Type SMB_Connect(SMB_Handle_Type Con_Handle,
         SMBlib_errno = -SMBlibE_CallFailed;
         return NULL;
     }
-    strcpy(con -> desthost, host);
+    strncpy(con->desthost, host, sizeof(con->desthost));
+    con->desthost[sizeof(con->desthost)-1]='\0';
 
     /* Now connect to the remote end, but first upper case the name of the
        service we are going to call, sine some servers want it in uppercase */

diff --git a/src/auth/digest/auth_digest.cc b/src/auth/digest/auth_digest.cc
index 7c7d424eeb3945c4b7153eca5cc9f937d9d30a1a..54dbdc46139e54d5287c8ccdc84790e3e3a63334 100644 (file)
--- a/src/auth/digest/auth_digest.cc
+++ b/src/auth/digest/auth_digest.cc
@@ -1025,7 +1025,7 @@ Auth::Digest::Config::decode(char const *proxy_auth)
         }
     } else {
         /* cnonce and nc both require qop */
-        if (digest_request->cnonce || digest_request->nc) {
+        if (digest_request->cnonce || digest_request->nc[0] != '\0') {
             debugs(29, 2, "missing qop!");
             rv = authDigestLogUsername(username, digest_request);
             safe_free(username);

diff --git a/src/client_side.cc b/src/client_side.cc
index 54fcf1ab9d378d1bd77d1653deca62921f57c57b..bbcd1d82148383583cb7c568b4f01788b044cfcc 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -2141,14 +2141,18 @@ prepareTransparentURL(ConnStateData * conn, ClientHttpRequest *http, char *url,
     }
 }
 
-/**
- *  parseHttpRequest()
+/** Parse an HTTP request
  *
- *  Returns
- *  NULL on incomplete requests
- *  a ClientSocketContext structure on success or failure.
- *  Sets result->flags.parsed_ok to 0 if failed to parse the request.
- *  Sets result->flags.parsed_ok to 1 if we have a good request.
+ *  \note Sets result->flags.parsed_ok to 0 if failed to parse the request,
+ *          to 1 if the request was correctly parsed.
+ *  \param[in] csd a ConnStateData. The caller must make sure it is not null
+ *  \param[in] hp an HttpParser
+ *  \param[out] mehtod_p will be set as a side-effect of the parsing.
+ *          Pointed-to value will be set to Http::METHOD_NONE in case of
+ *          parsing failure
+ *  \param[out] http_ver will be set as a side-effect of the parsing
+ *  \return NULL on incomplete requests,
+ *          a ClientSocketContext structure on success or failure.
  */
 static ClientSocketContext *
 parseHttpRequest(ConnStateData *csd, HttpParser *hp, HttpRequestMethod * method_p, HttpVersion *http_ver)
@@ -2224,7 +2228,7 @@ parseHttpRequest(ConnStateData *csd, HttpParser *hp, HttpRequestMethod * method_
     *method_p = HttpRequestMethod(&hp->buf[hp->req.m_start], &hp->buf[hp->req.m_end]+1);
 
     /* deny CONNECT via accelerated ports */
-    if (*method_p == Http::METHOD_CONNECT && csd && csd->port && csd->port->accel) {
+    if (*method_p == Http::METHOD_CONNECT && csd->port && csd->port->accel) {
         debugs(33, DBG_IMPORTANT, "WARNING: CONNECT method received on " << csd->port->protocol << " Accelerator port " << csd->port->s.GetPort() );
         /* XXX need a way to say "this many character length string" */
         debugs(33, DBG_IMPORTANT, "WARNING: for request: " << hp->buf);

diff --git a/src/client_side_request.cc b/src/client_side_request.cc
index c1f395f3b3759cd694692f3bcc5610eedebbdeaf..2d3d9e5b293afe04d49376681dad6f294379ca5c 100644 (file)
--- a/src/client_side_request.cc
+++ b/src/client_side_request.cc
@@ -1968,7 +1968,8 @@ ClientHttpRequest::handleAdaptationFailure(int errDetail, bool bypassable)
 #endif
         calloutContext->error->detailError(errDetail);
         calloutContext->readNextRequest = true;
-        c->expectNoForwarding();
+        if (c != NULL)
+            c->expectNoForwarding();
         doCallouts();
     }
     //else if(calloutContext == NULL) is it possible?

diff --git a/src/forward.cc b/src/forward.cc
index 4ed6f43ae0deb1e71bc4b1968ce40064e7e58158..171e0a4258ad3b6327c3785ce281fd1e16184cb1 100644 (file)
--- a/src/forward.cc
+++ b/src/forward.cc
@@ -878,8 +878,10 @@ FwdState::sslCrtvdCheckForErrors(Ssl::CertValidationResponse const &resp, Ssl::E
                 const char *aReason = i->error_reason.empty() ? NULL : i->error_reason.c_str();
                 errDetails = new Ssl::ErrorDetail(i->error_no, peerCert.get(), brokenCert, aReason);
             }
-            delete check->sslErrors;
-            check->sslErrors = NULL;
+            if (check) {
+                delete check->sslErrors;
+                check->sslErrors = NULL;
+            }
         }
 
         if (!errs)

diff --git a/src/htcp.cc b/src/htcp.cc
index a671bb5f2a3dda777aaaa043179910e5d73c7f5a..b24ab0d9803219ac97f1396cf348c015ace6c61d 100644 (file)
--- a/src/htcp.cc
+++ b/src/htcp.cc
@@ -1180,14 +1180,13 @@ htcpHandleTstRequest(htcpDataHeader * dhdr, char *buf, int sz, Ip::Address &from
     /* s is a new object */
     s = htcpUnpackSpecifier(buf, sz);
 
-    s->setFrom(from);
-
-    s->setDataHeader(dhdr);
-
-    if (NULL == s) {
+    if (s == NULL) {
         debugs(31, 3, "htcpHandleTstRequest: htcpUnpackSpecifier failed");
         htcpLogHtcp(from, dhdr->opcode, LOG_UDP_INVALID, dash_str);
         return;
+    } else {
+        s->setFrom(from);
+        s->setDataHeader(dhdr);
     }
 
     if (!s->request) {

diff --git a/src/peer_proxy_negotiate_auth.cc b/src/peer_proxy_negotiate_auth.cc
index 1f926b4654080b9afcde0e1f76f367d39724900e..845e85320b3967caee34a1ac90bac03f9c041cb7 100644 (file)
--- a/src/peer_proxy_negotiate_auth.cc
+++ b/src/peer_proxy_negotiate_auth.cc
@@ -331,8 +331,7 @@ restart:
                 p = strchr(buf, ':');
                 if (p)
                     ++p;
-                if (keytab_filename)
-                    xfree(keytab_filename);
+                xfree(keytab_filename);
                 keytab_filename = xstrdup(p ? p : buf);
             } else {
                 keytab_filename = xstrdup(kf);
@@ -425,6 +424,10 @@ restart:
             mem_cache =
                 (char *) xmalloc(strlen("FILE:/tmp/peer_proxy_negotiate_auth_")
                                  + 16);
+            if (!mem_cache) {
+                debugs(11, 5, "Error while allocating memory");
+                return(1);
+            }
             snprintf(mem_cache,
                      strlen("FILE:/tmp/peer_proxy_negotiate_auth_") + 16,
                      "FILE:/tmp/peer_proxy_negotiate_auth_%d", (int) getpid());
@@ -432,6 +435,10 @@ restart:
             mem_cache =
                 (char *) xmalloc(strlen("MEMORY:peer_proxy_negotiate_auth_") +
                                  16);
+            if (!mem_cache) {
+                debugs(11, 5, "Error while allocating memory");
+                return(1);
+            }
             snprintf(mem_cache,
                      strlen("MEMORY:peer_proxy_negotiate_auth_") + 16,
                      "MEMORY:peer_proxy_negotiate_auth_%d", (int) getpid());
@@ -439,8 +446,7 @@ restart:
 
             setenv("KRB5CCNAME", mem_cache, 1);
             code = krb5_cc_resolve(kparam.context, mem_cache, &kparam.cc);
-            if (mem_cache)
-                xfree(mem_cache);
+            xfree(mem_cache);
             if (code) {
                 debugs(11, 5,
                        HERE << "Error while resolving memory credential cache : "