fix(c): describe the real hazard in insecure-use-strtok-fn - #4022
Conversation
The message presented strtok()'s documented behavior (modifying the first argument in place) as the problem. The security-relevant issue is that strtok() is not reentrant or thread-safe: it stores the parser position in a hidden static buffer, so interleaved or concurrent calls corrupt each other's state. Reword the message around that, keep the in-place modification as a secondary note, and mention strtok_s() alongside strtok_r(). Signed-off-by: Eljees <3.14hell@gmail.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02990032ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| permanently erasing the | ||
| delimiter character. Use 'strtok_r()' instead. | ||
| Avoid 'strtok()': it is not reentrant or thread-safe, since it keeps the parser | ||
| position in a hidden static buffer, so interleaved or concurrent calls (even on |
There was a problem hiding this comment.
When this diagnostic is shown for any strtok() call, it incorrectly implies that the function stores the parser position in a hidden character buffer. The hidden object is parser state—typically a static pointer into the caller-provided input—while the input itself remains the only token buffer and is modified in place. Since this change is specifically intended to explain the real hazard, calling that state a “static buffer” gives users an inaccurate memory model; describe it as hidden static state or a saved pointer instead.
Useful? React with 👍 / 👎.
| Avoid 'strtok()': it is not reentrant or thread-safe, since it keeps the parser | ||
| position in a hidden static buffer, so interleaved or concurrent calls (even on | ||
| unrelated strings) clobber each other's state. It also modifies its input in |
There was a problem hiding this comment.
For implementations that keep the tokenizer state in thread-local storage, concurrent calls from different threads do not clobber one another, so the unconditional statement that strtok() is not thread-safe is stronger than the C interface guarantees. The portable problem is that C does not require strtok() to avoid data races, while interleaved calls in the same thread still share state. Phrase this as “not guaranteed to be thread-safe” rather than promising that every implementation exhibits concurrent state corruption.
Useful? React with 👍 / 👎.
| Avoid 'strtok()': it is not reentrant or thread-safe, since it keeps the parser | ||
| position in a hidden static buffer, so interleaved or concurrent calls (even on | ||
| unrelated strings) clobber each other's state. It also modifies its input in | ||
| place, overwriting each delimiter with a NUL byte. Use the reentrant |
There was a problem hiding this comment.
For inputs with leading, trailing, or consecutive delimiters, strtok() skips some delimiters without modifying them and overwrites only the delimiter that terminates each returned token. Thus “overwriting each delimiter” is observably false—for example, tokenizing ",,a,,b,," leaves several commas intact—and can mislead users inspecting or reusing the mutated buffer. State that token-ending delimiters are replaced with NUL bytes instead.
Useful? React with 👍 / 👎.
Address review feedback on the strtok message: - the saved parser position is hidden static state (a pointer into the caller's string), not a separate buffer; - the C standard does not require strtok() to be thread-safe, rather than every implementation racing; - only the delimiter that terminates each token is overwritten with NUL, not every delimiter. Signed-off-by: Eljees <3.14hell@gmail.com>
|
All three are right, and on a PR that is specifically about message accuracy they matter. Fixed in d2e8e14:
|
1 participant
Fixes #3772
Problem
The
insecure-use-strtok-fnmessage describesstrtok()'s documented behavior as if it were the hazard:Modifying the input in place is exactly what
strtok()is specified to do, and it is not why the rule recommendsstrtok_r(). As #3772 points out, the message leaves a reader unsure what the actual risk is — and it doesn't explain what makes the suggested replacement better, sincestrtok_r()modifies the buffer in the same way.Change
Message only — the pattern, metadata and severity are untouched.
The rewrite leads with the real reason
strtok_r()exists:strtok()keeps the parser position in a hidden static buffer, so it is neither reentrant nor thread-safe and interleaved or concurrent calls clobber each other's state. In-place modification is kept as a secondary note, andstrtok_s()(C11 Annex K) is mentioned next tostrtok_r()for portability.No test changes are needed since matching behavior is unchanged.