bubblewrap icon indicating copy to clipboard operation
bubblewrap copied to clipboard

Add get_current_dir_name from musl to fix building on Bionic libc

Open orowith2os opened this issue 2 years ago • 7 comments

get_current_dir_name is a GNU function, and is not supported on all C libraries. Bionic is the main one here, as musl provides the get_current_dir_name we're using here.

We mostly copy-paste the musl implementation in order to fix building on the Bionic libc. Bubblewrap will now build and run successfully on Android.

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

Closes https://github.com/containers/bubblewrap/issues/579

orowith2os avatar May 04 '23 17:05 orowith2os

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.

smcv avatar May 04 '23 18:05 smcv

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?

smcv avatar May 04 '23 18:05 smcv

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.

orowith2os avatar May 04 '23 18:05 orowith2os

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.

orowith2os avatar May 04 '23 18:05 orowith2os

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.

orowith2os avatar May 04 '23 18:05 orowith2os

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

orowith2os avatar May 12 '23 01:05 orowith2os

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.)

smcv avatar May 12 '23 11:05 smcv