Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add get_current_dir_name from musl to fix building on Bionic libc #580

Closed
wants to merge 2 commits into from
Closed

Add get_current_dir_name from musl to fix building on Bionic libc #580

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
81064d2
Add get_current_dir_name from musl to fix building on Bionic libc
orowith2os May 4, 2023
47cf000
Forgot to comment the license
orowith2os May 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions bubblewrap.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,14 @@
__result; }))
#endif

/* Bionic doesnt include this C function, so
* we pull it from the musl source tree for use here.
* Should be possible to add more checks for other libcs
* when needed. */
#ifdef __BIONIC__
#include "get_current_dir_name.c"
#endif

/* We limit the size of a tmpfs to half the architecture's address space,
* to avoid hitting arbitrary limits in the kernel.
* For example, on at least one x86_64 machine, the actual limit seems to be
Expand Down
37 changes: 37 additions & 0 deletions get_current_dir_name.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/* Copyright © 2005-2020 Rich Felker, et al., 2023 Dallas Strouse

Permission is hereby granted, free of charge, to any person obtaining
a copy of this software and associated documentation files (the
"Software"), to deal in the Software without restriction, including
without limitation the rights to use, copy, modify, merge, publish,
distribute, sublicense, and/or sell copies of the Software, and to
permit persons to whom the Software is furnished to do so, subject to
the following conditions:

The above copyright notice and this permission notice shall be
included in all copies or substantial portions of the Software.

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.
IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY
CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
*/

#include <stdlib.h>
#include <string.h>
#include <limits.h>
#include <unistd.h>
#include <sys/stat.h>

static char *get_current_dir_name(void) {
struct stat a, b;
char *res = getenv("PWD");
if (res && *res && !stat(res, &a) && !stat(".", &b)
&& (a.st_dev == b.st_dev) && (a.st_ino == b.st_ino))
return strdup(res);
return getcwd(0, 0);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This relies on a behaviour of getcwd() that is documented to be a GNU extension. getcwd(3) on my (GNU/)Linux system says:

[glibc's implementation of] getcwd() conforms to POSIX.1-2001. Note however that POSIX.1-2001 leaves the behavior of getcwd() unspecified if buf is NULL.

meaning that it's considered perfectly OK under POSIX for getcwd (NULL, 0) to crash, or even to be a security vulnerability. On GNU systems (and presumably also musl and bionic systems), as an extension, getcwd (NULL, 0) is documented to allocate a large enough buffer as if via malloc(), to be freed by the caller - but there is nothing to stop someone from deciding that musl and bionic are "too bloated" and writing a new libc that doesn't implement that GNU extension, in which case bubblewrap's behaviour would become undefined if we used this code on that libc.

Undefined behaviour in bubblewrap is scary, because it's sometimes setuid root, making it security-sensitive.

I see you're using this under #ifdef __BIONIC__ rather than #ifndef HAVE_GET_CURRENT_DIR_NAME, which means that as long as Bionic documents the GNU-like getcwd(NULL, 0) extension, it's safe to use. Does it document that behaviour?

I think that's subtle enough to deserve comments. Perhaps above the __BIONIC__ check, something like:

intentionally not using a HAVE_GET_CURRENT_DIR_NAME check, because our implementation relies on an extension that is documented by bionic libc but not guaranteed by POSIX

(except with appropriate line wrapping); and above this call to getcwd(0, 0),

Not guaranteed by POSIX, but bionic libc documents that if the first parameter here is null, it will allocate a new buffer as if via malloc()

(assuming that's true of course).

Copy link
Author

@orowith2os orowith2os May 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the Bionic implementation: https://android.googlesource.com/platform/bionic/+/master/libc/bionic/getcwd.cpp

I don't know much about the details of these, I'm mainly a Rust gal, and don't touch C often if at all. If needed, I'll add more comments.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the Bionic implementation: https://android.googlesource.com/platform/bionic/+/master/libc/bionic/getcwd.cpp

I see it implements the GNU-style extension, but does it document the extension (or a general API stability policy) so that it would be considered to be an incompatible change to remove it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A quick Google search doesn't turn anything up, I'll search some more and ask some Android devs if they can comment on it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading the following files, I think it's safe to assume they won't change getcwd on us, as they have it listed (and i assume supported because of that) on some of their docs. Can't find any specific details on getcwd though. Would the source code be enough to reference?

https://android.googlesource.com/platform/bionic/+/ics-mr1-release/libc/docs/CHANGES.TXT
https://android.googlesource.com/platform/bionic/+/refs/heads/master/docs/status.md

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I don't see anything by following those links that documents that their getcwd (NULL, 0) is intended to have GNU-compatible semantics.

}