From: Amos Jeffries <squid3@treenet.co.nz>
Date: Wed, 10 Aug 2016 00:45:22 +0000 (+1200)
Subject: Cleanup: make static analyzers happier with legacy config parsing
X-Git-Tag: SQUID_4_0_14~40
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=ba72460766dcc6ffd742610731d0183ba2f1566e;p=thirdparty%2Fsquid.git

Cleanup: make static analyzers happier with legacy config parsing

Use return statements after self_destruct() helps Coverity Scan see that
there is no NULL-dereference going on and lowers is false-positive rate.
---

diff --git a/src/cache_cf.cc b/src/cache_cf.cc
index 7ce82395c2..e71fe5f0ba 100644
--- a/src/cache_cf.cc
+++ b/src/cache_cf.cc
@@ -898,6 +898,7 @@ configDoConfigure(void)
             if (!p->sslContext) {
                 debugs(3, DBG_CRITICAL, "ERROR: Could not initialize cache_peer " << p->name << " TLS context");
                 self_destruct();
+                return;
             }
         }
     }
@@ -965,11 +966,15 @@ parse_obsolete(const char *name)
         Config.redirectChildren.concurrency = cval;
     }
 
-    if (!strcmp(name, "log_access"))
+    if (!strcmp(name, "log_access")) {
         self_destruct();
+        return;
+    }
 
-    if (!strcmp(name, "log_icap"))
+    if (!strcmp(name, "log_icap")) {
         self_destruct();
+        return;
+    }
 
     if (!strcmp(name, "ignore_ims_on_miss")) {
         // the replacement directive cache_revalidate_on_miss has opposite meanings for ON/OFF value
@@ -1001,6 +1006,7 @@ parse_obsolete(const char *name)
         else {
             debugs(3, DBG_CRITICAL, "ERROR: unknown directive: " << name);
             self_destruct();
+            return;
         }
 
         // add the value as unquoted-string because the old values did not support whitespace
@@ -1015,13 +1021,17 @@ parse_obsolete(const char *name)
 static void
 parseTimeLine(time_msec_t * tptr, const char *units,  bool allowMsec,  bool expectMoreArguments = false)
 {
-    time_msec_t u;
-    if ((u = parseTimeUnits(units, allowMsec)) == 0)
+    time_msec_t u = parseTimeUnits(units, allowMsec);
+    if (u == 0) {
         self_destruct();
+        return;
+    }
 
-    char *token;
-    if ((token = ConfigParser::NextToken()) == NULL)
+    char *token = ConfigParser::NextToken();;
+    if (!token) {
         self_destruct();
+        return;
+    }
 
     double d = xatof(token);
 
@@ -1033,6 +1043,7 @@ parseTimeLine(time_msec_t * tptr, const char *units,  bool allowMsec,  bool expe
 
         } else if (!expectMoreArguments) {
             self_destruct();
+            return;
 
         } else {
             token = NULL; // show default units if dying below
@@ -1761,14 +1772,17 @@ check_null_string(char *s)
 static void
 parse_authparam(Auth::ConfigVector * config)
 {
-    char *type_str;
-    char *param_str;
-
-    if ((type_str = ConfigParser::NextToken()) == NULL)
+    char *type_str = ConfigParser::NextToken();
+    if (!type_str) {
         self_destruct();
+        return;
+    }
 
-    if ((param_str = ConfigParser::NextToken()) == NULL)
+    char *param_str = ConfigParser::NextToken();
+    if (!param_str) {
         self_destruct();
+        return;
+    }
 
     /* find a configuration for the scheme in the currently parsed configs... */
     Auth::Config *schemeCfg = Auth::Config::Find(type_str);
@@ -1780,6 +1794,7 @@ parse_authparam(Auth::ConfigVector * config)
         if (theScheme == NULL) {
             debugs(3, DBG_CRITICAL, "Parsing Config File: Unknown authentication scheme '" << type_str << "'.");
             self_destruct();
+            return;
         }
 
         config->push_back(theScheme->createConfig());
@@ -1787,6 +1802,7 @@ parse_authparam(Auth::ConfigVector * config)
         if (schemeCfg == NULL) {
             debugs(3, DBG_CRITICAL, "Parsing Config File: Corruption configuring authentication scheme '" << type_str << "'.");
             self_destruct();
+            return;
         }
     }
 
@@ -1827,20 +1843,19 @@ find_fstype(char *type)
 static void
 parse_cachedir(Store::DiskConfig *swap)
 {
-    char *type_str;
-    char *path_str;
-    RefCount<SwapDir> sd;
-    int i;
-    int fs;
-
-    if ((type_str = ConfigParser::NextToken()) == NULL)
+    char *type_str = ConfigParser::NextToken();
+    if (!type_str) {
         self_destruct();
+        return;
+    }
 
-    if ((path_str = ConfigParser::NextToken()) == NULL)
+    char *path_str = ConfigParser::NextToken();
+    if (!path_str) {
         self_destruct();
+        return;
+    }
 
-    fs = find_fstype(type_str);
-
+    int fs = find_fstype(type_str);
     if (fs < 0) {
         debugs(3, DBG_PARSE_NOTE(DBG_IMPORTANT), "ERROR: This proxy does not support the '" << type_str << "' cache type. Ignoring.");
         return;
@@ -1848,7 +1863,8 @@ parse_cachedir(Store::DiskConfig *swap)
 
     /* reconfigure existing dir */
 
-    for (i = 0; i < swap->n_configured; ++i) {
+    RefCount<SwapDir> sd;
+    for (int i = 0; i < swap->n_configured; ++i) {
         assert (swap->swapDirs[i].getRaw());
 
         if ((strcasecmp(path_str, dynamic_cast<SwapDir *>(swap->swapDirs[i].getRaw())->path)) == 0) {
@@ -2015,19 +2031,21 @@ GetUdpService(void)
 static void
 parse_peer(CachePeer ** head)
 {
-    char *token = NULL;
-    CachePeer *p = new CachePeer;
-
-    if ((token = ConfigParser::NextToken()) == NULL)
+    char *host_str = ConfigParser::NextToken();
+    if (!host_str) {
         self_destruct();
+        return;
+    }
 
-    p->host = xstrdup(token);
-
-    p->name = xstrdup(token);
-
-    if ((token = ConfigParser::NextToken()) == NULL)
+    char *token = ConfigParser::NextToken();
+    if (!token) {
         self_destruct();
+        return;
+    }
 
+    CachePeer *p = new CachePeer;
+    p->host = xstrdup(host_str);
+    p->name = xstrdup(host_str);
     p->type = parseNeighborType(token);
 
     if (p->type == PEER_MULTICAST) {
@@ -2037,8 +2055,11 @@ parse_peer(CachePeer ** head)
 
     p->http_port = GetTcpService();
 
-    if (!p->http_port)
+    if (!p->http_port) {
+        delete p;
         self_destruct();
+        return;
+    }
 
     p->icp.port = GetUdpService();
 
@@ -2390,13 +2411,14 @@ free_denyinfo(AclDenyInfoList ** list)
 static void
 parse_peer_access(void)
 {
-    char *host = NULL;
-    CachePeer *p;
-
-    if (!(host = ConfigParser::NextToken()))
+    char *host = ConfigParser::NextToken();
+    if (!host) {
         self_destruct();
+        return;
+    }
 
-    if ((p = peerFindByName(host)) == NULL) {
+    CachePeer *p = peerFindByName(host);
+    if (!p) {
         debugs(15, DBG_CRITICAL, "" << cfg_filename << ", line " << config_lineno << ": No cache_peer '" << host << "'");
         return;
     }
@@ -2409,30 +2431,31 @@ parse_peer_access(void)
 static void
 parse_hostdomaintype(void)
 {
-    char *host = NULL;
-    char *type = NULL;
-    char *domain = NULL;
-
-    if (!(host = ConfigParser::NextToken()))
+    char *host = ConfigParser::NextToken();
+    if (!host) {
         self_destruct();
+        return;
+    }
 
-    if (!(type = ConfigParser::NextToken()))
+    char *type = ConfigParser::NextToken();
+    if (!type) {
         self_destruct();
+        return;
+    }
 
+    char *domain = nullptr;
     while ((domain = ConfigParser::NextToken())) {
-        NeighborTypeDomainList *l = NULL;
-        NeighborTypeDomainList **L = NULL;
-        CachePeer *p;
-
-        if ((p = peerFindByName(host)) == NULL) {
+        CachePeer *p = peerFindByName(host);
+        if (!p) {
             debugs(15, DBG_CRITICAL, "" << cfg_filename << ", line " << config_lineno << ": No cache_peer '" << host << "'");
             return;
         }
 
-        l = static_cast<NeighborTypeDomainList *>(xcalloc(1, sizeof(NeighborTypeDomainList)));
+        auto *l = static_cast<NeighborTypeDomainList *>(xcalloc(1, sizeof(NeighborTypeDomainList)));
         l->type = parseNeighborType(type);
         l->domain = xstrdup(domain);
 
+        NeighborTypeDomainList **L = nullptr;
         for (L = &(p->typelist); *L; L = &((*L)->next));
         *L = l;
     }
@@ -2488,9 +2511,10 @@ void
 parse_onoff(int *var)
 {
     char *token = ConfigParser::NextToken();
-
-    if (token == NULL)
+    if (!token) {
         self_destruct();
+        return;
+    }
 
     if (!strcmp(token, "on")) {
         *var = 1;
@@ -2529,9 +2553,10 @@ static void
 parse_tristate(int *var)
 {
     char *token = ConfigParser::NextToken();
-
-    if (token == NULL)
+    if (!token) {
         self_destruct();
+        return;
+    }
 
     if (!strcmp(token, "on")) {
         *var = 1;
@@ -2557,9 +2582,10 @@ void
 parse_pipelinePrefetch(int *var)
 {
     char *token = ConfigParser::PeekAtToken();
-
-    if (token == NULL)
+    if (!token) {
         self_destruct();
+        return;
+    }
 
     if (!strcmp(token, "on")) {
         debugs(0, DBG_PARSE_NOTE(DBG_IMPORTANT), "WARNING: 'pipeline_prefetch on' is deprecated. Please update to use 1 (or a higher number).");
@@ -2820,11 +2846,13 @@ dump_string(StoreEntry * entry, const char *name, char *var)
 static void
 parse_string(char **var)
 {
-    char *token = ConfigParser::NextToken();
     safe_free(*var);
 
-    if (token == NULL)
+    char *token = ConfigParser::NextToken();
+    if (!token) {
         self_destruct();
+        return;
+    }
 
     *var = xstrdup(token);
 }
@@ -2868,11 +2896,13 @@ parse_eol(char *volatile *var)
 static void
 parse_TokenOrQuotedString(char **var)
 {
-    char *token = ConfigParser::NextQuotedToken();
     safe_free(*var);
 
-    if (token == NULL)
+    char *token = ConfigParser::NextQuotedToken();
+    if (!token){
         self_destruct();
+        return;
+    }
 
     *var = xstrdup(token);
 }
@@ -3110,9 +3140,10 @@ static void
 parse_uri_whitespace(int *var)
 {
     char *token = ConfigParser::NextToken();
-
-    if (token == NULL)
+    if (!token) {
         self_destruct();
+        return;
+    }
 
     if (!strcmp(token, "strip"))
         *var = URI_WHITESPACE_STRIP;
@@ -3219,8 +3250,10 @@ static void
 parse_memcachemode(SquidConfig *)
 {
     char *token = ConfigParser::NextToken();
-    if (!token)
+    if (!token) {
         self_destruct();
+        return;
+    }
 
     if (strcmp(token, "always") == 0) {
         Config.onoff.memory_cache_first = 1;
@@ -3298,8 +3331,10 @@ parse_IpAddress_list(Ip::Address_list ** head)
             s->s = ipa;
 
             *head = s;
-        } else
+        } else {
             self_destruct();
+            return;
+        }
     }
 }
 
@@ -3357,16 +3392,19 @@ parsePortSpecification(const AnyP::PortCfgPointer &s, char *token)
         if (!t) {
             debugs(3, DBG_CRITICAL, "FATAL: " << portType << "_port: missing ']' on IPv6 address: " << token);
             self_destruct();
+            return;
         }
         *t = '\0';
         ++t;
         if (*t != ':') {
             debugs(3, DBG_CRITICAL, "FATAL: " << portType << "_port: missing Port in: " << token);
             self_destruct();
+            return;
         }
         if (!Ip::EnableIpv6) {
             debugs(3, DBG_CRITICAL, "FATAL: " << portType << "_port: IPv6 is not available.");
             self_destruct();
+            return;
         }
         port = xatos(t + 1);
     } else if ((t = strchr(token, ':'))) {
@@ -3382,11 +3420,13 @@ parsePortSpecification(const AnyP::PortCfgPointer &s, char *token)
     } else {
         debugs(3, DBG_CRITICAL, "FATAL: " << portType << "_port: missing Port: " << token);
         self_destruct();
+        return;
     }
 
     if (port == 0 && host != NULL) {
         debugs(3, DBG_CRITICAL, "FATAL: " << portType << "_port: Port cannot be 0: " << token);
         self_destruct();
+        return;
     }
 
     if (NULL == host) {
@@ -3443,6 +3483,7 @@ parse_port_option(AnyP::PortCfgPointer &s, char *token)
         if (s->flags.isIntercepted()) {
             debugs(3, DBG_CRITICAL, "FATAL: " << cfg_directive << ": Accelerator mode requires its own port. It cannot be shared with other modes.");
             self_destruct();
+            return;
         }
         s->flags.accelSurrogate = true;
         s->vhost = true;
@@ -3450,6 +3491,7 @@ parse_port_option(AnyP::PortCfgPointer &s, char *token)
         if (s->flags.accelSurrogate || s->flags.tproxyIntercept) {
             debugs(3, DBG_CRITICAL, "FATAL: " << cfg_directive << ": Intercept mode requires its own interception port. It cannot be shared with other modes.");
             self_destruct();
+            return;
         }
         s->flags.natIntercept = true;
         Ip::Interceptor.StartInterception();
@@ -3460,6 +3502,7 @@ parse_port_option(AnyP::PortCfgPointer &s, char *token)
         if (s->flags.natIntercept || s->flags.accelSurrogate) {
             debugs(3,DBG_CRITICAL, "FATAL: " << cfg_directive << ": TPROXY option requires its own interception port. It cannot be shared with other modes.");
             self_destruct();
+            return;
         }
         s->flags.tproxyIntercept = true;
         Ip::Interceptor.StartTransparency();
@@ -3473,6 +3516,7 @@ parse_port_option(AnyP::PortCfgPointer &s, char *token)
         if (!Ip::Interceptor.ProbeForTproxy(s->s)) {
             debugs(3, DBG_CRITICAL, "FATAL: " << cfg_directive << ": TPROXY support in the system does not work.");
             self_destruct();
+            return;
         }
 
     } else if (strcmp(token, "require-proxy-header") == 0) {
@@ -3487,6 +3531,7 @@ parse_port_option(AnyP::PortCfgPointer &s, char *token)
         if (!s->flags.accelSurrogate) {
             debugs(3, DBG_CRITICAL, "FATAL: " << cfg_directive << ": defaultsite option requires Acceleration mode flag.");
             self_destruct();
+            return;
         }
         safe_free(s->defaultsite);
         s->defaultsite = xstrdup(token + 12);
@@ -3505,24 +3550,28 @@ parse_port_option(AnyP::PortCfgPointer &s, char *token)
         if (!s->flags.accelSurrogate) {
             debugs(3, DBG_CRITICAL, "FATAL: " << cfg_directive << ": vport option requires Acceleration mode flag.");
             self_destruct();
+            return;
         }
         s->vport = -1;
     } else if (strncmp(token, "vport=", 6) == 0) {
         if (!s->flags.accelSurrogate) {
             debugs(3, DBG_CRITICAL, "FATAL: " << cfg_directive << ": vport option requires Acceleration mode flag.");
             self_destruct();
+            return;
         }
         s->vport = xatos(token + 6);
     } else if (strncmp(token, "protocol=", 9) == 0) {
         if (!s->flags.accelSurrogate) {
             debugs(3, DBG_CRITICAL, "FATAL: " << cfg_directive << ": protocol option requires Acceleration mode flag.");
             self_destruct();
+            return;
         }
         s->transport = parsePortProtocol(ToUpper(SBuf(token + 9)));
     } else if (strcmp(token, "allow-direct") == 0) {
         if (!s->flags.accelSurrogate) {
             debugs(3, DBG_CRITICAL, "FATAL: " << cfg_directive << ": allow-direct option requires Acceleration mode flag.");
             self_destruct();
+            return;
         }
         s->allow_direct = true;
     } else if (strcmp(token, "act-as-origin") == 0) {
@@ -3535,6 +3584,7 @@ parse_port_option(AnyP::PortCfgPointer &s, char *token)
         if (!s->flags.accelSurrogate) {
             debugs(3, DBG_CRITICAL, "FATAL: " << cfg_directive << ": ignore-cc option requires Acceleration mode flag.");
             self_destruct();
+            return;
         }
 #endif
         s->ignore_cc = true;
@@ -3556,12 +3606,15 @@ parse_port_option(AnyP::PortCfgPointer &s, char *token)
             s->disable_pmtu_discovery = DISABLE_PMTU_TRANSPARENT;
         else if (!strcmp(token + 23, "always"))
             s->disable_pmtu_discovery = DISABLE_PMTU_ALWAYS;
-        else
+        else {
             self_destruct();
+            return;
+        }
     } else if (strcmp(token, "ipv4") == 0) {
         if ( !s->s.setIPv4() ) {
             debugs(3, DBG_CRITICAL, "FATAL: " << cfg_directive << ": IPv6 addresses cannot be used as IPv4-Only. " << s->s );
             self_destruct();
+            return;
         }
     } else if (strcmp(token, "tcpkeepalive") == 0) {
         s->tcp_keepalive.enabled = true;
@@ -3697,26 +3750,31 @@ parsePortCfg(AnyP::PortCfgPointer *head, const char *optionName)
         if (s->flags.tunnelSslBumping && !hijacked) {
             debugs(3, DBG_CRITICAL, "FATAL: ssl-bump on https_port requires tproxy/intercept which is missing.");
             self_destruct();
+            return;
         }
         if (hijacked && !s->flags.tunnelSslBumping) {
             debugs(3, DBG_CRITICAL, "FATAL: tproxy/intercept on https_port requires ssl-bump which is missing.");
             self_destruct();
+            return;
         }
 #endif
         if (s->flags.proxySurrogate) {
             debugs(3,DBG_CRITICAL, "FATAL: https_port: require-proxy-header option is not supported on HTTPS ports.");
             self_destruct();
+            return;
         }
     } else if (protoName.cmp("FTP") == 0) {
         /* ftp_port does not support ssl-bump */
         if (s->flags.tunnelSslBumping) {
             debugs(3, DBG_CRITICAL, "FATAL: ssl-bump is not supported for ftp_port.");
             self_destruct();
+            return;
         }
         if (s->flags.proxySurrogate) {
             // Passive FTP data channel does not work without deep protocol inspection in the frontend.
             debugs(3,DBG_CRITICAL, "FATAL: require-proxy-header option is not supported on ftp_port.");
             self_destruct();
+            return;
         }
     }
 
@@ -4163,8 +4221,8 @@ parse_CpuAffinityMap(CpuAffinityMap **const cpuAffinityMap)
     debugs(3, DBG_CRITICAL, "FATAL: Squid built with no CPU affinity " <<
            "support, do not set 'cpu_affinity_map'");
     self_destruct();
-#endif /* HAVE_CPU_AFFINITY */
 
+#else /* HAVE_CPU_AFFINITY */
     if (!*cpuAffinityMap)
         *cpuAffinityMap = new CpuAffinityMap;
 
@@ -4185,6 +4243,7 @@ parse_CpuAffinityMap(CpuAffinityMap **const cpuAffinityMap)
                "contain numbers <= 0");
         self_destruct();
     }
+#endif
 }
 
 static void
@@ -4307,10 +4366,12 @@ static void parse_icap_service_failure_limit(Adaptation::Icap::Config *cfg)
     if (strcmp(token,"in") != 0) {
         debugs(3, DBG_CRITICAL, "expecting 'in' on'"  << config_input_line << "'");
         self_destruct();
+        return;
     }
 
     if ((token = ConfigParser::NextToken()) == NULL) {
         self_destruct();
+        return;
     }
 
     d = static_cast<time_t> (xatoi(token));
@@ -4322,8 +4383,11 @@ static void parse_icap_service_failure_limit(Adaptation::Icap::Config *cfg)
     else if ((token = ConfigParser::NextToken()) == NULL) {
         debugs(3, DBG_CRITICAL, "No time-units on '" << config_input_line << "'");
         self_destruct();
-    } else if ((m = parseTimeUnits(token, false)) == 0)
+        return;
+    } else if ((m = parseTimeUnits(token, false)) == 0) {
         self_destruct();
+        return;
+    }
 
     cfg->oldest_service_failure = (m * d);
 }
@@ -4791,12 +4855,14 @@ parse_UrlHelperTimeout(SquidConfig::UrlHelperTimeout *config)
             } else {
                 debugs(3, DBG_CRITICAL, "FATAL: unsuported \"on_timeout\"  action:" << value);
                 self_destruct();
+                return;
             }
         } else if (strcasecmp(key, "response") == 0) {
             config->response = xstrdup(value);
         } else {
             debugs(3, DBG_CRITICAL, "FATAL: unsuported option " << key);
             self_destruct();
+            return;
         }
     }