Support running scripts as the entrypoint

[aneessahib]

This will enable users to run scripts in Gramine (as opposed to manually providing a ELF entrypoint - this can be quite an inconvenience to users). This will also minimize the need to create wrapper docker files for entrypoint manipulation while using GSC.

Fixes #320

---

This change is 

[dimakuv]

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @aneessahib)

a discussion (no related file):
This is a new feature, and it needs at least one (preferrably 2 or 3) different tests, with manifests containinglibos.entrypoint = "script.sh".

---

---

libos/include/libos_process.h line 73 at r1 (raw file):

int init_process(void);
int init_process_args(int argc, const char** argv);

I suggest to rename to init_process_cmdline(), because this is the only thing that this function does -- it initializes /proc/self/cmdline based on arguments.

---

libos/src/libos_init.c line 431 at r1 (raw file):

        log_error("libos_init: failed to load exec: %d", ret);
        PalProcessExit(1);
    }

You should wrap it in RUN_INIT, smth like this:

    struct libos_handle* exec = NULL;
    char** new_argv = NULL;
    RUN_INIT(load_and_check_exec, argv[0], argv, &exec, &new_argv);

Actually, can you create a new helper function init_process_args() (note that the old init_process_args() should be renamed to init_process_cmdline(), as per my other comment) and put this call to load_and_check_exec() in that new function? This way it will be much cleaner and easy to understand.

Also, you have a dummy exec handle which should be put_handle() at the end of that new function, otherwise there will be a memory leak. I would also rename the variable to dummy_exec.

---

libos/src/bookkeep/libos_handle.c line 114 at r1 (raw file):

    }

    struct libos_handle* exec_handle = NULL;

Please add a comment before this snippet:

/* entrypoint may be a shebang script instead of the ELF binary,  get the actual ELF binary name in this case */

---

libos/src/bookkeep/libos_handle.c line 115 at r1 (raw file):

    struct libos_handle* exec_handle = NULL;
    char** new_argv = NULL;

new_argv -> dummy_new_argv (you're not using these arguments)

---

libos/src/bookkeep/libos_handle.c line 116 at r1 (raw file):

    struct libos_handle* exec_handle = NULL;
    char** new_argv = NULL;
    const char* argv[] = {exec_path, NULL};

argv -> dummy_argv (you're not using these arguments)

---

libos/src/bookkeep/libos_handle.c line 117 at r1 (raw file):

    char** new_argv = NULL;
    const char* argv[] = {exec_path, NULL};
    ret = load_and_check_exec(argv[0], argv, &exec_handle, &new_argv);

argv[0] -> exec_path (semantically equivalent, but easier to read)

---

libos/src/bookkeep/libos_handle.c line 122 at r1 (raw file):

    }

    hdl = get_new_handle();

This is already done inside load_and_check_exec(). The created-and-opened handle is returned by that function in exec_handle, so you could just use that one.

---

libos/src/bookkeep/libos_handle.c line 129 at r1 (raw file):

    const char* new_exec_path = new_argv[0];
    ret = open_executable(hdl, new_exec_path);

This is already done inside load_and_check_exec(). The created-and-opened handle is returned by that function in exec_handle, so you could just use that one.

So what I propose is to remove this snippet with get_new_handle() and open_executable() and simply have:

    char** dummy_new_argv = NULL;
    const char* dummy_argv[] = {exec_path, NULL};
    ret = load_and_check_exec(exec_path, dummy_argv, &hdl, &dummy_new_argv);
    if (ret < 0) {
        log_error("Error opening %s: %d", exec_path, ret);
        goto out;
    }

    free(*dummy_new_argv);
    free(dummy_new_argv);

---

libos/src/bookkeep/libos_process.c line 66 at r1 (raw file):

     * They are not separated by space, but by NUL instead. So, it is essential to maintain the
     * cmdline_size also as a member here. */

Please remove this empty line.

[aneessahib]

Reviewable status: 0 of 6 files reviewed, 10 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @aneessahib and @dimakuv)

---

libos/include/libos_process.h line 73 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I suggest to rename to init_process_cmdline(), because this is the only thing that this function does -- it initializes /proc/self/cmdline based on arguments.

Done.

---

libos/src/libos_init.c line 431 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

You should wrap it in RUN_INIT, smth like this:

    struct libos_handle* exec = NULL;
    char** new_argv = NULL;
    RUN_INIT(load_and_check_exec, argv[0], argv, &exec, &new_argv);

Actually, can you create a new helper function init_process_args() (note that the old init_process_args() should be renamed to init_process_cmdline(), as per my other comment) and put this call to load_and_check_exec() in that new function? This way it will be much cleaner and easy to understand.

Also, you have a dummy exec handle which should be put_handle() at the end of that new function, otherwise there will be a memory leak. I would also rename the variable to dummy_exec.

Done.

---

libos/src/bookkeep/libos_handle.c line 114 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please add a comment before this snippet:

/* entrypoint may be a shebang script instead of the ELF binary,  get the actual ELF binary name in this case */

Done.

---

libos/src/bookkeep/libos_handle.c line 115 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

new_argv -> dummy_new_argv (you're not using these arguments)

Done.

---

libos/src/bookkeep/libos_handle.c line 116 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

argv -> dummy_argv (you're not using these arguments)

Done.

---

libos/src/bookkeep/libos_handle.c line 117 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

argv[0] -> exec_path (semantically equivalent, but easier to read)

Done.

---

libos/src/bookkeep/libos_handle.c line 122 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This is already done inside load_and_check_exec(). The created-and-opened handle is returned by that function in exec_handle, so you could just use that one.

Done.

---

libos/src/bookkeep/libos_handle.c line 129 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This is already done inside load_and_check_exec(). The created-and-opened handle is returned by that function in exec_handle, so you could just use that one.

So what I propose is to remove this snippet with get_new_handle() and open_executable() and simply have:

    char** dummy_new_argv = NULL;
    const char* dummy_argv[] = {exec_path, NULL};
    ret = load_and_check_exec(exec_path, dummy_argv, &hdl, &dummy_new_argv);
    if (ret < 0) {
        log_error("Error opening %s: %d", exec_path, ret);
        goto out;
    }

    free(*dummy_new_argv);
    free(dummy_new_argv);

Done.

---

libos/src/bookkeep/libos_process.c line 66 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please remove this empty line.

Done.

[aneessahib]

Reviewable status: 0 of 6 files reviewed, 11 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @aneessahib and @dimakuv)

---

libos/src/sys/libos_exec.c line 173 at r5 (raw file):

        file, argv[0]);
    }
    const char* orig_argv[] = {file, argv[1] ? argv[1] : NULL, NULL};

Typically argv[0] and file would be the same. As in, an application normally passes argv[0]  to pathname while calling execve . Occasionally I see this not being met - and mostly in cases where the binary is present in one of the $PATH locations. for eg - execve ("/usr/local/bin/baz.sh", ["baz.sh", .. ], .. )

In such cases when Gramine obtains the interpreter for /usr/local/bin/baz.h by the reading the shebang line (which lets say is /bin/sh ) , but what gets passed to it is just baz.sh and not the full the path, resulting in Gramine not finding baz.sh

[dimakuv]

Reviewed 4 of 6 files at r3, 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @aneessahib)

---

libos/include/libos_process.h line 74 at r6 (raw file):

int init_process(void);
int init_process_cmdline(int argc, const char** argv);
int init_process_args(const char* path, const char** argv, char*** out_new_argv);

There is no need for the path argument -- you are passing argv[0] here anyway, so just use argv[0] intead of path in this func. And remove the path argument.

---

libos/src/libos_init.c line 426 at r6 (raw file):

    RUN_INIT(init_mount);

    char** new_argv = NULL;

In this function, we have now new_argv and new_argp. This is very confusing.

I suggest to rename the newly added new_argv to expanded_argv.

---

libos/src/libos_rtld.c line 697 at r6 (raw file):

    size_t curr_argv_bytes = 0, curr_argv_cnt = 0;
    for (const char** a = argv; *a; a++) {
        curr_argv_bytes += strlen(curr_argv_cnt == 0? path: *a) + 1;

Why is this change needed? What happens if you revert this change?

I highly dislike this change. Something is wrong here, and should be fixed at another level, not here.

---

libos/src/libos_rtld.c line 716 at r6 (raw file):

    for (const char** a = argv; *a; a++) {
        size_t size = strlen(curr_argv_idx == 0? path: *a) + 1;
        memcpy(curr_argv_ptr, curr_argv_idx == 0? path: *a, size);

ditto

---

libos/src/bookkeep/libos_handle.c line 114 at r6 (raw file):

    }

    /* entrypoint may be a shebang script instead of the ELF binary,  get the actual ELF binary name in this case */

Please remove a double-space after comma

---

libos/src/bookkeep/libos_process.c line 63 at r6 (raw file):

int init_process_args(const char* path, const char** argv, char*** out_new_argv) {

Please remove empty line.

---

libos/src/bookkeep/libos_process.c line 67 at r6 (raw file):

    int ret = load_and_check_exec(path, argv, &dummy_exec, out_new_argv);
    if (ret < 0) {
        log_error("libos_init: failed to load exec:%s  ret %d", argv[0], ret);

I see no need in this error message. The caller of init_process_args() will print some error message anyway. Just remove this line.

---

libos/src/bookkeep/libos_process.c line 68 at r6 (raw file):

    if (ret < 0) {
        log_error("libos_init: failed to load exec:%s  ret %d", argv[0], ret);
        PalProcessExit(1);

This is wrong. You need to return ret.

---

libos/src/sys/libos_exec.c line 173 at r5 (raw file):

Previously, aneessahib (Anees Sahib) wrote…

Typically argv[0] and file would be the same. As in, an application normally passes argv[0]  to pathname while calling execve . Occasionally I see this not being met - and mostly in cases where the binary is present in one of the $PATH locations. for eg - execve ("/usr/local/bin/baz.sh", ["baz.sh", .. ], .. )

In such cases when Gramine obtains the interpreter for /usr/local/bin/baz.h by the reading the shebang line (which lets say is /bin/sh ) , but what gets passed to it is just baz.sh and not the full the path, resulting in Gramine not finding baz.sh

I don't get the explanation. This will need a more detailed flow that breaks.

Anyway, I have two issues with this:

1. How is this related to this PR (to "running scripts as the entrypoint")? This seems to deserve a separate PR.
2. Why the need for this debug message? It doesn't change anything in Gramine execution, so how does it help in anything?

Bottom line: let's remove this code snippet from this PR, and create a new PR. This code snippet is very suspicious, and we'll need several rounds of discussions before it come to some conclusion. So by removing this snippet from this PR, we can concentrate on the core of the PR and merge it faster.

UPDATE: Hm, now I realize that you added this snippet because you also added a strange fix inside load_and_check_exec() (see my comments there). I believe both these changes have nothing to do with your current PR and should be discussed in another PR.

[aneessahib]

Reviewable status: 2 of 6 files reviewed, 10 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @aneessahib and @dimakuv)

---

libos/include/libos_process.h line 74 at r6 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

There is no need for the path argument -- you are passing argv[0] here anyway, so just use argv[0] intead of path in this func. And remove the path argument.

Done.

---

libos/src/libos_init.c line 426 at r6 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

In this function, we have now new_argv and new_argp. This is very confusing.

I suggest to rename the newly added new_argv to expanded_argv.

Done.

---

libos/src/bookkeep/libos_handle.c line 114 at r6 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please remove a double-space after comma

Done.

---

libos/src/bookkeep/libos_process.c line 63 at r6 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please remove empty line.

Done.

---

libos/src/bookkeep/libos_process.c line 67 at r6 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I see no need in this error message. The caller of init_process_args() will print some error message anyway. Just remove this line.

Done.

---

libos/src/bookkeep/libos_process.c line 68 at r6 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This is wrong. You need to return ret.

Done.

---

libos/src/sys/libos_exec.c line 173 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I don't get the explanation. This will need a more detailed flow that breaks.

Anyway, I have two issues with this:

1. How is this related to this PR (to "running scripts as the entrypoint")? This seems to deserve a separate PR.
2. Why the need for this debug message? It doesn't change anything in Gramine execution, so how does it help in anything?

Bottom line: let's remove this code snippet from this PR, and create a new PR. This code snippet is very suspicious, and we'll need several rounds of discussions before it come to some conclusion. So by removing this snippet from this PR, we can concentrate on the core of the PR and merge it faster.

UPDATE: Hm, now I realize that you added this snippet because you also added a strange fix inside load_and_check_exec() (see my comments there). I believe both these changes have nothing to do with your current PR and should be discussed in another PR.

The need for this change (or some fix) arises only when executing scripts, and not with elf, which is why I chose to include it here. I'll create another PR just to address this.

[aneessahib]

Reviewable status: 1 of 6 files reviewed, 10 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @aneessahib and @dimakuv)

---

libos/src/libos_rtld.c line 697 at r6 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why is this change needed? What happens if you revert this change?

I highly dislike this change. Something is wrong here, and should be fixed at another level, not here.

Per our separate discussion - submitted another PR with this change

---

libos/src/libos_rtld.c line 716 at r6 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

ditto

Per our separate discussion - submitted another PR with this change #754

[dimakuv]

Reviewed 4 of 4 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @aneessahib)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This is a new feature, and it needs at least one (preferrably 2 or 3) different tests, with manifests containinglibos.entrypoint = "script.sh".

This will need to be done.

---

---

libos/src/libos_init.c line 428 at r8 (raw file):

    char** expanded_argv = NULL;
    RUN_INIT(init_process_args, argv, &expanded_argv);
    RUN_INIT(init_process_cmdline, argc, (const char**)expanded_argv);

Wait, actually, does it make sense? Your new expanded_argv may have a different number of arguments now. But you're still using the old argc value.

I think you need to modify init_process_cmdline() implementation to remove this argc argument completely -- we can calculate the number of arguments from argv itself.

---

libos/src/libos_init.c line 429 at r8 (raw file):

    RUN_INIT(init_process_args, argv, &expanded_argv);
    RUN_INIT(init_process_cmdline, argc, (const char**)expanded_argv);
    RUN_INIT(init_threading);

Can you add an empty line before this init_threading() invocation? To visually separate "work on args" and "work on other stuff".

---

libos/src/libos_init.c line 439 at r8 (raw file):

    const char** new_argp;
    elf_auxv_t* new_auxv;

Please remove this empty line. These two variable declarations naturally "belong" to the init_stack() invocation.

---

libos/src/sys/libos_exec.c line 173 at r5 (raw file):

Previously, aneessahib (Anees Sahib) wrote…

The need for this change (or some fix) arises only when executing scripts, and not with elf, which is why I chose to include it here. I'll create another PR just to address this.

Please revert this code snippet here. I see no useful information to users when they see Unusual anomaly... message. Also, it's not unusual, now we now that this is how execve() works -- it is written in the man pages (and somewhere in the POSIX standard).

[aneessahib]

Reviewable status: 2 of 6 files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @aneessahib and @dimakuv)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This will need to be done.

Working on it. Tests framework seem to be designed around test executables written in C. So this will be a first (I think) to run scripts as the entrypoint.

---

---

libos/src/libos_init.c line 428 at r8 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Wait, actually, does it make sense? Your new expanded_argv may have a different number of arguments now. But you're still using the old argc value.

I think you need to modify init_process_cmdline() implementation to remove this argc argument completely -- we can calculate the number of arguments from argv itself.

Done. But note that now you don't use this argument at all in the entire libos_init function.

---

libos/src/libos_init.c line 429 at r8 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Can you add an empty line before this init_threading() invocation? To visually separate "work on args" and "work on other stuff".

Done.

---

libos/src/libos_init.c line 439 at r8 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please remove this empty line. These two variable declarations naturally "belong" to the init_stack() invocation.

Done.

---

libos/src/sys/libos_exec.c line 173 at r5 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Please revert this code snippet here. I see no useful information to users when they see Unusual anomaly... message. Also, it's not unusual, now we now that this is how execve() works -- it is written in the man pages (and somewhere in the POSIX standard).

Done.

[dimakuv]

Reviewed 4 of 4 files at r9, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @aneessahib)

a discussion (no related file):

Previously, aneessahib (Anees Sahib) wrote…

Working on it. Tests framework seem to be designed around test executables written in C. So this will be a first (I think) to run scripts as the entrypoint.

The tests framework shouldn't care about scripts. I mean, you can just create a script file execscript.sh in libos/test/regression/ directory and create a manifest file execscript.manifest.template that will contain (among other things):

libos.entrypoint = "execscript.sh"

Then you add your execscript manifest entry here:

gramine/libos/test/regression/tests.toml

Line 3 in e5cbd15

manifests = [

and here:

gramine/libos/test/regression/tests_musl.toml

Line 5 in e5cbd15

manifests = [

And you add a new test somewhere here:

gramine/libos/test/regression/test_libos.py

Line 224 in e5cbd15

That should be it. No need to modify the framework, just add required files.

---

---

libos/src/libos_init.c line 428 at r8 (raw file):

Previously, aneessahib (Anees Sahib) wrote…

Done. But note that now you don't use this argument at all in the entire libos_init function.

True, but that's for another PR. The current UNUSED(argc) is good.

[aneessahib]

Reviewable status: 6 of 11 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @aneessahib and @dimakuv)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

The tests framework shouldn't care about scripts. I mean, you can just create a script file execscript.sh in libos/test/regression/ directory and create a manifest file execscript.manifest.template that will contain (among other things):

libos.entrypoint = "execscript.sh"

Then you add your execscript manifest entry here:

gramine/libos/test/regression/tests.toml

Line 3 in e5cbd15

manifests = [

and here:

gramine/libos/test/regression/tests_musl.toml

Line 5 in e5cbd15

manifests = [

And you add a new test somewhere here:

gramine/libos/test/regression/test_libos.py

Line 224 in e5cbd15

That should be it. No need to modify the framework, just add required files.

Done. I misunderstood that the test must be a part of meson too. Btw - I named the test as run_script (too much similarity between execscript and exec_script)

---

[aneessahib]

Reviewable status: 6 of 11 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)

---

libos/src/libos_init.c line 428 at r8 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

True, but that's for another PR. The current UNUSED(argc) is good.

Done.

[mkow]

Reviewed 1 of 1 files at r8, 3 of 4 files at r9, 5 of 5 files at r10, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @aneessahib and @dimakuv)

a discussion (no related file):

aneessahib:nonelf_entrypoint

Please read https://gramine.readthedocs.io/en/latest/devel/contributing.html#branch-names (and actually this whole document).

---

---

libos/src/libos_init.c line 429 at r10 (raw file):

    char** expanded_argv = NULL;
    RUN_INIT(init_process_args, argv, &expanded_argv);
    RUN_INIT(init_process_cmdline, (const char**)expanded_argv);

Why can't you change the type of expanded_argv instead? (and all functions using it)

Code quote:

(const char**)expanded_argv

---

libos/src/bookkeep/libos_handle.c line 114 at r10 (raw file):

    }

    /* entrypoint may be a shebang script instead of the ELF binary, get the actual ELF binary name in this case */

line too long

---

libos/src/bookkeep/libos_handle.c line 114 at r10 (raw file):

    }

    /* entrypoint may be a shebang script instead of the ELF binary, get the actual ELF binary name in this case */

Suggestion:

an ELF binary

---

libos/test/regression/run_script.sh line 1 at r10 (raw file):

#!  /bin/sh

Please name this script differently, so that it's visible that it's a test (not some utility script for running tests, as it may sound now), e.g. prefix it with shebang_test_.

---

libos/test/regression/run_script.manifest.template line 3 at r10 (raw file):

loader.entrypoint = "file:{{ gramine.libos }}"
libos.entrypoint = "run_script.sh"
loader.argv0_override = "run_script.sh"

Why is this needed?

---

libos/test/regression/run_script.manifest.template line 25 at r10 (raw file):

  "file:/usr{{ arch_libdir }}/libstdc++.so.6",
  "file:{{ binary_dir }}/{{ entrypoint }}",
  "file:{{ binary_dir }}/exec_victim",

What's this? And half of the things above?

---

libos/test/regression/test_libos.py line 225 at r10 (raw file):

            'scripts/foo.sh STRING FROM EXECVE', stdout)

    @unittest.skipIf(USES_MUSL, 'Test uses /bin/sh from the host which is built against Glibc')

Suggestion:

which is usually built against glibc

[dimakuv]

Reviewed 4 of 5 files at r10, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @aneessahib and @mkow)

---

libos/src/libos_init.c line 429 at r10 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Why can't you change the type of expanded_argv instead? (and all functions using it)

@mkow I don't get your comment. If changing anything, then Anees should change either one of those:

• either change the type of expanded_argv to const char** (but then also typecast it in init_process_args()? I'm not sure about it, probably not needed),
• or change the argument types in init_process_cmdline() and init_stack() to accept char** instead of const char**

I would prefer the first option.

---

libos/test/regression/run_script.sh line 1 at r10 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Please name this script differently, so that it's visible that it's a test (not some utility script for running tests, as it may sound now), e.g. prefix it with shebang_test_.

+1, I like shebang_test_script.sh. This file and all corresponding files (the manifest).

---

libos/test/regression/run_script.manifest.template line 3 at r10 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Why is this needed?

I have a feeling we need this line. Otherwise we'll end up with argc == 0, and this is not how Linux starts programs.

Alternatively, we could have loader.insecure__use_cmdline_argv.

---

libos/test/regression/run_script.manifest.template line 25 at r10 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

What's this? And half of the things above?

+1, please remove all the unnecessary lines in this file

---

libos/test/regression/test_libos.py line 225 at r10 (raw file):

            'scripts/foo.sh STRING FROM EXECVE', stdout)

    @unittest.skipIf(USES_MUSL, 'Test uses /bin/sh from the host which is built against Glibc')

When resolving according to @mkow's comment, please also fix it in the test above.

---

libos/test/regression/tests_musl.toml line 87 at r10 (raw file):

  "readdir",
  "rename_unlink",
  "run_script",

You don't need to update this file since you're only testing for Glibc, and skipping for Musl.

[mkow]

Reviewable status: all files reviewed, 9 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @aneessahib and @dimakuv)

---

libos/src/libos_init.c line 429 at r10 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

@mkow I don't get your comment. If changing anything, then Anees should change either one of those:

• either change the type of expanded_argv to const char** (but then also typecast it in init_process_args()? I'm not sure about it, probably not needed),
• or change the argument types in init_process_cmdline() and init_stack() to accept char** instead of const char**

I would prefer the first option.

But why init_process_args() can't receive a compatible argument? I.e. why we need any casts here?

---

libos/test/regression/run_script.manifest.template line 3 at r10 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I have a feeling we need this line. Otherwise we'll end up with argc == 0, and this is not how Linux starts programs.

Alternatively, we could have loader.insecure__use_cmdline_argv.

Wait, why argc would be 0? If it would, isn't this just a bug in this PR?

[dimakuv]

Reviewable status: all files reviewed, 9 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @aneessahib and @boryspoplawski)

---

libos/test/regression/run_script.manifest.template line 3 at r10 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Wait, why argc would be 0? If it would, isn't this just a bug in this PR?

Do you mean that Gramine should correctly handle the case where the manifest doesn't specify any argv-related options (i.e., argc == 0)?

Yeah, sounds like you're right. Gramine should detect this case and put the libos.entrypoint string as the first and only argv. I don't know if this is actually supported in Gramine currently. Or maybe I'm incorrectly understanding how Linux starts programs? @boryspoplawski

[mkow]

Reviewable status: all files reviewed, 9 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @aneessahib and @boryspoplawski)

---

libos/test/regression/run_script.manifest.template line 3 at r10 (raw file):

Do you mean that Gramine should correctly handle the case where the manifest doesn't specify any argv-related options (i.e., argc == 0)?

Yes, although it's not argc == 0 then - it's just not specified anywhere.

Or maybe I'm incorrectly understanding how Linux starts programs?

This is unrelated to how Linux starts programs. Linux always gets requests to start processes from execve where you always have argv specified. The only similar case is init process, where arguments are (I think) taken from some config file / boot options (like in our case).

[boryspoplawski]

Reviewed 1 of 6 files at r3.
Reviewable status: all files reviewed, 9 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @aneessahib and @mkow)

---

libos/test/regression/run_script.manifest.template line 3 at r10 (raw file):
I think argc == 0 should work in Graimne, but argv == NULL won't. We probably need to fix it at some point, but that was never a priority, since nothing really uses it.

from execve where you always have argv specified.

Not true, you can have argv == NULL.

Wait, why argc would be 0? If it would, isn't this just a bug in this PR?

Why wouldn't it be? If we did not specify any arguments anywhere...
I think argv0_override was introduced exactly due to this - not to have argc == 0 (bascially poor-mans inline in manifest argv), but I might remember it wrong.

Gramine should detect this case and put the libos.entrypoint string as the first and only argv. I don't know if this is actually supported in Gramine currently. Or maybe I'm incorrectly understanding how Linux starts programs? @boryspoplawski

Not sure what you mean. As @mkow pointed out, all args always come from execve

[dimakuv]

Reviewable status: all files reviewed, 9 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @aneessahib and @mkow)

---

libos/test/regression/run_script.manifest.template line 3 at r10 (raw file):

Not sure what you mean. As @mkow pointed out, all args always come from execve

But for the first process, Gramine should do some magic; similar to how Linux does some magic for the init process. I mean, there is no execve for the first process under Gramine. I'm talking solely about this case.

[boryspoplawski]

Reviewable status: all files reviewed, 9 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @aneessahib and @mkow)

---

libos/test/regression/run_script.manifest.template line 3 at r10 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Not sure what you mean. As @mkow pointed out, all args always come from execve

But for the first process, Gramine should do some magic; similar to how Linux does some magic for the init process. I mean, there is no execve for the first process under Gramine. I'm talking solely about this case.

Yes, but then it's not "how Linux starts programs", just something more specific to Gramine.
Putting entrypoint as argv0 sounds good to me

[dimakuv]

Reviewable status: all files reviewed, 9 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @aneessahib and @mkow)

---

libos/test/regression/run_script.manifest.template line 3 at r10 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Yes, but then it's not "how Linux starts programs", just something more specific to Gramine.
Putting entrypoint as argv0 sounds good to me

Yes, agreed, my initial statement was wrong. I'd be fine with argv0 as it is currently (or with loader.args = ["entrypoint"] once it is done). @mkow ?

[mkow]

Reviewable status: all files reviewed, 9 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @aneessahib, @boryspoplawski, and @dimakuv)

---

libos/test/regression/run_script.manifest.template line 3 at r10 (raw file):

from execve where you always have argv specified.

Not true, you can have argv == NULL.

Ok, I rather meant that you have clearly specified what should process receive.

Wait, why argc would be 0? If it would, isn't this just a bug in this PR?

Why wouldn't it be? If we did not specify any arguments anywhere...

"we did not specify any arguments anywhere" - exactly, so argc/argv is just unspecified, not "specified as empty" ;) The user just didn't configure this at all in this case.

I think argv0_override was introduced exactly due to this - not to have argc == 0 (bascially poor-mans inline in manifest argv), but I might remember it wrong.

argv0_override is super old (although it had a different name before), AFAIR even older than the support to block host argv from being passed through.

So, summarizing: in this particular case we don't have argv specified anywhere, so we need to pick some default. And IMO argv = [entrypoint] is a good one.

[dimakuv]

Reviewable status: all files reviewed, 9 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @aneessahib and @mkow)

---

libos/src/libos_init.c line 429 at r10 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

But why init_process_args() can't receive a compatible argument? I.e. why we need any casts here?

Maybe we don't need any casts. @aneessahib Could you change:

char** expanded_argv = NULL;

to

const char** expanded_argv = NULL;

And then update the init_process_args() function definition to accept the compatible type in the second argument?

---

libos/test/regression/run_script.manifest.template line 3 at r10 (raw file):

So, summarizing: in this particular case we don't have argv specified anywhere, so we need to pick some default. And IMO argv = [entrypoint] is a good one.

I agree with this. But didn't we derail the conversation? The PR is called "Support running scripts as the entrypoint", and what we're discussing here is a different feature. We should make a new PR for it.

[aneessahib]

Reviewable status: 2 of 11 files reviewed, 9 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)

a discussion (no related file):

Previously, mkow (Michał Kowalczyk) wrote…

aneessahib:nonelf_entrypoint

Please read https://gramine.readthedocs.io/en/latest/devel/contributing.html#branch-names (and actually this whole document).

Ok - will it mess with the ongoing PR if I rename branch? I'll take care of this in future PRs if that's ok.

---

---

libos/src/libos_init.c line 429 at r10 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Maybe we don't need any casts. @aneessahib Could you change:

char** expanded_argv = NULL;

to

const char** expanded_argv = NULL;

And then update the init_process_args() function definition to accept the compatible type in the second argument?

Does this look ok now? I thought Michal's concern is the cast that we do here (which is actually needed). i.e use char** as the type all through init_process_cmdline

---

libos/src/bookkeep/libos_handle.c line 114 at r10 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

line too long

Done.

---

libos/src/bookkeep/libos_handle.c line 114 at r10 (raw file):

    }

    /* entrypoint may be a shebang script instead of the ELF binary, get the actual ELF binary name in this case */

Done.

---

libos/test/regression/test_libos.py line 225 at r10 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

When resolving according to @mkow's comment, please also fix it in the test above.

Done.

---

libos/test/regression/tests_musl.toml line 87 at r10 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

You don't need to update this file since you're only testing for Glibc, and skipping for Musl.

Done.

---

libos/test/regression/run_script.sh line 1 at r10 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

+1, I like shebang_test_script.sh. This file and all corresponding files (the manifest).

Done.

---

libos/test/regression/run_script.manifest.template line 25 at r10 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

+1, please remove all the unnecessary lines in this file

Done.

[aneessahib]

Reviewable status: 2 of 11 files reviewed, 9 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)

---

libos/src/libos_init.c line 429 at r10 (raw file):

Previously, aneessahib (Anees Sahib) wrote…

Does this look ok now? I thought Michal's concern is the cast that we do here (which is actually needed). i.e use char** as the type all through init_process_cmdline

typo - actually not needed

[dimakuv]

Reviewed 8 of 9 files at r11, all commit messages.
Reviewable status: 10 of 11 files reviewed, 13 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @aneessahib and @mkow)

a discussion (no related file):

Previously, aneessahib (Anees Sahib) wrote…

Ok - will it mess with the ongoing PR if I rename branch? I'll take care of this in future PRs if that's ok.

@aneessahib Yep, please don't rename this branch. Just keep it in mind for the future, that you need not only a name-explanation for the branch (like nonelf_entrypoint) but also your nickname as a prefix (so it should have been aneessahib/nonelf_entrypoint).

---

---

libos/include/libos_process.h line 73 at r11 (raw file):

int init_process(void);
int init_process_cmdline(char** argv);

This is a wrong change, please keep const

---

libos/src/libos_init.c line 429 at r10 (raw file):

Previously, aneessahib (Anees Sahib) wrote…

typo - actually not needed

No, this change looks wrong. init_process_cmdline() should take const char** as input. This basically tells the reader of this code and the compiler that "this argument is an input argument, the strings in this list are constant". Now you removed const, and the meaning changed -- now it is "this argument is an input/output argument, maybe this function will change the strings in this list".

So you should actually change expanded_argv variable to be const char**.

---

libos/src/libos_init.c line 441 at r11 (raw file):

    const char** new_argp;
    elf_auxv_t* new_auxv;
    RUN_INIT(init_stack, (const char**)expanded_argv, envp, &new_argp, &new_auxv);

@aneessahib Also note that you have this typecast here.

---

libos/src/bookkeep/libos_process.c line 75 at r11 (raw file):

}

int init_process_cmdline(char** argv) {

This is a wrong change, please keep const

---

libos/src/bookkeep/libos_process.c line 83 at r11 (raw file):

    size_t tmp_size = 0;

    for (char** a = argv; *a; a++) {

This is a wrong change, please keep const

---

libos/test/regression/test_libos.py line 217 at r11 (raw file):

        self.assertIn('execve(invalid-envp) correctly returned error', stdout)

    @unittest.skipIf(USES_MUSL, 'Test uses /bin/sh from the host which is built against Glibc')

You forgot to change this test as well.

[aneessahib]

Reviewable status: 10 of 11 files reviewed, 13 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @aneessahib, @dimakuv, and @mkow)

---

libos/src/libos_init.c line 429 at r10 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

No, this change looks wrong. init_process_cmdline() should take const char** as input. This basically tells the reader of this code and the compiler that "this argument is an input argument, the strings in this list are constant". Now you removed const, and the meaning changed -- now it is "this argument is an input/output argument, maybe this function will change the strings in this list".

So you should actually change expanded_argv variable to be const char**.

Yes i get that. Its just that we will run into this casting issue somewhere down the line. Read my other comment on init_stack

---

libos/src/libos_init.c line 441 at r11 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

@aneessahib Also note that you have this typecast here.

Yep - This is similar to what we are doing here.

gramine/libos/src/sys/libos_exec.c

Line 125 in cf8095d

ret = init_stack((const char**)argv, envp, &new_argp, &new_auxv);

So to fix this whole casting issue - we are going to have to get to what we have in TODO.

[dimakuv]

Reviewable status: 10 of 11 files reviewed, 13 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @aneessahib, @boryspoplawski, @dimakuv, and @mkow)

---

libos/src/libos_init.c line 429 at r10 (raw file):

Previously, aneessahib (Anees Sahib) wrote…

Yes i get that. Its just that we will run into this casting issue somewhere down the line. Read my other comment on init_stack

Hm. I'll let @mkow and @boryspoplawski decide on the correct type for expanded_argv and the function signatures. I'm unsure how they would like it to be fixed.

[aneessahib]

Reviewable status: 9 of 11 files reviewed, 13 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @aneessahib, @boryspoplawski, @dimakuv, and @mkow)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

@aneessahib Yep, please don't rename this branch. Just keep it in mind for the future, that you need not only a name-explanation for the branch (like nonelf_entrypoint) but also your nickname as a prefix (so it should have been aneessahib/nonelf_entrypoint).

sure will do

---

---

libos/test/regression/test_libos.py line 217 at r11 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

You forgot to change this test as well.

Done.

[dimakuv]

Reviewed 1 of 1 files at r12, all commit messages.
Reviewable status: 10 of 11 files reviewed, 12 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @aneessahib, @boryspoplawski, @dimakuv, and @mkow)

[boryspoplawski]

Reviewed 3 of 9 files at r11, 1 of 1 files at r12, all commit messages.
Reviewable status: all files reviewed, 17 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @aneessahib, @dimakuv, and @mkow)

---

libos/src/libos_init.c line 429 at r10 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Hm. I'll let @mkow and @boryspoplawski decide on the correct type for expanded_argv and the function signatures. I'm unsure how they would like it to be fixed.

Why the strings have to be const? They array should not strings, so this function should take char* const*
But see the other comments.

---

libos/src/libos_init.c line 370 at r12 (raw file):

noreturn void* libos_init(int argc, const char** argv, const char** envp) {
    __UNUSED(argc);

If it's unused then please remove it.

---

libos/src/libos_init.c line 424 at r12 (raw file):

    RUN_INIT(init_process, argc, argv);
    RUN_INIT(init_mount_root);
    RUN_INIT(init_threading);

Wait, you cannot just change order of initialization functions as you please, it's important to have this in correct order.
Why cannot init_process_args and init_process_cmdline run together with init_stack?

---

libos/test/regression/test_libos.py line 218 at r12 (raw file):

    @unittest.skipIf(USES_MUSL, 'Test uses /bin/sh from the host which is usually built against'
    'glibc')

Please align the string

---

libos/test/regression/test_libos.py line 227 at r12 (raw file):

    @unittest.skipIf(USES_MUSL, 'Test uses /bin/sh from the host which is usually built against'
    'glibc')

ditto

---

libos/test/regression/shebang_test_script.sh line 2 at r12 (raw file):

#!  /bin/sh
scripts/foo.sh

Suggestion:

#!/bin/sh

exec scripts/foo.sh

[dimakuv]

Reviewable status: all files reviewed, 18 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @aneessahib, @dimakuv, and @mkow)

a discussion (no related file):
I'm taking over this branch, to fix the const typecast issue.

---

[boryspoplawski]

Reviewable status: all files reviewed, 18 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @aneessahib, @dimakuv, and @mkow)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I'm taking over this branch, to fix the const typecast issue.

But I would rather fix other issues - they affect where we have these pointers

---

[dimakuv]

Reviewed 5 of 5 files at r13, all commit messages.
Reviewable status: all files reviewed, 18 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @aneessahib, @boryspoplawski, @dimakuv, and @mkow)

---

libos/src/libos_init.c line 429 at r10 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Why the strings have to be const? They array should not strings, so this function should take char* const*
But see the other comments.

So this is a complicated change by itself. I will do it in a separate PR, blocking this PR until we merge that one.

---

libos/src/libos_init.c line 441 at r11 (raw file):

Previously, aneessahib (Anees Sahib) wrote…

Yep - This is similar to what we are doing here.

gramine/libos/src/sys/libos_exec.c

Line 125 in cf8095d

ret = init_stack((const char**)argv, envp, &new_argp, &new_auxv);

So to fix this whole casting issue - we are going to have to get to what we have in TODO.

I am on it...

---

libos/src/libos_init.c line 370 at r12 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

If it's unused then please remove it.

Done. I'm not sure it's easier to read now, given introduced weirdness in the asm code...

---

libos/test/regression/test_libos.py line 218 at r12 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Please align the string

Done (did similarly as to how other skipIfs are implemented in this file)

---

libos/test/regression/test_libos.py line 227 at r12 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

ditto

Done (did similarly as to how other skipIfs are implemented in this file)

---

libos/test/regression/shebang_test_script.sh line 2 at r12 (raw file):

#!  /bin/sh
scripts/foo.sh

Done

[boryspoplawski]

Reviewed 4 of 5 files at r13, all commit messages.
Reviewable status: all files reviewed, 14 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @aneessahib, @dimakuv, and @mkow)

---

libos/src/libos_init.c line 370 at r12 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done. I'm not sure it's easier to read now, given introduced weirdness in the asm code...

What's weird about it?

---

libos/test/regression/test_libos.py line 218 at r12 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done (did similarly as to how other skipIfs are implemented in this file)

I don't like it, but it's probably some PEP requirement or something..

[boryspoplawski]

Reviewable status: all files reviewed, 15 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @aneessahib, @dimakuv, and @mkow)

---

libos/src/libos_init.c line 422 at r13 (raw file):

    RUN_INIT(init_ipc);
    RUN_INIT(init_process);

Testing Reviewable, ignore this comment please

Suggestion:

    RUN_INIT(aaa);
    
    RUN_INIT(bbb);

[boryspoplawski]

Reviewable status: all files reviewed, 15 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @aneessahib, @dimakuv, and @mkow)

---

libos/src/libos_init.c line 422 at r13 (raw file):

    RUN_INIT(init_ipc);
    RUN_INIT(init_process);

Testing Reviewable, ignore this comment please

Suggestion:

    RUN_INIT(aaaa);

    RUN_INIT(bbbb);

[dimakuv]

Reviewable status: all files reviewed, 14 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @aneessahib, @boryspoplawski, and @mkow)

a discussion (no related file):

Previously, boryspoplawski (Borys Popławski) wrote…

But I would rather fix other issues - they affect where we have these pointers

Blocking this PR on this prerequisite: #777

---

[dimakuv]

Reviewable status: all files reviewed, 14 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @aneessahib, @boryspoplawski, and @mkow)

---

libos/src/libos_init.c line 429 at r10 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

So this is a complicated change by itself. I will do it in a separate PR, blocking this PR until we merge that one.

I created a PR: #777

---

libos/src/libos_init.c line 370 at r12 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

What's weird about it?

Using argc passed by PAL to calculate envp. But maybe it's normal, I'm just too used to C conventions...

[dimakuv]

Reviewed 8 of 8 files at r14, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @boryspoplawski and @mkow)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Blocking this PR on this prerequisite: #777

Done, rebased on top of #777

---

a discussion (no related file):
NOTE: I rebased on top of #777 (current master), and also modified the PR substantially, so at this point it is easier to review from the very first revision!

---

---

libos/include/libos_handle.h line 281 at r14 (raw file):

int init_handle(void);
int init_std_handles(void);
int init_exec_handle(const char* const* argv, char*** out_new_argv);

FYI: I had to separate init_important_handles() into two functions:

1. It is more readable.
2. More importantly, init_exec_handle() also expands the entrypoint if it is a shebang, expanding the arguments as well.

---

libos/src/libos_init.c line 429 at r10 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I created a PR: #777

Done. Rebased on top of #777.

---

libos/src/libos_init.c line 441 at r11 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I am on it...

Done.

---

libos/src/libos_init.c line 424 at r12 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Wait, you cannot just change order of initialization functions as you please, it's important to have this in correct order.
Why cannot init_process_args and init_process_cmdline run together with init_stack?

Done. Not exactly as Borys wrote, but without any re-ordering.

---

libos/src/sys/libos_exec.c line 126 at r14 (raw file):

    elf_auxv_t* new_auxv;

    ret = init_process_cmdline((const char* const*)argv);

FYI: This was a missing functionality. We forgot to update /proc/self/cmdline after execve()

[dimakuv]

Jenkins, test this please

[mkow]

Reviewed 3 of 9 files at r11, 2 of 5 files at r13, 8 of 8 files at r14, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @aneessahib, @boryspoplawski, and @dimakuv)

a discussion (no related file):
Please add "Fixes #320" to the PR description.

---

---

-- commits line 4 at r14:
@dimakuv: You'll need to add Co-authored-by and your sign-off here.

---

libos/test/regression/run_script.manifest.template line 3 at r10 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

So, summarizing: in this particular case we don't have argv specified anywhere, so we need to pick some default. And IMO argv = [entrypoint] is a good one.

I agree with this. But didn't we derail the conversation? The PR is called "Support running scripts as the entrypoint", and what we're discussing here is a different feature. We should make a new PR for it.

@dimakuv: Could you create an issue for this and then fix it (after this PR gets merged), while you're on it?

[dimakuv]

Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @boryspoplawski and @mkow)

a discussion (no related file):

Previously, mkow (Michał Kowalczyk) wrote…

Please add "Fixes #320" to the PR description.

Done

---

---

-- commits line 4 at r14:

Previously, mkow (Michał Kowalczyk) wrote…

@dimakuv: You'll need to add Co-authored-by and your sign-off here.

Blocked myself on this.

---

libos/test/regression/run_script.manifest.template line 3 at r10 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

@dimakuv: Could you create an issue for this and then fix it (after this PR gets merged), while you're on it?

Done: #795

Yes, I can do a follow up PR.

[boryspoplawski]

Reviewed 8 of 8 files at r14, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @aneessahib and @mkow)

---

libos/src/libos_init.c line 370 at r14 (raw file):

noreturn void libos_init(const char* const* argv, const char* const* envp) {
    int ret;

Why this moved here? Not opposing, just curious, what's the point

---

libos/src/libos_init.c line 430 at r14 (raw file):

    RUN_INIT(init_std_handles);

    char** expanded_argv;

Suggestion:

char** expanded_argv = NULL;

---

libos/src/bookkeep/libos_handle.c line 72 at r14 (raw file):

}

int init_exec_handle(const char* const* argv, char*** out_new_argv) {

Actually, why this is a step separate from init_elf_objects()?

[mkow]

Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @aneessahib)

[dimakuv]

Reviewed 1 of 1 files at r15, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @aneessahib and @boryspoplawski)

---

libos/src/libos_init.c line 370 at r14 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Why this moved here? Not opposing, just curious, what's the point

Actually, leftover from an intermediate solution (which used ret extensively). Reverted, since this only clatters the PR.

---

libos/src/libos_init.c line 430 at r14 (raw file):

    RUN_INIT(init_std_handles);

    char** expanded_argv;

Done

---

libos/src/bookkeep/libos_handle.c line 72 at r14 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Actually, why this is a step separate from init_elf_objects()?

No idea, but I wouldn't clatter this PR with the merge of init_exec_handle() and init_elf_objects(). Deserves a separate PR.

[boryspoplawski]

Reviewed 1 of 1 files at r15, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @aneessahib and @dimakuv)

---

libos/src/bookkeep/libos_handle.c line 72 at r14 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

No idea, but I wouldn't clatter this PR with the merge of init_exec_handle() and init_elf_objects(). Deserves a separate PR.

But you already reworked how it behaves in this PR...
Well, approving anyway

[dimakuv]

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @mkow)

---

-- commits line 4 at r14:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Blocked myself on this.

Done

---

libos/src/bookkeep/libos_handle.c line 72 at r14 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

But you already reworked how it behaves in this PR...
Well, approving anyway

My main problem is that init_exec_handle() and init_elf_objects() are called far away from each other during LibOS init. Rearranging the order of function calls may break something unexpected, and we'll spend time in debugging that.

[dimakuv]

Jenkins, test this please

[mkow]

Reviewed 1 of 1 files at r15, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved