Spurious "filename too long" failure on bots #33844

Activity

1. alexcrichton

   added

   O-windowsOperating system: WindowsOperating system: Windows

   A-spuriousArea: Spurious failures in builds (spuriously == for no apparent reason)Area: Spurious failures in builds (spuriously == for no apparent reason)

   on May 24, 2016
2. retep998 commented on May 26, 2016

   

   retep998

   on May 26, 2016

   Contributor

   More actions

   This error can occur when the gcc command is over 32,767 characters. It would be nice if there was some way to check the length of the lpCommandLine that is passed to CreateProcessW when spawning a Command so a better diagnostic can be emitted. At the very least print out the linker arguments when that error code occurs.
3. alexcrichton commented on May 26, 2016

   

   alexcrichton

   on May 26, 2016

   MemberAuthor

   More actions

   Failure appears to always be the auto-win-gnu-32-opt-rustbuild builder so far, probably related to how cargo passes a few extra arguments.
4. xen0n commented on Jun 1, 2016

   

   xen0n

   on Jun 1, 2016

   Contributor

   More actions

   Hi everyone, what about moving to the so-called response files? (stackoverflow, msdn, gcc) I programmed in Windows not so long ago and it's how Visual Studio has always been doing to workaround the argv limit. Apart from having to deal with exactly one temp file it'll solve the problem once and for all. Maybe I could even take this and produce a patch.

   Opinions?
5. retep998 commented on Jun 1, 2016

   

   retep998

   on Jun 1, 2016

   Contributor

   More actions

   The correct link for link.exe is https://msdn.microsoft.com/en-us/library/9xch38h8.aspx
   This is definitely a valid approach to take, although we really should investigate why the command line is so long in the first place to see if there is something accidentally inflating it.
6. xen0n commented on Jun 1, 2016

   

   xen0n

   on Jun 1, 2016

   Contributor

   More actions

   @retep998 That link was simply copy'n'pasted from the SO link, thanks for the correction. I agree that the problematic invocations should be investigated first because it's only occasionally showing up.
7. alexcrichton commented on Jun 3, 2016

   

   alexcrichton

   on Jun 3, 2016

   MemberAuthor

   More actions

   The captured instance of this shows the command line as not very long, so my guess is that it's actually related to the environment variable block. This can also be only up to 32K.

   The compiler will attempt to update the PATH environment variable when loading dylibs in a fashion that with concurrent tests in rustdoc it's possible for PATH to just keep growing indefinitely. That way once you start hitting the final tests you'll probably start seeing failures with the environment block too big.

   Specifically, I suspect that this block needs to be changed. I believe we've run into this before in #17327 which led to #17360, so I'm gonna close this in favor of #17360.
8. alexcrichton

   closed this as completedon Jun 3, 2016
9. vadimcn commented on Jun 3, 2016

   

   vadimcn

   on Jun 3, 2016

   Contributor

   More actions

   The compiler will attempt to update the PATH environment variable when loading dylibs in a fashion that with concurrent tests in rustdoc it's possible for PATH to just keep growing indefinitely.

   I am kinda skeptical of this. It'd take a lot of threads entering this block at the same time to add the PATH up to 32k.
   As far as I understand, the main danger of raciness in this code is that another thread may restore the original value of PATH between the current thread adding to it and calling LoadLibrary. But in that case the error would be something along the lines of "DLL not found".

   Also, if PATH is growing too long, wouldn't the error be happening in SetEnvironmentVariable rather than in CreateProcess?
10. retep998 commented on Jun 3, 2016

    

    retep998

    on Jun 3, 2016

    Last edited by retep998

    Contributor

    More actions

    @vadimcn The error is happening as a result of Command::output which means potentially any system function called inside there, not necessarily CreateProcessW.
11. retep998 commented on Jun 3, 2016

    

    retep998

    on Jun 3, 2016

    Last edited by retep998

    Contributor

    More actions

    Also, on the matter of the environment block being too big...

    The ANSI version of this function, CreateProcessA fails if the total size of the environment block for the process exceeds 32,767 characters. MSDN

    Since we're calling CreateProcessW and not CreateProcessA, that shouldn't be the issue, probably. Then again MSDN could be lying to us. Or maybe with the unicode version while there is no limit on the block itself, there could still be a limit on the individual environment variables themselves. If so, the proper solution is to get rid of the raciness and just thread PATH through somehow.
12. xen0n commented on Jun 3, 2016

    

    xen0n

    on Jun 3, 2016

    Last edited by xen0n

    Contributor

    More actions

    @retep998 I just confirmed a limit of ~32K on at least individual environment variables indeed exists:

    use std::process::{Command, Output};
    
    
    fn test(n: usize) -> std::io::Result<Output> {
        // just about anything works, I simply used a hello world
        let mut c = Command::new("hello.exe");
    
        let path = {
            let mut parts: Vec<u8> = Vec::with_capacity(n + 3);  // "C:\\AAA...A"
            parts.push(67);
            parts.push(58);
            parts.push(92);
            for i in 0..n {
                parts.push(65);
            }
            // parts.push(0);
            String::from_utf8(parts).unwrap()
        };
        c.env("PATH", path);
    
        c.output()
    }
    
    
    fn main() {
        let mut l = 1usize;
        let mut r = 1usize;
    
        loop {
            let ret = test(r);
            if let Err(_) = ret {
                break;
            }
    
            l = r;
            r = 2 * r;
        }
    
        loop {
            let mid = (l + r) / 2;
            let ret = test(mid);
            println!("left={} right={} mid={} ({:?})", l, r, mid, ret);
            if let Err(_) = ret {
                r = mid;
            } else {
                l = mid;
            }
            if r - l <= 1 {
                break;
            }
        }
    }

    This gives a value close to 32K on my Windows 7 VM. So MSDN actually is lying 😺 In fact they're simply not being accurate there, as all the ANSI variants of Windows API are just shims for the wide ones. EDIT: sorry this is just testing only one variable, but it's already sufficient. I tested again with separate variables of constant length, and the program was happily eating all my memory. So the limitation should be on sizes of individual variables.
13. alexcrichton commented on Jun 5, 2016

    

    alexcrichton

    on Jun 5, 2016

    MemberAuthor

    More actions

    @vadimcn it looks like so far it's almost always been doc tests in libcore that are failing (that's just from memory, not verified). The most recent failure indicates that libcore has 954 tests which means that so long as each test adds ~30 bytes to PATH it'll overflow eventually.

    In our case get_dylib_search_paths will probably return paths looking like:

    C:\bot\slave\auto-win-gnu-32-opt-rustbuild\build\obj\build\x86_64-pc-windows-msvc\stage1\lib\rustlib\x86_64-pc-windows-msvc\lib
    C:\bot\slave\auto-win-gnu-32-opt-rustbuild\build\obj\build\x86_64-pc-windows-msvc\stage1-std\release\x86_64-pc-windows-msvc
    C:\bot\slave\auto-win-gnu-32-opt-rustbuild\build\obj\build\x86_64-pc-windows-msvc\stage1-std\release\x86_64-pc-windows-msvc\deps
    

    All of those together are 381 bytes which means of our 954 tests only about 84 have to fail this race (< 10%). The race here, that is, where because threads just append to PATH they append to an already lengthened PATH, and then restore the longer path (causing further threads to lengthen it).

    We don't actually call SetEnvironmentVariable I believe, this is all just threaded through to CreateProcessW, and @xen0n's investigation seems to confirm that super long values will cause explosions.

    Plus, this diagnosis fits the bill of the symptoms so far:

    • We know for sure it's not the command line.
    • It only happens in rustdoc tests, which are the only ones exercising this race
    • It rarely happens externally of our own build system, but it's also very rare to find a crate with 954 doc tests always tested on Windows
    • This happens sporadically, so it seems racy
    • The error coincides with the theory
    • After one test fails all others start failing as well as each new append would keep hitting the same limit.
14. alexcrichton commented on Jun 5, 2016

    

    alexcrichton

    on Jun 5, 2016

    MemberAuthor

    More actions

    Ah and this also only happens on WIndows, which is the only spot with that logic active.
15. xen0n commented on Jun 5, 2016

    

    xen0n

    on Jun 5, 2016

    Contributor

    More actions

    Did further investigation and confirmed:

    • Other environment variables are not affected, only PATH.
    • The size limit is on the length of value, not on individual path entries. Even an overlong series of C:\;C:\;C:\; ... C:\ is triggering the error.

    I'm not bothering to check implementation of CreateProcessW (in ReactOS though) for certainty, as it seems this is the only problem.
16. alexcrichton

    added a commit that references this issue on Jun 5, 2016

    rustc: Try to contain prepends to PATH

    ...

    1564e92
17. bors

    added a commit that references this issue on Jun 8, 2016

    Auto merge of #34083 - alexcrichton:dumb-hack, r=nrc

    ...

    4b240fe