Argument-passing-related cleanup #1428
Conversation
|
Jenkins, retest Jenkins-18.04 please (weird timeout on nginx: |
|
Jenkins, retest Jenkins-Debug-18.04 please (weird timeout on nginx: |
There was a problem hiding this comment.
Reviewable status: 0 of 21 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "DO NOT MERGE" found in commit messages' one-liners (waiting on @mkow)
a discussion (no related file):
We debugged the Nginx failure offline (on Ubuntu 18.04), and I came up with this patch: #1434. @mkow Please apply the patch and try again, it should work.
|
Jenkins, retest Jenkins-Debug-18.04 please ( |
There was a problem hiding this comment.
Btw, flock() system call is not implemented in Graphene. It's curious that we have an LTP test flock06 running :) (I looked at it, and the only thing that is checked in our tests is that the second call to this syscall with non-existing file fails with any error -- which is ENOSYS in Graphene case). This is just an observation, not related to this PR.
Reviewed 18 of 18 files at r1, 1 of 1 files at r2, 2 of 2 files at r3, 2 of 2 files at r4.
Reviewable status: all files reviewed, 10 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "DO NOT MERGE" found in commit messages' one-liners (waiting on @mkow)
a discussion (no related file):
This should be rebased on the current master since the Nginx fix is merged now
Jenkinsfiles/Linux-18.04, line 108 at r3 (raw file):
make make start-graphene-server & sleep 5
Not needed now
Jenkinsfiles/Linux-Debug-18.04, line 104 at r3 (raw file):
make make start-graphene-server & sleep 5
Not needed now
Pal/src/host/Linux/db_main.c, line 264 at r1 (raw file):
if (!first_process) { // Children receive their argv and config via IPC. int parent_pipe_fd = atoi(argv[2]);
Do we want to put some guards here? E.g., that parent_pipe_fd >= 0?
Pal/src/host/Linux/db_main.c, line 281 at r1 (raw file):
if (first_process) { // We need to find a binary to run. const char* exec_target = argv[2];
Do we want to put some guards here? E.g., that strnlen(exec_target, PATH_MAX) < PATH_MAX?
Pal/src/host/Linux/db_main.c, line 288 at r1 (raw file):
snprintf(uri, size, URI_PREFIX_FILE "%s", exec_target); PAL_HANDLE file; // `options` should be `PAL_OPTION_CLOEXEC`, but _DkStreamOpen is totally broken.
Shouldn't you add FIXME: prefix?
Pal/src/host/Linux/pal_linux.h, line 200 at r1 (raw file):
} PAL_TCB_LINUX; int pal_thread_init(void * tcbptr);
void* tcbptr
Pal/src/host/Linux-SGX/sgx_main.c, line 972 at r1 (raw file):
/* We're one of the children spawned to host new processes started inside Graphene. * We'll receive our argv and config via IPC. */ int parent_pipe_fd = atoi(argv[2]);
ditto
Pal/src/host/Linux-SGX/sgx_main.c, line 1075 at r1 (raw file):
size_t args_size; if (first_process) { args = argv[2];
ditto
Pal/src/host/Linux-SGX/sgx_main.c, line 1078 at r1 (raw file):
args_size = argc > 2 ? (argv[argc - 1] - args) + strlen(argv[argc - 1]) + 1 : 0; } else { args = argv[3];
ditto
There was a problem hiding this comment.
Btw, flock() system call is not implemented in Graphene. It's curious that we have an LTP test flock06 running :)
lol :)
Reviewable status: 19 of 21 files reviewed, 10 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
We debugged the Nginx failure offline (on Ubuntu 18.04), and I came up with this patch: #1434. @mkow Please apply the patch and try again, it should work.
I cherry-picked the commit and now it passed the tests. Will drop it on the final rebase. Thanks for debugging this!
Jenkinsfiles/Linux-18.04, line 108 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Not needed now
will drop this during rebase
Pal/src/host/Linux/db_main.c, line 264 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Do we want to put some guards here? E.g., that
parent_pipe_fd >= 0?
IMO doesn't make sense because currently we don't have any number conversion function which provides error checking. We'd need to implement something more sane than atoi(), but that's for another PR.
Pal/src/host/Linux/db_main.c, line 281 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Do we want to put some guards here? E.g., that
strnlen(exec_target, PATH_MAX) < PATH_MAX?
Not sure if it's worth checking here, something underneath should fail on access. I mean, such check should rather belong to the filesystem layer, not the general loading code.
Pal/src/host/Linux/db_main.c, line 288 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Shouldn't you add
FIXME:prefix?
Done.
Pal/src/host/Linux/pal_linux.h, line 200 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
void* tcbptr
Done.
Pal/src/host/Linux-SGX/sgx_main.c, line 972 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto
ditto
Pal/src/host/Linux-SGX/sgx_main.c, line 1075 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto
ditto
Pal/src/host/Linux-SGX/sgx_main.c, line 1078 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto
ditto
There was a problem hiding this comment.
Reviewed 2 of 2 files at r5.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), "fixup! " found in commit messages' one-liners (waiting on @mkow)
This property was already implicitly assumed in a few callsites. We'd be better to document & assert it.
This is a preparation for even more clean-ups and bugfixes, which are required by protected argv+envp implementation.
There was a problem hiding this comment.
Reviewed 2 of 20 files at r6, 2 of 2 files at r7, 1 of 5 files at r8, 4 of 5 files at r9, 11 of 11 files at r10.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required) (waiting on @mkow)
There was a problem hiding this comment.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This should be rebased on the current master since the Nginx fix is merged now
Done.
|
Jenkins, retest Jenkins-Debug please ( |
There was a problem hiding this comment.
Reviewed 1 of 18 files at r1, 2 of 2 files at r7, 1 of 5 files at r8, 4 of 5 files at r9, 11 of 11 files at r10.
Reviewable status:complete! all files reviewed, all discussions resolved
Description of the changes
While preparing protected argv/env PR it turned out that everything around argv passing is in a terrible state. This is the first PR in a series which will try to untangle this mess.
The biggest change in this PR is refactoring main functions and interfaces of PAL binaries. Now they take an explicit argument informing them in which mode they start (initial process vs child) and in the latter case, fd number with a pipe to the parent. Previously this differentiation was done by checking a magic fd number.
Reviewing commit-by-commit is recommended.
How to test this PR?
Jenkins.
This change is