New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix tiny memory leak upon edge case connecting to proxy #1255

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@cam888eron

cam888eron commented Feb 8, 2017

[cURL][CID 11121] Possible tiny memory leak upon edge case connecting to proxy.

Bug found directly in source code by Coverity Connect bug-finding tool.

This is out-of-context, unusual approach. I cannot tell how serious the bug is when running the program. In any case, it purports to only leak a 1-byte '\0' NUL string terminator.

Low priority. In my opinion, this is not worth any release activity. If your org does fix it, perhaps it could go out with some more important change?

In curl-7.52.1/lib/http_proxy.c Coverity says:

221 if(!Curl_checkProxyheaders(conn, "Host:")) {
13. alloc_fn: Storage is returned from allocation function curl_maprintf. [show details]
14. var_assign: Assigning: host = storage returned from curl_maprintf("Host: %s\r\n", hostheader).
222 host = aprintf("Host: %s\r\n", hostheader);
...
21. Condition host, taking true branch
22. Condition *host, taking false branch
251 if(host && *host)
252 free(host);
...
CID 11121 (#1 of 2): Resource leak (RESOURCE_LEAK)27. leaked_storage: Variable host going out of scope leaks the storage it points to.
272 }

An empty string containing only the 1-byte
'\0' NUL string terminator can be returned at
13. alloc_fn: Storage is returned from allocation function curl_maprintf. [show details]
14. var_assign: Assigning: host = storage returned from curl_maprintf("Host: %s\r\n", hostheader).
222 host = aprintf("Host: %s\r\n", hostheader);
via curl_maprintf at
curl-7.52.1/lib/mprintf.c:#L1100 return strdup("");

Remove ' && *host' from #L251 above? That would let it free an empty string.

Let me know if I should find an example run of the program which reproduces the problem.

@mention-bot

This comment has been minimized.

mention-bot commented Feb 8, 2017

@cam888eron, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bagder, @jay and @rousskov to be potential reviewers.

@bagder

bagder approved these changes Feb 9, 2017

@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Feb 9, 2017

host is initialized to literal '\0' here:
https://github.com/cam888eron/curl/blob/d97a2dcd131c1e0dbd1dfe0fb84c91b42f2e835f/lib/http_proxy.c#L202
That probably shouln't be freed?

@bagder bagder closed this in 7fe81ec Feb 9, 2017

@bagder

This comment has been minimized.

Member

bagder commented Feb 9, 2017

Thanks!

@bagder

This comment has been minimized.

Member

bagder commented Feb 9, 2017

Thanks @MarcelRaad that seems correct. We need to fix that...

@bagder bagder reopened this Feb 9, 2017

@bagder

This comment has been minimized.

Member

bagder commented Feb 9, 2017

I'll commit this:

@@ -197,11 +197,11 @@ CURLcode Curl_proxyCONNECT(struct connectdata *conn,
       result = Curl_http_output_auth(conn, "CONNECT", host_port, TRUE);
 
       free(host_port);
 
       if(!result) {
-        char *host=(char *)"";
+        char *host = NULL;
         const char *proxyconn="";
         const char *useragent="";
         const char *http = (conn->http_proxy.proxytype == CURLPROXY_HTTP_1_0) ?
           "1.0" : "1.1";
         bool ipv6_ip = conn->bits.ipv6_ip;
@@ -240,11 +240,11 @@ CURLcode Curl_proxyCONNECT(struct connectdata *conn,
                            "%s"  /* Proxy-Authorization */
                            "%s"  /* User-Agent */
                            "%s", /* Proxy-Connection */
                            hostheader,
                            http,
-                           host,
+                           host?host:"",
                            conn->allocptr.proxyuserpwd?
                            conn->allocptr.proxyuserpwd:"",
                            useragent,
                            proxyconn);
 
@bagder

This comment has been minimized.

Member

bagder commented Feb 9, 2017

Fixed in e2e1822

@bagder bagder closed this Feb 9, 2017

@cam888eron cam888eron deleted the cam888eron:fix_tiny_memory_leak_edge_case branch Feb 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment