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.

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

Fix corner cases and style in the argv/argc code. #118

Merged
merged 1 commit into from
Oct 22, 2019
Merged
Changes from all commits
Commits
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
56 changes: 37 additions & 19 deletions libc-bottom-half/crt/crt1.c
Original file line number Diff line number Diff line change
Expand Up @@ -13,29 +13,47 @@ void _Exit(int) __attribute__((noreturn));
static __wasi_errno_t populate_args(size_t *argc, char ***argv) {
__wasi_errno_t err;

/* Get the sizes of the arrays we'll have to create to copy in the args. */
// Get the sizes of the arrays we'll have to create to copy in the args.
size_t argv_buf_size;
err = __wasi_args_sizes_get(argc, &argv_buf_size);
size_t new_argc;
err = __wasi_args_sizes_get(&new_argc, &argv_buf_size);
if (err != __WASI_ESUCCESS) {
return err;
}
if (*argc == 0) {
if (new_argc == 0) {
return __WASI_ESUCCESS;
}

/* Allocate memory for the array of pointers, adding null terminator. */
*argv = malloc(sizeof(char *) * (*argc + 1));
/* Allocate memory for storing the argument chars. */
char *argv_buf = malloc(sizeof(char) * argv_buf_size);
if (*argv == NULL || argv_buf == NULL) {
// Add 1 for the NULL pointer to mark the end, and check for overflow.
size_t num_ptrs = new_argc + 1;
if (num_ptrs == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't see how this can be zero since you just checked that new_argc is not zero.

Copy link
Member Author

Choose a reason for hiding this comment

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

It would be zero if the + 1 overflows (per the comment). Would it be more clear if this code used __builtin_add_overflow instead?

Copy link
Member

Choose a reason for hiding this comment

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

Oh yes, forgot about the pesky people passing 2^32 command line arguments :)

return __WASI_ENOMEM;
}

/* Make sure the last pointer in the array is NULL. */
*argv[*argc] = NULL;
// Allocate memory for storing the argument chars.
char *argv_buf = malloc(argv_buf_size);
Copy link
Member

Choose a reason for hiding this comment

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

In emscripten I ended up inlining this function into _start itself so that these could become alloca and simple programs could avoid depending malloc + free. Not related to this change but worth mentioning for later maybe?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good idea. Once the libpreopen reorg PR lands, and the next PR to make it so that the libpreopen code doesn't get linked in if you don't use it, then this will be the only malloc left, so we should remove it. I'll submit a separate PR for that.

if (argv_buf == NULL) {
return __WASI_ENOMEM;
}

// Allocate memory for the array of pointers. This uses `calloc` both to
// handle overflow and to initialize the NULL pointer at the end.
char **argv_ptrs = calloc(num_ptrs, sizeof(char *));
if (argv_ptrs == NULL) {
free(argv_buf);
return __WASI_ENOMEM;
}

/* Fill the argument chars, and the argv array with pointers into those chars. */
return __wasi_args_get(*argv, argv_buf);
// Fill the argument chars, and the argv array with pointers into those chars.
err = __wasi_args_get(argv_ptrs, argv_buf);
if (err == __WASI_ESUCCESS) {
*argc = new_argc;
*argv = argv_ptrs;
} else {
free(argv_buf);
free(argv_ptrs);
}
return err;
}

static __wasi_errno_t populate_libpreopen(void) {
Expand Down Expand Up @@ -80,35 +98,35 @@ static __wasi_errno_t populate_libpreopen(void) {
}

void _start(void) {
/* Record the preopened resources. */
// Record the preopened resources.
if (populate_libpreopen() != __WASI_ESUCCESS) {
_Exit(EX_OSERR);
}

/* Fill in the environment from WASI syscalls, if needed. */
// Fill in the environment from WASI syscalls, if needed.
if (&__wasilibc_populate_environ != NULL) {
if (__wasilibc_populate_environ() != __WASI_ESUCCESS) {
_Exit(EX_OSERR);
}
}

/* Fill in the arguments from WASI syscalls. */
// Fill in the arguments from WASI syscalls.
size_t argc;
char **argv;
if (populate_args(&argc, &argv) != __WASI_ESUCCESS) {
_Exit(EX_OSERR);
}

/* The linker synthesizes this to call constructors. */
// The linker synthesizes this to call constructors.
__wasm_call_ctors();

/* Call main with the arguments. */
// Call main with the arguments.
int r = main(argc, argv);

/* Call atexit functions, destructors, stdio cleanup, etc. */
// Call atexit functions, destructors, stdio cleanup, etc.
__prepare_for_exit();

/* If main exited successfully, just return, otherwise call _Exit. */
// If main exited successfully, just return, otherwise call _Exit.
if (r != 0) {
_Exit(r);
}
Expand Down