SGX's std::env::args function is not thread-safe

[KamilaBorowska]

The args method is defined as follows.

pub fn args() -> Args {
    let args = unsafe { (ARGS.load(Ordering::Relaxed) as *const ArgsStore).as_ref() };
    if let Some(args) = args {
        Args(args.iter())
    } else {
        Args([].iter())
    }
}

Clean-up function is defined as follows;

pub unsafe fn cleanup() {
    let args = ARGS.swap(0, Ordering::Relaxed);
    if args != 0 {
        drop(Box::<ArgsStore>::from_raw(args as _))
    }
}

It is possible for another thread to use std::env::args() while the main thread quits, and access already freed memory - assuming the following sequence of events.

// Secondary thread
let args = unsafe { (ARGS.load(Ordering::Relaxed) as *const ArgsStore).as_ref() };
// Main thread
{
    let args = ARGS.swap(0, Ordering::Relaxed);
    if args != 0 {
        drop(Box::<ArgsStore>::from_raw(args as _))
    }
}
// Secondary thread
if let Some(args) = args {
    Args(args.iter())
}

This issue has been assigned to @Goirad via this comment.

[jonas-schievink]

cc @jethrogb

(do we want an O-fortanix-sgx label?)

[jethrogb]

I suppose we could just turn cleanup into a NOP. How do other platforms handle this?

[Goirad]

@rustbot claim

[Goirad]

Looking at other implementations, the only times when this is not a NOP is for hermit vxcode and unix on non-apple variants. In all these cases the code is:

pub unsafe fn cleanup() {
    let _guard = LOCK.lock();
    ARGC = 0;
    ARGV = ptr::null();
}

I am inclined to just change it to a NOP. What was the motivation for the existing logic?

[jethrogb]

What was the motivation for the existing logic?

Implement the required function cleanup without knowing exactly what needs to happen.

Having done some git archeology, it looks to me like the cleanup function exists solely for certain platforms where the arguments may be stored on the stack of the main thread. If the main thread exits, that stack may no longer be available so other threads may not reference it.

I think turning it into a nop for SGX is probably fine.