sys::unix::fs::canonicalize can lead to undefined-ish behavior on Android

[glandium]

Its code is:

    let path = CString::new(p.as_os_str().as_bytes())?;
    let buf;
    unsafe {
        let r = libc::realpath(path.as_ptr(), ptr::null_mut());
        if r.is_null() {
            return Err(io::Error::last_os_error())
        }
        buf = CStr::from_ptr(r).to_bytes().to_vec();
        libc::free(r as *mut _);
    }
    Ok(PathBuf::from(OsString::from_vec(buf)))
}

The problem comes from the pair of calls libc::realpath, libc::free. Under normal conditions, libc::realpath calls malloc, libc::free corresponds to free and everyone lives happily every after.

Now, funky things are funky. Say you have a binary that makes malloc and free be jemalloc. From rust, libc::malloc and libc::free then point to jemalloc. But here's the catch: libc's malloc is not necessarily jemalloc's. That is, while on e.g. Linux, when you have such a setup, libc will happily use jemalloc's malloc/free for its allocations (as long as it's not built with -Bsymbolic or -Bsymbolic-functions), that's not the case on Android: IIRC, libc is linked in such a way that all its "internal" function calls stay internal (that is, it's built with -Bsymbolic). So when realpath calls malloc, it calls libc's not jemalloc's. And then canonicalize calls libc::free with the resulting pointer, which is jemalloc's. Kaboom.

Yes, there is __malloc_hook and friends, which would solve the problem if they existed on Android. (Well, they do now, but they don't on Android versions < 9 (and apparently 7 for IoT if I'm to believe tags in the bionic repository).

https://bugzilla.mozilla.org/show_bug.cgi?id=1531887 is a real issue that results from this, although it doesn't involve -Bsymbolic, but some subtle dynamic linking setup where rust code is in libxul.so, which depends on libmozglue.so, which provides malloc/free as jemalloc's, for use by libxul, but all native Android code still ends up using libc's malloc/free.

For this particular case, it's possible to pass an appropriately-sized buffer to realpath instead of null, which makes it allocate on its own (https://pubs.opengroup.org/onlinepubs/009696799/functions/realpath.html says PATH_MAX ; the only system I know that doesn't use those limits is Hurd). For the more general case, it almost feels like there should be an overridable GlobalAlloc for the non-rust system allocator.

Cc: @SimonSapin

[glandium]

FWIW, I pulled libc.so from my android phone, and its realpath does call malloc through the PLT, so the problem may not appear if you try, but I've certainly, in the past, seen libc.so's from android devices that didn't go through the PLT for internal calls.

[SimonSapin]

As far as I understand this issue is not about sys::unix::fs::canonicalize or even about Rust. The same would happen in a C program that configures jemalloc to provide malloc and free symbols, right? To me this sounds like a bug in jemalloc and/or that libc.

Or are you saying that using the free symbol to deallocate memory allocated inside of libc is never correct, and instead users (including in C or C++) should only ever pass pre-allocated buffers? Is realpath the only libc API that is affected?

[ids1024]

The same would happen in a C program that configures jemalloc to provide malloc and free symbols, right?

I suppose this raises the question: does the C code in Firefox carefully avoid doing things like this?

To me this sounds like a bug in jemalloc and/or that libc.

I don't know if it can be considered a bug in jemalloc, since there may simply be nothing it can do. Nor in bionic, since I don't think anything in the C standard (or probably other standards) would imply it's possible to replace the allocator this way.

Someone more familar with jemalloc and linker wizardry might have a better idea, but perhaps the conclusion is just that it's wrong to use an alternate malloc/free implementation with Android?

Searching for "jemalloc Android", apparently since Android 7, jemalloc is actually used by default.

One thing that comes to mind: wouldn't it in principle be possible to use an allocator like jemalloc for allocation in Rust without overriding malloc and free? I don't know if jemalloc provides a way to be used that way.

[glandium]

The same would happen in a C program that configures jemalloc to provide malloc and free symbols, right?

Right. But only in some configurations. Firefox has an even weirder configuration than the cases where it might happen without all the Firefox weirdness.

To me this sounds like a bug in jemalloc and/or that libc.

And it is, to some extent, but there's also no escape hatch provided by rust.

One thing that comes to mind: wouldn't it in principle be possible to use an allocator like jemalloc for allocation in Rust without overriding malloc and free? I don't know if jemalloc provides a way to be used that way.

It's possible, but that wouldn't solve the problem, because the it's not a problem with rust allocations. It's a problem with rust libstd deallocation of a buffer that was allocated by libc. At least it's using libc::free, and not rust's allocator, which would be another layer of problems.

One part of the problem is that all this is statically linked, and linkers don't allow to redirect symbols differently depending on where they're used from. And even if I were to locally modify the libc crate so that its free does something else, that wouldn't change what libstd uses.

[steveklabnik]

Triage: not aware of any changes