Ignore malformed environment strings like glibc does #29297

tbu- · 2015-10-25T12:08:28Z

Otherwise, the iterator and the functions for getting specific
environment variables might disagree, for environments like

FOOBAR

rust-highfive · 2015-10-25T12:08:41Z

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

tbu- · 2015-10-25T12:11:09Z

Here's a test program for this behavior:

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>

extern char **environ;

int main(int argc, char **argv) {
    printf("ARGV:\n");
    for(; *argv; argv++) {
        printf("%s\n", *argv);
    }
    printf("\n");
    printf("ENV:\n");
    char **env = environ;
    for(; *env; env++) {
        printf("%s\n", *env);
    }
    printf("\n");

    char *new_argv[] = {
        NULL
    };

    char *new_env[] = {
        "FOOBAR",
        NULL
    };

    if(argc != 0) {
        execve("./a.out", new_argv, new_env);
        perror("execve ./a.out");
    } else {
        char *interesting = getenv("FOOBAR");
        if(interesting) {
            printf("FOOBAR=%s\n", interesting);
        } else {
            printf("(nil)\n");
        }
    }

    return 0;
}

alexcrichton · 2015-10-26T16:45:27Z

src/libstd/sys/unix/os.rs

+        // by an ASCII equals sign '='. Since a variable name must not be
+        // empty, allow variable names starting with an equals sign. Skip all
+        // malformed lines.
+        if input.len() < 1 {


Perhaps this could just be if input.len() == 0?

if input.is_empty() also works.

I explicitly chose to use < 1 as I use the index 1 later on. But if you want me to change that, please say so.

Yeah let's just stick to is_empty here

alexcrichton · 2015-10-26T16:47:05Z

Could a linux-specific run-pass test be added for this perhaps?

tbu- · 2015-10-26T18:22:03Z

Sure, will look into that.

tbu- · 2015-10-27T12:43:31Z

What kind of test do you think of? I can't really set a malformed environment variable without resorting to the execve syscall.

eddyb · 2015-10-27T14:48:09Z

src/libstd/sys/unix/os.rs

@@ -399,24 +399,33 @@ pub unsafe fn environ() -> *mut *const *const c_char {
 pub fn env() -> Env {
    return unsafe {
        let mut environ = *environ();
-        if environ as usize == 0 {
+        if environ == ptr::null() {


Could use .is_null().

eddyb · 2015-10-27T14:49:51Z

@tbu- You'd have to maybe write a run-make test with a C program? Not sure if we support that easily.

alexcrichton · 2015-10-27T14:53:06Z

Ah yeah I was thinking of a run-pass test that just called execve directly (e.g. translate the C into Rust). I personally prefer to avoid run-make wherever possible.

tbu- · 2015-11-05T22:06:05Z

@alexcrichton Added a test.

alexcrichton · 2015-11-05T23:05:05Z

@bors: r+ 2373415a17e4d9d237b364d787efe4844c8ab98c

bors · 2015-11-05T23:37:28Z

⌛ Testing commit 2373415 with merge 9c2df72...

bors · 2015-11-06T00:27:22Z

💔 Test failed - auto-linux-64-opt

tbu- · 2015-11-06T00:32:18Z

<program name unknown>: error while loading shared libraries: libstd-71b07a99.so: cannot open shared object file: No such file or directory

Clearly caused by my changes, but I'm not sure how to fix that.

eddyb · 2015-11-06T00:48:21Z

@tbu- If your changes remove LD_LIBRARY_PATH or something similar, try appending something malformed to the environment instead of replacing it.

alexcrichton · 2015-11-06T01:46:35Z

Yeah, @eddyb's diagnosis is right. You may be able to get around this with // no-prefer-dynamic which should build a mostly-static binary

tbu- · 2015-11-06T15:02:23Z

@alexcrichton Fixed.

alexcrichton · 2015-11-06T18:07:41Z

@bors: r+ 9dc752fa3be645c74eed41c331a7c5351345ea05

bors · 2015-11-06T18:26:47Z

⌛ Testing commit 9dc752f with merge d383c36...

bors · 2015-11-06T19:20:01Z

💔 Test failed - auto-win-gnu-64-opt

tbu- · 2015-11-06T20:50:46Z

I have no experience with Windows. Does this fail because execve always fails on Windows? Does Windows need an nonempty argv?

alexcrichton · 2015-11-06T22:36:35Z

Oh execve isn't actually a thing on Windows, I believe mingw must just have a wrapper around it? It's fine to just completely ignore this tests on Windows.

tbu- · 2015-11-14T20:02:00Z

@alexcrichton I believe this one is ready to be merged.

alexcrichton · 2015-11-16T00:35:20Z

Thanks! Could you also squash all these commits down?

tbu- · 2015-11-16T01:02:54Z

@alexcrichton Done.

alexcrichton · 2015-11-16T01:06:29Z

Looks like travis is reporting some errors with make tidy

tbu- · 2015-11-16T02:09:25Z

Fixed.

alexcrichton · 2015-11-16T02:36:42Z

@bors: r+

On Sunday, November 15, 2015, tbu- notifications@github.com wrote:

Fixed.

—
Reply to this email directly or view it on GitHub
#29297 (comment).

bors · 2015-11-16T02:36:43Z

📌 Commit 2917cd4 has been approved by alexcrichton

bors · 2015-11-16T04:40:54Z

⌛ Testing commit 2917cd4 with merge fa2f0fd...

bors · 2015-11-16T05:48:34Z

💔 Test failed - auto-linux-64-x-android-t

tbu- · 2015-11-16T09:07:08Z

.. segmentation fault? That's not supposed to happen. No idea where this comes from.

alexcrichton · 2015-11-16T17:55:48Z

It may be the case that the libc on android just doesn't handle calls like this and has bug itself. Either way it's fine to ignore android as well.

alexcrichton · 2015-11-16T17:55:55Z

(so long as a comment is added as to why it's ignored on android)

Otherwise, the iterator and the functions for getting specific environment variables might disagree, for environments like FOOBAR Variable names starting with equals sign are OK: glibc only interprets equals signs not in the first position as separators between variable name and variable value. Instead of skipping them entirely, a leading equals sign is interpreted to be part of the variable name.

tbu- · 2015-11-16T23:33:42Z

@alexcrichton Ignored the test, added a comment.

alexcrichton · 2015-11-16T23:59:08Z

@bors: r+ 87243bc

Otherwise, the iterator and the functions for getting specific environment variables might disagree, for environments like FOOBAR

bors · 2015-11-17T00:31:21Z

⌛ Testing commit 87243bc with merge a7644b3...

bors · 2015-11-17T02:21:18Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

petrochenkov · 2015-11-17T23:11:06Z

@tbu-
test/run-pass/env-vars.rs fails on my Windows 8.1 64-bit / MSYS2 / MinGW-w64

---- [run-pass] run-pass/env-vars.rs stdout ----

error: test run failed!
status: exit code: 101
command: PATH="x86_64-pc-windows-gnu/stage2/lib/rustlib/x86_64-pc-windows-gnu/lib;C:\msys64\home\we\rust\x86_64-pc-windows-gnu\stage2\bin;C:\msys64\mingw64\bin;C:\msys64\usr\local\bin;C:\msys64\usr\bin;C:\msys64\usr\bin;C:\Windows\system32;C:\Windows;C:\Windows\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0;C:\ProgramData\chocolatey\bin;C:\Program Files (x86)\Skype\Phone;C:\msys64\usr\bin\site_perl;C:\msys64\usr\bin\vendor_perl;C:\msys64\usr\bin\core_perl" x86_64-pc-windows-gnu/test/run-pass\env-vars.stage2-x86_64-pc-windows-gnu.exe
stdout:
------------------------------------------

------------------------------------------
stderr:
------------------------------------------
thread '<main>' panicked at 'bad vars->var transition: "" "C:=C:\\Windows\\System32" None', C:/msys64/home/we/rust/src/test/run-pass/env-vars.rs:17

------------------------------------------

thread '[run-pass] run-pass/env-vars.rs' panicked at 'explicit panic', C:/msys64/home/we/rust/src/compiletest\runtest.rs:1501

tbu- · 2015-11-17T23:35:29Z

@petrochenkov Can you check whether #29901 fixes that?

petrochenkov · 2015-11-18T12:49:41Z

@tbu-
No.
It doesn't compile, I modified it to compile, but it still doesn't fix the test.

tbu- · 2015-11-18T14:22:41Z

@petrochenkov Made a pull request to ignore it again: #29910. See also #29911.

…ichton See also #29297.

rust-highfive assigned brson Oct 25, 2015

tbu- force-pushed the pr_env_ignore_malformed branch from 9e8bf6e to 556a121 Compare October 25, 2015 21:58

alexcrichton reviewed Oct 26, 2015
View reviewed changes

eddyb reviewed Oct 27, 2015
View reviewed changes

tbu- force-pushed the pr_env_ignore_malformed branch from 96a9627 to c910006 Compare November 16, 2015 01:01

tbu- force-pushed the pr_env_ignore_malformed branch from c910006 to 2917cd4 Compare November 16, 2015 01:10

tbu- force-pushed the pr_env_ignore_malformed branch from 2917cd4 to 87243bc Compare November 16, 2015 23:32

bors added a commit that referenced this pull request Nov 17, 2015

Auto merge of #29297 - tbu-:pr_env_ignore_malformed, r=alexcrichton

a7644b3

Otherwise, the iterator and the functions for getting specific environment variables might disagree, for environments like FOOBAR

bors merged commit 87243bc into rust-lang:master Nov 17, 2015

tbu- mentioned this pull request Nov 17, 2015

Ignore malformed environment variables on Windows too #29901

Merged

This was referenced Nov 18, 2015

Revert not-ignoring weird environment variables for MinGW #29910

Merged

MinGW's env::vars_os and env::var_os disagree #29911

Closed

bors added a commit that referenced this pull request Nov 19, 2015

Auto merge of #29901 - tbu-:pr_env_ignore_malformed_windows, r=alexcr…

6861c51

…ichton See also #29297.

Ignore malformed environment strings like glibc does #29297

Ignore malformed environment strings like glibc does #29297

Uh oh!

Conversation

tbu- commented Oct 25, 2015

Uh oh!

rust-highfive commented Oct 25, 2015

Uh oh!

tbu- commented Oct 25, 2015

Uh oh!

alexcrichton Oct 26, 2015

Choose a reason for hiding this comment

Uh oh!

apasel422 Oct 26, 2015

Choose a reason for hiding this comment

Uh oh!

tbu- Oct 26, 2015

Choose a reason for hiding this comment

Uh oh!

alexcrichton Oct 26, 2015

Choose a reason for hiding this comment

Uh oh!

alexcrichton commented Oct 26, 2015

Uh oh!

tbu- commented Oct 26, 2015

Uh oh!

tbu- commented Oct 27, 2015

Uh oh!

eddyb Oct 27, 2015

Choose a reason for hiding this comment

Uh oh!

eddyb commented Oct 27, 2015

Uh oh!

alexcrichton commented Oct 27, 2015

Uh oh!

tbu- commented Nov 5, 2015

Uh oh!

alexcrichton commented Nov 5, 2015

Uh oh!

bors commented Nov 5, 2015

Uh oh!

bors commented Nov 6, 2015

Uh oh!

tbu- commented Nov 6, 2015

Uh oh!

eddyb commented Nov 6, 2015

Uh oh!

alexcrichton commented Nov 6, 2015

Uh oh!

tbu- commented Nov 6, 2015

Uh oh!

alexcrichton commented Nov 6, 2015

Uh oh!

bors commented Nov 6, 2015

Uh oh!

bors commented Nov 6, 2015

Uh oh!

tbu- commented Nov 6, 2015

Uh oh!

alexcrichton commented Nov 6, 2015

Uh oh!

tbu- commented Nov 14, 2015

Uh oh!

alexcrichton commented Nov 16, 2015

Uh oh!

tbu- commented Nov 16, 2015

Uh oh!

alexcrichton commented Nov 16, 2015

Uh oh!

tbu- commented Nov 16, 2015

Uh oh!

alexcrichton commented Nov 16, 2015

Uh oh!

bors commented Nov 16, 2015

Uh oh!

bors commented Nov 16, 2015

Uh oh!

bors commented Nov 16, 2015

Uh oh!

tbu- commented Nov 16, 2015