| Message ID | 1d955f8bc6d2797def516897d019a186e461b648.1681428696.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | banned: mark `strok()`, `strtok_r()` as banned | expand |
Taylor Blau <me@ttaylorr.com> writes: > - `strtok_r()` forces the caller to maintain an extra string pointer > to pass as its `saveptr` value > > - `strtok_r()` also requires that its `saveptr` value be unmodified > between calls. > > - `strtok()` (and by extension, `strtok_r()`) is confusing when used > across multiple functions, since the caller is supposed to pass NULL > as its first argument after the first call. This makes it difficult > to determine what string is actually being tokenized without clear > dataflow. It seems that the only existing users of strtok() are all in t/helper/ directory, so I personally do not think it is a huge loss if these two are forbidden. While I do not see why we should use strtok(), none of the above sound like sensible reasons to ban strtok_r(). At best, they may point out awkwardness of the function to make you try finding an alternative that is easier-to-use before choosing strtok_r() for your application on a case-by-case basis. If your application wants to chomp a string into tokens from left to right, inspecting the resulting token one-by-one as it goes until it hits a token that satisfies some condition and then terminate without wasting cycles on the rest, string_list_split_in_place() is a poor choice. In such a use case, you do not know upfront where in the string the sought-after token would be, so you have to split the string in full without taking an early exit via maxsplit. Also, you are restricted to a single byte value for the delimiter, and unlike strtok[_r](), string_list_split_in_place() does not squash a run of delimiter bytes into one inter-token delimiter. One gripe I have against use of strtok() is actually not with threading but because people often misuse it when strcspn() is what they want (i.e. measure the length of the "first token", so that they can then xmemdupz() a copy out), forgetting that strtok[_r]() is destructive. So, I dunno.
On Thu, Apr 13, 2023 at 7:00 PM Junio C Hamano <gitster@pobox.com> wrote:
> ... At best, they may point out awkwardness of the function [strtok_r]
The awkward interface for strtok as extended to strtok_r is why
we (Keith Bostic and I) came up with what's now strsep. Of course
strsep is not standard and has its own issues but it can be used
to parse $PATH for instance, or old-style /etc/passwd entries.
In this case (tests) it's probably not that important to be efficient
anyway,
Chris
On Thu, Apr 13, 2023 at 06:39:18PM -0700, Junio C Hamano wrote: > Taylor Blau <me@ttaylorr.com> writes: > > > - `strtok_r()` forces the caller to maintain an extra string pointer > > to pass as its `saveptr` value > > > > - `strtok_r()` also requires that its `saveptr` value be unmodified > > between calls. > > > > - `strtok()` (and by extension, `strtok_r()`) is confusing when used > > across multiple functions, since the caller is supposed to pass NULL > > as its first argument after the first call. This makes it difficult > > to determine what string is actually being tokenized without clear > > dataflow. > > It seems that the only existing users of strtok() are all in > t/helper/ directory, so I personally do not think it is a huge loss > if these two are forbidden. While I do not see why we should use > strtok(), none of the above sound like sensible reasons to ban > strtok_r(). At best, they may point out awkwardness of the function > to make you try finding an alternative that is easier-to-use before > choosing strtok_r() for your application on a case-by-case basis. For what it's worth, I could certainly live if we accomplished getting strtok(2) on the banned list, but left strtok_r(2) un-banned. TBH, I think that leaving the reenterant version of a banned function as un-banned is a little awkward, I don't mind it if you don't feel like the above are sufficient reasons to ban it. > If your application wants to chomp a string into tokens from left to > right, inspecting the resulting token one-by-one as it goes until it > hits a token that satisfies some condition and then terminate > without wasting cycles on the rest, string_list_split_in_place() is > a poor choice. In such a use case, you do not know upfront where in > the string the sought-after token would be, so you have to split the > string in full without taking an early exit via maxsplit. Also, you > are restricted to a single byte value for the delimiter, and unlike > strtok[_r](), string_list_split_in_place() does not squash a run of > delimiter bytes into one inter-token delimiter. I don't quite agree with this. In practice, you could repeatedly call `string_list_split_in_place()` with maxsplit of "1", using the tail of the string list you're splitting into as the string to split. That would allow you to split tokens one at a time into the string list without having to split the whole line up front. That all said, I don't think that we have such a use case in the tree, at least from my searching for strtok() and string_list_split_in_place(). It may be that using strtok_r() for such a thing would be less awkward: not having tried both (and having no examples to reference) I honestly do not know for certain. > One gripe I have against use of strtok() is actually not with > threading but because people often misuse it when strcspn() is what > they want (i.e. measure the length of the "first token", so that > they can then xmemdupz() a copy out), forgetting that strtok[_r]() > is destructive. Heh, I'm happy to add that to this, if you want ;-). Thanks, Taylor
diff --git a/banned.h b/banned.h index 6ccf46bc197..9bd23ce5732 100644 --- a/banned.h +++ b/banned.h @@ -18,6 +18,12 @@ #define strncpy(x,y,n) BANNED(strncpy) #undef strncat #define strncat(x,y,n) BANNED(strncat) +#if 0 +#undef strtok +#define strtok(x,y) BANNED(strtok) +#undef strtok_r +#define strtok_r(x,y,z) BANNED(strtok_r) +#endif #undef sprintf #undef vsprintf
`strtok_r()` is reentrant, but `strtok()` is not, meaning that using it is not thread-safe. We could ban `strtok()` and force callers to use its reentrant counterpart, but there are a few drawbacks to doing so: - `strtok_r()` forces the caller to maintain an extra string pointer to pass as its `saveptr` value - `strtok_r()` also requires that its `saveptr` value be unmodified between calls. - `strtok()` (and by extension, `strtok_r()`) is confusing when used across multiple functions, since the caller is supposed to pass NULL as its first argument after the first call. This makes it difficult to determine what string is actually being tokenized without clear dataflow. So while we could ban only `strtok()`, there really is no good reason to use either when callers could instead use the much friendlier `string_list_split_in_place()` API, which avoids the above issues. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- banned.h | 6 ++++++ 1 file changed, 6 insertions(+)