[LibOS] Remove migrated_envp
#781
Conversation
Signed-off-by: Dmitrii Kuvaiskii <dmitrii.kuvaiskii@intel.com>
There was a problem hiding this comment.
Reviewable status: 0 of 4 files reviewed, all discussions resolved, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel)
libos/src/sys/libos_exec.c line 141 at r1 (raw file):
/* * TODO: Linux treats argv == NULL and envp == NULL as `argv = {NULL}` and `envp = {NULL}`. * Gramine currently doesn't implement this corner case.
FYI: I didn't want to introduce a fix for this in this PR. Though the fix should be rather trivial.
I wonder if any real-world app relies on this non-standard behavior...
libos/src/sys/libos_exec.c line 165 at r1 (raw file):
* * See https://github.com/gramineproject/graphene/issues/2081 for details. */
FYI: I honestly don't think we can/should do any better than this... If the user removes LD_LIBRARY_PATH from the envvars, then the user is on her own... Re-adding LD_LIBRARY_PATH sounds totally wrong.
There was a problem hiding this comment.
Reviewable status: 0 of 4 files reviewed, all discussions resolved, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel)
libos/src/sys/libos_exec.c line 165 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
FYI: I honestly don't think we can/should do any better than this... If the user removes
LD_LIBRARY_PATHfrom the envvars, then the user is on her own... Re-addingLD_LIBRARY_PATHsounds totally wrong.
What about apps which uses clearenv(3)?
There was a problem hiding this comment.
Reviewable status: 0 of 4 files reviewed, all discussions resolved, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel)
libos/src/sys/libos_exec.c line 165 at r1 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
What about apps which uses
clearenv(3)?
@oshogbo Do any apps actually do it? I guess some do... Yeah, maybe this "your-app-is-stupid" fix is wrong. How should we fix it then?
There was a problem hiding this comment.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv and @oshogbo)
a discussion (no related file):
Any volunteer to finally fix gramineproject/graphene#2081 ? Should be rather simple :)
libos/src/sys/libos_exec.c line 141 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
FYI: I didn't want to introduce a fix for this in this PR. Though the fix should be rather trivial.
I wonder if any real-world app relies on this non-standard behavior...
Could you create an issue for this? Seems easy to fix, so it will be good for new developers to pick up.
libos/src/sys/libos_exec.c line 165 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
@oshogbo Do any apps actually do it? I guess some do... Yeah, maybe this "your-app-is-stupid" fix is wrong. How should we fix it then?
But why not just implement the execve logic identically to Linux? (i.e. to fall back to some hardcoded dirs when it's missing)
libos/test/regression/sigprocmask_pending.c line 149 at r1 (raw file):
char ld_library_path_envvar[512]; strcpy(ld_library_path_envvar, "LD_LIBRARY_PATH="); strcat(ld_library_path_envvar, ld_library_path_value);
I don't like this, I know it's just a test, but ld_library_path_value is externally controlled and may corrupt memory on some configurations.
There was a problem hiding this comment.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv and @mkow)
libos/src/sys/libos_exec.c line 165 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
But why not just implement the
execvelogic identically to Linux? (i.e. to fall back to some hardcoded dirs when it's missing)
I think you got the problem wrong. There is no logic in execve that is related to this env var. It's just ld.so that has some defaults.
There are three (potential) problems:
- (I believe we've fixed this already) mounting our patched libc at some path that is default in
ld.so- we do it in/libso I guess it's already ok. Update: well, see the point 3, it's not ok. - Clash of libcs. There could be host glibc mounted at different path - we must make sure that our glibc is prioritized.
- Wrong paths. When we build glibc and ld.so, we forward libdir and prefix from meson. These paths are then hardcoded as defaults in
ld.sosearch algorithm - in most testing cases (like CI) these are some custom paths like/home/user/gramine/installand are not re-created in Gramine virtual FS, sold.sois not able to find the glibc.
libos/test/regression/sigprocmask_pending.c line 149 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I don't like this, I know it's just a test, but
ld_library_path_valueis externally controlled and may corrupt memory on some configurations.
+1, just use strdup. Or even better: execve -> execv. This test doesn't depend on passing empty env. We should add such test once we fix other issues.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv and @mkow)
a discussion (no related file):
All non sgx pipelines failed
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv)
libos/src/sys/libos_exec.c line 165 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
I think you got the problem wrong. There is no logic in
execvethat is related to this env var. It's justld.sothat has some defaults.
There are three (potential) problems:
- (I believe we've fixed this already) mounting our patched libc at some path that is default in
ld.so- we do it in/libso I guess it's already ok. Update: well, see the point 3, it's not ok.- Clash of libcs. There could be host glibc mounted at different path - we must make sure that our glibc is prioritized.
- Wrong paths. When we build glibc and ld.so, we forward libdir and prefix from meson. These paths are then hardcoded as defaults in
ld.sosearch algorithm - in most testing cases (like CI) these are some custom paths like/home/user/gramine/installand are not re-created in Gramine virtual FS, sold.sois not able to find the glibc.
Ok, seems I was fooled by this comment saying that Linux does this :P
3. sounds quite bad, and I'm not sure how to fix it...
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv)
libos/src/sys/libos_exec.c line 165 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Ok, seems I was fooled by this comment saying that Linux does this :P
3. sounds quite bad, and I'm not sure how to fix it...
We should stop using meson configured prefix and use our own hardcoded one (basically change prefix of these paths).
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv)
libos/src/sys/libos_exec.c line 165 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
We should stop using meson configured prefix and use our own hardcoded one (basically change prefix of these paths).
@boryspoplawski Where did you get this information about point 3? I don't see any hard-coded paths in our ld.so (I did strings on it on my gramine/build-debug/blabla directory, so I would expect to see this hard-coded path).
Looking at these:
And observing what I have on my local Ubuntu 20.04, I can see that the actual problematic paths are under /etc/ld.so.conf. That's what I have:
$ cat /etc/ld.so.conf
include /etc/ld.so.conf.d/*.conf
$ cat /etc/ld.so.conf.d/*.conf
/usr/lib/x86_64-linux-gnu/libfakeroot
# libc default configuration
/usr/local/lib
# Multiarch support
/usr/local/lib/x86_64-linux-gnu
/lib/x86_64-linux-gnu
/usr/lib/x86_64-linux-gnu
# Legacy biarch compatibility support
/lib32
/usr/lib32
# Legacy biarch compatibility support
/libx32
/usr/libx32
But in Gramine, we typically do not allow these /etc/*.conf files. So we can't really recreate the ld's logic of "I will search for libs in these dirs"...
There was a problem hiding this comment.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv)
libos/src/sys/libos_exec.c line 165 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
@boryspoplawski Where did you get this information about point 3? I don't see any hard-coded paths in our
ld.so(I didstringson it on mygramine/build-debug/blabladirectory, so I would expect to see this hard-coded path).Looking at these:
And observing what I have on my local Ubuntu 20.04, I can see that the actual problematic paths are under
/etc/ld.so.conf. That's what I have:$ cat /etc/ld.so.conf include /etc/ld.so.conf.d/*.conf $ cat /etc/ld.so.conf.d/*.conf /usr/lib/x86_64-linux-gnu/libfakeroot # libc default configuration /usr/local/lib # Multiarch support /usr/local/lib/x86_64-linux-gnu /lib/x86_64-linux-gnu /usr/lib/x86_64-linux-gnu # Legacy biarch compatibility support /lib32 /usr/lib32 # Legacy biarch compatibility support /libx32 /usr/libx32But in Gramine, we typically do not allow these
/etc/*.conffiles. So we can't really recreate theld's logic of "I will search for libs in these dirs"...
What was --prefix to meson when you build it? If it was default then you wouldn't see it - that what happens when you install Gramine systemwide.
Steps to reproduce:
- Configure meson with custom installation prefix
- Build Gramine
- Build regression tests and their manifests
- Remove
LD_LIBRARY_PATHfrombootstrap.manifest, addalllog level - Observe the paths where the program looks for
libc.soand fails to find it.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @boryspoplawski, @dimakuv, and @oshogbo)
libos/src/sys/libos_exec.c line 165 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
What was
--prefixto meson when you build it? If it was default then you wouldn't see it - that what happens when you install Gramine systemwide.Steps to reproduce:
- Configure meson with custom installation prefix
- Build Gramine
- Build regression tests and their manifests
- Remove
LD_LIBRARY_PATHfrombootstrap.manifest, addalllog level- Observe the paths where the program looks for
libc.soand fails to find it.
I build with a local-dir prefix (build-debug/). But I think you're talking about a different point...
My point is -- if we want to emulate the exact host behavior, then we need to do something about /etc/*.conf files.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @boryspoplawski, @dimakuv, @mkow, and @oshogbo)
libos/src/sys/libos_exec.c line 141 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Could you create an issue for this? Seems easy to fix, so it will be good for new developers to pick up.
Done #794
There was a problem hiding this comment.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv and @mkow)
libos/src/sys/libos_exec.c line 165 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I build with a local-dir prefix (
build-debug/). But I think you're talking about a different point...My point is -- if we want to emulate the exact host behavior, then we need to do something about
/etc/*.conffiles.
We don't, they are just extensions. The problem is if you mount them from host (e.g. mount whole /etc) - you should not do that.
|
This PR went nowhere. I'll close it for now since it has very low priority. |
Description of the changes
This variable is wrong and shouldn't be used.
But also see gramineproject/graphene#2081 and the TODOs in this PR.
How to test this PR?
CI is enough. I modified
sigprocmask_pendingLibOS regression test, otherwise (ifenvp = {NULL}) it failed with:This change is