Bug 225434 - [patch] Treat set but empty environment variables as unset in /usr/libexec/phttpget
Summary: [patch] Treat set but empty environment variables as unset in /usr/libexec/ph...
Status: New
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 11.1-STABLE
Hardware: Any Any
: --- Affects Many People
Assignee: freebsd-bugs mailing list
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2018-01-24 16:32 UTC by Vasil Dimov
Modified: 2018-01-27 14:35 UTC (History)
3 users (show)

See Also:


Attachments
patch to treat set-to-empty environment variables as not-set (1.39 KB, patch)
2018-01-24 16:32 UTC, Vasil Dimov
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vasil Dimov freebsd_committer 2018-01-24 16:32:11 UTC
Created attachment 190037 [details]
patch to treat set-to-empty environment variables as not-set

If http_proxy is set to an empty string in the environment of portsnap, then this is propagated to /usr/libexec/phttpget (which is used by portsnap) and this tool then fails, but the error is silenced and ignored by portsnap, leading to "metadata corrupt" error down the execution of portsnap:

root# portsnap --debug --interactive fetch
...
Fetching 5 metadata files...
/usr/libexec/phttpget ec2-eu-west-1.portsnap.freebsd.org f/34c6...
phttpget: host = , port = 3128: hostname nor servname provided, or not known
Verifying 34c6... /usr/sbin/portsnap: cannot open 34c6....gz: No such file or directory
metadata is corrupt.

The mailing lists and forums are littered with similar reports, for example:
https://forums.freebsd.org/threads/901/page-2

A shell script may contain http_proxy=${http_proxy:-""} in which case http_proxy will be set to an empty string and phttpget should treat that as if the variable is not set (no proxy provided).
Comment 1 Conrad Meyer freebsd_committer 2018-01-25 21:25:04 UTC
It's unclear to me why HTTP_PROXY is set to an empty string and http_proxy is set to the actual proxy.  That seems like an environmental misconfiguration to me.

Also unclear why a shell script should export an empty string value instead of not exporting it.
Comment 2 Vasil Dimov freebsd_committer 2018-01-25 21:39:26 UTC
(In reply to Conrad Meyer from comment #1)

Conrad, you got it wrong, just "http_proxy" needs to be set to an empty string to trigger the bug (with HTTP_PROXY being irrelevant).

> Also unclear why a shell script should export an empty string value instead of not exporting it.

Because it may contain http_proxy=${http_proxy:-""} which is a common idiom.

root# http_proxy= portsnap --debug fetch
...
phttpget: host = , port = 3128: hostname nor servname provided, or not known
...
Comment 3 Conrad Meyer freebsd_committer 2018-01-25 23:10:29 UTC
(In reply to Vasil Dimov from comment #2)
> Conrad, you got it wrong, just "http_proxy" needs to be set to an empty
> string to trigger the bug (with HTTP_PROXY being irrelevant).

Did I?  The forum link seems to suggest the opposite:

> The problem will occur if you set http_proxy variable instead of setting
> both HTTP_PROXY and HTTP_PROXY_AUTH.

And the code check HTTP_PROXY first.

> Because it may contain http_proxy=${http_proxy:-""} which is a common idiom.

Citation needed?  That line seems wrong for most/all uses.  It explicitly replaces unset variable with an empty string version, which causes this whole mess.

By the way, I don't think the user agent portion of the proposed change makes sense.  An empty string user agent may be desirable, even if non-conformant.
Comment 4 Vasil Dimov freebsd_committer 2018-01-26 18:01:20 UTC
Well, to remove any confusion:

130         env_HTTP_PROXY = getenv("HTTP_PROXY");
131         if (env_HTTP_PROXY == NULL)
132                 env_HTTP_PROXY = getenv("http_proxy");
133         if (env_HTTP_PROXY != NULL) {
134                 if (strncmp(env_HTTP_PROXY, "http://", 7) == 0)
135                         env_HTTP_PROXY += 7;
136                 p = strchr(env_HTTP_PROXY, '/');
137                 if (p != NULL)
138                         *p = 0;
139                 p = strchr(env_HTTP_PROXY, ':');
140                 if (p != NULL) {
141                         *p = 0;
142                         proxyport = p + 1;
143                 } else
144                         proxyport = "3128";
145         }

The bug surfaces if:

1. HTTP_PROXY is set to an empty string (in this case the value of http_proxy is not looked up / is ignored)
OR
2. HTTP_PROXY is unset and http_proxy is set to an empty string

> By the way, I don't think the user agent portion of the proposed change makes > sense.  An empty string user agent may be desirable, even if non-conformant.
Agreed.