ext/pcntl: Refactor usage of strlcpy #17172
Conversation
ext/pcntl/pcntl.c
Outdated
| strlcpy(*pair, ZSTR_VAL(key), ZSTR_LEN(key) + 1); | ||
| strlcat(*pair, "=", pair_length); | ||
| strlcat(*pair, Z_STRVAL_P(element), pair_length); | ||
| const uint8_t equal_len = strlen("="); |
There was a problem hiding this comment.
not sure equal_len existence is really vital but not against it either.
ext/pcntl/pcntl.c
Outdated
| current_arg++; | ||
| } ZEND_HASH_FOREACH_END(); | ||
| *current_arg = NULL; | ||
| } else { | ||
| argv = emalloc(2 * sizeof(char *)); | ||
| argv = safe_emalloc(2, sizeof(char *), 0); |
There was a problem hiding this comment.
This isn't a necessary, this can't overflow anyway.
There was a problem hiding this comment.
I never really know when one is meant to use the safe version compared to normal emalloc.
There was a problem hiding this comment.
If overflow by summation/multiplication is possible, use the safe version
ext/pcntl/pcntl.c
Outdated
| zend_string_release(key); | ||
| goto cleanup_env_vars; | ||
| } | ||
| // TODO Check key and element do not have nul bytes? |
ext/pcntl/pcntl.c
Outdated
| strlcat(*pair, Z_STRVAL_P(element), pair_length); | ||
| const uint8_t equal_len = strlen("="); | ||
| size_t pair_length = ZSTR_LEN(element_str) + equal_len + ZSTR_LEN(key) + 1; | ||
| ZEND_ASSERT(pair_length < SIZE_MAX); |
There was a problem hiding this comment.
This assertion doesn't really make a lot of sense, pair_length is size_t after all...
Granted, the assertions that were here prior to this PR also didn't make a lot of sense.
ext/pcntl/pcntl.c
Outdated
| strlcat(*pair, "=", pair_length); | ||
| strlcat(*pair, Z_STRVAL_P(element), pair_length); | ||
| const uint8_t equal_len = strlen("="); | ||
| size_t pair_length = ZSTR_LEN(element_str) + equal_len + ZSTR_LEN(key) + 1; |
There was a problem hiding this comment.
Never open-code an addition of string lengths, unless you know they're bounded such that they can't overflow; or if you overflow-check them yourself.
In this case, you should keep using safe_emalloc and only perform this summation after that call.
If you don't, you risk integer overflow which can turn into heap buffer overflow.
We allocate the buffer, so we know that it will fit. Drive by refactorings and questions
Girgias
force-pushed
the
pcntl-strlcat-refactor
branch
from
c54e11c to
c97e105
Compare
Co-authored-by: Niels Dossche <[email protected]>
We allocate the buffer, so we know that it will fit.
Drive by refactorings and questions