feat(os): add copy_argv() function #281
Conversation
C programs such as run_command() accept argv as an `char *`, not
a `const char *`. This is because the `main()` function of programs
is explicitly allowed to modify the data in `argv()`.
This can cause issues, since we declare arrays using string literals,
(e.g. `char * argv[] = {"hello", "world", NULL};`), and modifying
string literals is undefined behaviour.
To get around this, we can just make a mofiiable copy of argv before
passing it to run_command().
There is a warning we can enable that will check for this
(`-Wwrite-strings`), but it creates 100s of warnings throughout our
code-base, so it's something we should slowly fix.
Codecov Report
@@ Coverage Diff @@
## main #281 +/- ##
==========================================
+ Coverage 49.04% 49.14% +0.10%
==========================================
Files 116 116
Lines 18519 18542 +23
==========================================
+ Hits 9083 9113 +30
+ Misses 9436 9429 -7
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| log_errno("Failed to malloc %d bytes", argv_array_size + strings_length); | ||
| return NULL; | ||
| } | ||
| char *const argv_string_buffer = &((char *)argv_copy)[argv_array_size]; |
There was a problem hiding this comment.
Not clear where argv_copy[0] is assigned.
There was a problem hiding this comment.
argv_copy[0] is assigned a few lines down:
Line 314 in e6bb700
The first half of the malloc()-ed data is filled char* (string pointers).
The second half of the malloc()-ed data is where the string buffer/contents are.
Essentially, if you copy const char * argv[] = {"Hello", "World!", NULL};, the data structure of argv_copy looks something like the following:
| Address | 0-7 | 8-15 | 16-23 | 24-29 | 30-36 |
|---|---|---|---|---|---|
| CType | char* | char* | char* | char[] | char[] |
| Value | 24 | 30 | NULL | "Hello" | "World!" |
I've added some more comments, since I had to make the data structure a bit confusing so that a single free() will delete all the data, see
Lines 332 to 336 in 430604f
Co-authored-by: Alexandru Mereacre <mereacre@gmail.com>
Adds some additional comments on what certain confusing variables do.
aloisklink
force-pushed
the
feat/add-copy-argv-func
branch
from
c8a4bc8
to
430604f
Compare
|
Tests added in 430604f! |
C programs such as
run_command()accept argv as anchar *, not aconst char *. This is because themain()function of programs is explicitly allowed to modify the data inargv().This can cause issues, since we declare arrays using string literals, (e.g.
char * argv[] = {"hello", "world", NULL};), and modifying string literals is undefined behaviour.To get around this, we can just make a mofiiable copy of argv before passing it to run_command().
There is a warning we can enable that will check for this (
-Wwrite-strings), but it creates 100s of warnings throughout our code-base, so it's something we should slowly fix.