Note that Android currently prohibits user namespaces and suid programs, so this needs root access to function.

I think this probably means that making bubblewrap build successfully on Android is not actually practically useful? Sorry, I'm not really willing to pay an ongoing maintenance cost for portability to a platform where bubblewrap can't perform its stated purpose.

The second commit is missing a Signed-off-by, but if this was going to be merged, the two commits should be squashed into one anyway.

I would be inclined to close this without merging if the resulting bubblewrap isn't practically useful on Android/Bionic systems, because the cost (we have more code in the tree, which we'll become responsible for, forever) seems like it exceeds the benefit (if you have root, you can ... do things you would already be able to do by just calling C functions).

However, perhaps there's a reason why this is practically useful, that I'm not seeing?

I think this probably means that making bubblewrap build successfully on Android is not actually practically useful?

If Android ever decides to enable one or both of the features, it'll work ootb. It's also worth noting that Bionic is not at all Android-only, that's just where it's most commonly used. I want to tinker with Android some, so having this here would be very nice. I'd also like to see Android move away from using different users entirely for each application as it does right now, this could be a step towards that.

The second commit is missing a Signed-off-by, but if this was going to be merged, the two commits should be squashed into one anyway.

I tried 🥹 it'll probably be signed off on in the squashed commit anyways.

One more add-on: Android does have suid binaries, they're just very limited (iirc only su is suid). Bubblewrap could be shipped on Android alongside su as a suid binary.

The current limitation seems to come from the fact that Android disables suid binaries on some filesystem mounts.

As a general comment, I'm not a fan of "minimal" "non-bloated" libc implementations that achieve their minimalism by not providing straightforward, useful extension functions like get_current_dir_name(),

I'd like to note that Bionic is intended for use in systems with memory constraints, so a libc like you mentioned here would be a good fit. Most users would be using it indirectly anyways, since most Android apps are made using Kotlin or Java and don't need to concern themselves with libc. It's mainly just a system library concern.

@smcv thoughts on the patch applied by Termux? https://github.com/termux/termux-packages/blob/master/root-packages/bubblewrap/swap-getcwd-func.patch

thoughts on the patch applied by Termux?

This has the same problem as what you proposed here: instead of relying on a non-standardized function with known behaviour (GNU get_current_dir_name), it's relying on non-standardized extension behaviour of a standardized function.

If get_current_dir_name exists, we can be fairly confident that it has the GNU behaviour, because there would be no reasonable way to implement its API other than malloc'ing and returning a new buffer.

If getcwd exists, POSIX.1-2001 specifies what getcwd(a_nonnull_pointer, its_length) should do, but leaves the behaviour of getcwd(NULL, .) unspecified: it might behave like GNU get_current_dir_name (as it is documented to do in glibc, and as it apparently does in Bionic), or it might try to dereference the null pointer and crash, or it might fail an assertion, or anything else.

If there is documentation somewhere that says Bionic (or other C libraries) won't remove this extension in a future release, then the best implementation might be something like this:

#if defined(__BIONIC__) /* || defined(__some_other_c_library__) || ... */
char *get_current_dir_name (void)
{
  /* POSIX leaves the behaviour of getcwd(NULL, .) unspecified,
  * but these C libraries implement the common extension that
  * getcwd(NULL, .) allocates a buffer with malloc. */
  return getcwd (NULL, 0);
}

Or if the getcwd extension isn't documented as something we can rely on, then the only portable thing to do is to allocate our own buffer of some reasonable size, getcwd() into it, and if getcwd() fails with ERANGE, keep allocating a larger buffer until it either succeeds or reports a different error. That would end up as basically a copy/paste of g_get_current_dir() from GLib, except without the Windows code path, and using Standard C malloc, types, etc. instead of their GLib equivalents.

(In more or less any other codebase I'd just use g_get_current_dir(), but because bubblewrap is sometimes setuid root, it needs to have minimal dependencies, so we have to do everything with one hand tied behind our collective backs.)

If the Android community wants to have bubblewrap available on Bionic (or, more generally, if users of other non-GNU libc implementations want to have bubblewrap available on their platforms), I think the best route would probably be to have a library that provides useful GNU extensions like get_current_dir_name(), and make bubblewrap depend on that library on those platforms. Unfortunately, I'm not aware of such a thing existing at the moment.

Analogous: libbsd provides a compatibility layer allowing software designed for *BSD to be compiled and run on GNU systems, by implementing missing BSD functions. Android could have an analogous library that allows software designed for GNU to be compiled and run on Android systems, if the Android community wants to provide one.

That would also set expectations for security support: if there was an exploitable security vulnerability resulting from a function in the GNU-compatibility shim library either containing vulnerable code or not being fully compatible with glibc, then it would be unambiguously a bug in that library, and not a bug in bubblewrap.

The other option here would be (as I said in a previous comment) to allocate our own buffer of some reasonable size, getcwd() into it, and if getcwd() fails with ERANGE, keep allocating a larger buffer until it either succeeds or reports a different error. If a contributor is interested in pursuing that, please open a separate merge request.

I am not happy with making bubblewrap depend on undocumented behaviour of non-GNU C libraries (in this case, the fact that Bionic getcwd (NULL, 0) apparently implements the GNU extension of allocating a new buffer), because bubblewrap is security-sensitive, and security vulnerabilities can easily soak up a lot of maintainer time that we don't really have. So I'm closing this, sorry.