The abominable strtok() | Hacker News Reader

The abominable strtok()

zefonseca.com

18 pointswhackberry31 comments

I've heard lots of dev teams gripe about strsep(3) because it "isn't standard" or "isn't cross-platform". Preposterous! strsep is a ~20 line ANSI C function; it will compile and run flawlessly on any platform that doesn't natively provide it.

I agree strongly with the other comments on this thread that strsep(3) is just peachy, even though it "alters its inputs". Unlike in functional programs, reasonable destructive functions are to be preferred in C programs; it's easier to work around destructiveness than it is to track state (and/or to go through contortions to pretend that you aren't tracking state).

First off, strtok() is dead, long live strsep().

Secondly, this is C code--it's pretty low-level. Yes, strsep() messes with your input. But that is very well documented. If you don't want it to, strdup() beforehand (or strndup() if you don't trust the input).

His whole example of strtok() dying on a character constant is stupid--why on earth would you do that? If you've got a string constant you may as well just have the constant array and save yourself the parsing headache.

This isn't rocket science.

Always use strndup(). Never trust your input.

If you're trying to strdup() something you're not even sure is null-terminated, you're already doomed.

The parent said to use strndup(), which takes a max value of characters to copy.

And adds a \0 after the characters copied, which means you have to remember to do the sizeof(dest)-1 thing to avoid an overwrite.

The people who designed the C string functions should have been made to pee in a cup before they were allowed into the building. So much trouble could have been avoided if they had only been consistent about using memory block sizes as parameters rather than "string" (sic) lengths.

Which is annoying, yes, but using strndup() negates apenwarr's point.

What are you talking about? There is no sizeof(dest) with strndup()--it's newly allocated memory sized appropriately so there's no chance of an overwrite.

While we're on the subject, what is the point of strndup()? Keeping people from running you out of memory with big long strings? They'll just find other ways to do the same thing (by repeatedly feeding you strings of threshold length).

(Actually it was strncpy's wacky behavior I was thinking of. I don't think I've ever used strndup().)

I tend to use and recommend str(c)spn, which althought more low-level are more flexible and often what you want. And also you don't have to modify input if you don't want to.

strcspn is really, really fussy. When what you want is the semantic equivalent of "foo,bar, baz".split(','), I don't think it's worth the potential for error, or the tangliness of the code.

I began to have serious concerns about this article when I saw the author allocate and then immediately leak memory in the first example.

I agree with the message: don't use strtok, it has unpleasant side effects that you probably don't want. I do not feel this article does a good job of presenting that message. It spends too much time on pathologically bad examples of strtok usage while only briefly mentioning (and providing no example code for) any of the alternatives.

He also elided basic error checking. I have no problem if people don't follow all good programming practices in pedagogical examples.

I would say that the tok = calloc(); tok = strtok(something-which-is-not-in-any-way-related-to-tok) is to some extent clear sign of cargo-cult C programming and has nothing to do with good programming practices.

I thought the complaint was that he didn't free what he malloced. On second look, yes, that's not the kind of code I want to see anywhere, pedagogical or not.

What error checking are you referring to? If it's the alloc, I think the explicit error check on it is usually an antipattern and should be avoided.

Curious: you think it's better to crash attempting to use a NULL pointer? If so, why is that better than checking for the NULL?

I think regimes that involve manual checking introduce the possibility of mistakes; when those mistakes involve array references, they can be exploitable.

I've always, in both my dev career and in security research, felt that manual checking is dumb. In 99.999% of cases, the only option given to code when allocations fail is to start a chain of events that ends the program. Inflating each and every allocation (or, more likely, missing the little things that allocate silently, like strdup) into 4-5 lines of code seems wasteful.

What I advocate is what my friend Danny told me he did at Juno in the mid '90s: preload a wrapper on malloc (don't use silly wrappers like "xmalloc") that catches faults and does something intelligent; centralize your handling of allocation failures, instead of trying to graft it onto each and every allocation point.

The nice thing about my philosophy: it means that when you write normal, non-runtime code, you just pretend malloc never fails. Your code is cleaner, and it's probably safer.

It depends what kind of program it is. A long running GUI program probably doesn't want to completely abort if the user tries to open file that it doesn't have enough memory to process--especially if there are other unsaved documents open.

Similarly an embedded program might not want to reboot itself if there aren't enough memory resources to carry out a command (due to other commands being processed at the same time).

But you are right, a lot of the time (perhaps even the majority of the time) it really is perfectly acceptable to just die if you can't malloc().

And it really does take a lot of effort to manage memory like that, but if it is done right it can really make things work robustly.

Your allocation failure strategy is orthogonal to how individual allocations should be handled. In a UI program, keep a reserve of memory, return valid addresses to the caller, and pop up a warning.

It's actually easier to do things like this if you aren't hand-crafting individual little alloc-check-handle routines.

That seems full of edge cases. If your reserve isn't big enough it doesn't help and if it's too big then it's a waste. It seems much cleaner to be able to gracefully abort whatever operation you were doing. Presumably there is already error handling for fs/network errors. Usually you can hop onto those handlers with memory errors and be just fine.

Though if it's linux it'll just kill you anyway :-). (Yeah, I know you can turn the OOM killer off).

I wasn't actually talking about error checking, I was talking about the lines:

    char *tok = (char *)malloc(SOMEMEMORY);
    tok = strtok(data, ":");

I don't actually care whether the malloc succeeded or whether or not he checked for an error, as the second line blows away the only reference to the potentially allocated memory. Whatever may or may not have been allocated has now officially been leaked.

The cast on that malloc drives me f'ing nuts. DON'T CAST MALLOC.

I think anyone that's done even a little bit of C work on any platform is aware of this issue.... but it's always worth griping over it some more. I guess the poster just ran into it and couldn't help but express his frustration :)

For Win32 developers who don't have the glib g_strsplit function, you can use strtok_s which is detailed on MSDN here: http://msdn.microsoft.com/en-us/library/ftsafwz3(VS.80).aspx

strtok_s is re-entrant, thread-safe, and uses no global data, but keep in mind it still modifies your input string.

The man page covers the problems with strtok pretty well. I found it useful enough and while the code that uses it is pretty awkward it works well enough and gets the job done.

I think I that I have used it in shipping code two or three times. These days I don't do much in C so it's unlikely I'll ever use it again.

strtok() altering the source string is a natural consequence of null termination. The traditional Pascalian equivalent (put the string length at the head of the string) creates a symmetric problem at the head of the string. My (perhaps naive) idea is to use a struct containing the string length and pointing to the beginning of the string data. Nondestructive tokenization becomes simple.

This post actually gave me an impetus to go back and add tokenization to my string library that's built around this idea.

The big problem with tok isn't that it alters the string; it's that it keeps state, and so can't be used in multicontext code. sep changes the interface to delegate state tracking to the caller, which is the way it should be.

Just as map and reduce in Python is "unpythonic", interfaces that mint new temp strings to process what should be a simple charstar are un-c-like, and should be avoided.

Just as map and reduce in Python is "unpythonic", interfaces that mint new temp strings to process what should be a simple charstar are un-c-like, and should be avoided.

That's not what I'm doing at all, though. I'm just giving char *'s and size_t's for portions of the original character array, not duplicating subsections of the array at all.

You are, in other words, reinventing strspn?

strspn takes a set of characters to match against and tells you how long to go until you reach a non-matching character. My tokenizer, like strtok, takes a set of delimiter characters. So same thing except with complement sets, I guess.

It's also part of a larger library that consistently uses pointer/length pairs to designate strings as opposed to null termination, which admittedly is quite non-C. No clue whether it's actually a good idea.

EDIT: If anyone's actually curious, I have it on github: http://github.com/philwelch/string

Reply on news.ycombinator.com