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 (Nobody)
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2018-01-24 16:32 UTC by Vasil Dimov
Modified: 2021-09-22 15:12 UTC (History)
4 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.
Comment 5 Florian Smeets freebsd_committer 2021-09-22 15:12:31 UTC
It looks like iocage currently fails to successfully run "iocage update $jail" because of this behavior. I think this iocage commit https://github.com/iocage/iocage/commit/9e3f905c839b9e6dea561e5471a71bdf2ba979d1 is responsible for always setting HTTP_PROXY, even if it's empty on the host system. It also gets set if you enter the jail using "iocage console $jail", HTTP_PROXY is set to an empty string. 

I agree that iocage should be fixed in this case, but wouldn't it make sense to either apply the patch so phttpget can cope with HTTP_PROXY being empty, or at least print an error message?

In this case all you see is:

Fetching 2 metadata files... failed.

I'll create an issue on github for iocage and link to this comment, but I'd like to see us improving phttpget in one way or the other.

Thanks,
Florian