Fix corner cases and style in the argv/argc code. #118
Conversation
|
@tschneidereit There's a Windows CI failure affecting this and other PRs which seems unrelated to the actual PRs; do you know what might be causing this? The error message is: |
|
@sunfishcode this is unfortunately a known bug, but you can fix it by passing |
This works around [this bug], which manifests as ``` error: could not remove 'setup' file: 'C:\Users\VssAdministrator\.cargo\bin/rustup-init.exe' info: caused by: Access is denied. (os error 5) ``` as suggested by [this comment]. [this bug]: https://github.com/microsoft/azure-pipelines-image-generation/issues/1224 [this comment]: #118 (comment)
This works around [this bug], which manifests as ``` error: could not remove 'setup' file: 'C:\Users\VssAdministrator\.cargo\bin/rustup-init.exe' info: caused by: Access is denied. (os error 5) ``` as suggested by [this comment]. [this bug]: https://github.com/microsoft/azure-pipelines-image-generation/issues/1224 [this comment]: #118 (comment)
This fixes some potential memory leaks in pathological situations, and updates the code to use `//`-style comments.
sunfishcode
force-pushed
the
args-cleanup
branch
from
596a96a to
0f9e435
Compare
|
Thanks! The workaround is now in, and the CI is passing. |
| 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) { |
There was a problem hiding this comment.
I don't see how this can be zero since you just checked that new_argc is not zero.
There was a problem hiding this comment.
It would be zero if the + 1 overflows (per the comment). Would it be more clear if this code used __builtin_add_overflow instead?
There was a problem hiding this comment.
Oh yes, forgot about the pesky people passing 2^32 command line arguments :)
| /* 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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
This way, if an application doesn't otherwise use malloc, they don't need to link in malloc. Idea from #118 (comment)!
This fixes some potential memory leaks in pathological situations, and
updates the code to use
//-style comments.This is a subset of #112, extracted for easier reviewing.