5 ms·
Quiz time, what's the bug in this C code: int parse_until(FILE *src, char *ptr, ssize_t maxlen, const char *s) { int out = 0; while (out < maxlen) {
by jcranmer 3y ago
Quiz time, what's the bug in this C code:
int parse_until(FILE *src, char *ptr, ssize_t maxlen, const char *s) {
int out = 0;
while (out < maxlen) {
*ptr = fgetc(src);
if (*ptr == EOF) { // hit error while scanning
*ptr = 0;
return ferror(src) ? -out : out;
} else if (strchr(s, *ptr)) {
*ptr = 0;
return out;
}
ptr++; out++;
}
// we only make it here if we hit maxlen
(*--ptr) = 0;
int skipped = parse_skipwhile(src, s);
if (skipped > 0) {
return out + skipped; // errors are negative, eof is ok
}
return ferror(src) ? (skipped - out) : (out - skipped);
}
Hint: it relies on implementation-defined behavior such that it (mostly) works on x86 but not on ARM.
(The fact that it silently clips the string if it's overlong is annoying, but it's not what I was thinking).
So EOF is defined as -1. Depending on how the environment chooses to define char, one of two things can happen. On systems like x86 where char is signed, it's impossible to read a byte value of 0xff, as it is confused with EOF. If instead you're on a system like ARM where char is unsigned, then you can never read EOF.
- 634636346 3y agoThis (distinguishing -1 from a character value when char is unsigned) is one of the reasons character literals in C have type int instead of char (C++ changed them to char.) Note the return type of getchar().
- atahanacar 3y ago>fgetc() reads the next character from stream and returns it as an unsigned char cast to an int, or EOF on end of file or error. >getc() is equivalent to fgetc() except that it may be implemented as a macro which evaluates stream more than once. >getchar() is equivalent to getc(stdin).
- synergy20 3y agofgetc() must return to int instead of char, otherwise you can not detect EOF.
- gsliepen 3y agoAnother bug: if `maxlen <= 0`, then it will overwrite the character before the start of `ptr`. And then there's the infinite loop if `maxlen > INT_MAX` on 64-bit machines. And maybe `out + skipped` can wrap as well?
- jcranmer 3y agomaxlen is meant to be a constant provided by a #define, so I wouldn't be worried about things going haywire if maxlen is a weird value. But C-like code in general makes me nervous in parsers, since there's many ways things can go wrong and C is inherently fail-deadly if you make a mistake.
- jstimpfle 3y agoCoding in C doesn't mean that you shouldn't create a layering of appropriate abstractions.
- SubjectToChange 3y agoThe problem is that writing abstractions in C is more difficult than virtually any other practical language.
- jstimpfle 3y agoThat's a wild claim. Why do you think is that? What is an abstraction and what do you need to code it?
- NobodyNada 3y agoOne more: if an error occurs while reading the first character, the function returns -out = 0, which violates the documented behavior that errors are negative, and means a caller can’t distinguish between a failing to read and reading an empty file.
- dleslie 3y agoAh, implicit type conversions being a footgun yet again. int fgetc( FILE *stream ); But the retval is treated as a char.
- kevin_thibedeau 3y agoThe retval is an int. It only gets treated as a char if it isn't EOF. The caller is supposed to know this and do the right thing rather than blindly downcasting. This is more apparent with the wide char library where there is a special wint_t with the sole purpose of serving as a return value that represents a wchar_t or WEOF.
- Someone 3y ago> Hint: it relies on implementation-defined behavior I don’t think so. fgetc returns an int (https://en.cppreference.com/w/c/io/fgetc https://en.cppreference.com/w/c/io/fgetc) and EOF is an int (https://en.cppreference.com/w/c/io https://en.cppreference.com/w/c/io) So, *ptr = fgetc(src); discards part of the result of fgetc. If my C isn’t too rusty, that means the code can’t discriminate between hitting end of file and reading some byte value from the files (AFAIK EOF is -1 on most systems, so that value would often be 0xFF)
- jcranmer 3y agoEOF being -1 means that if (*ptr == EOF) can never fire if *ptr can only take on the values 0..255 [i.e., char is unsigned] as opposed to -128..127 [i.e., char is signed]. Whether or not char is signed or unsigned is implementation-defined.
- boywitharupee 3y agoThis is interesting and I'm curious how is this being solved in the wild? I'm on an ARM (m1 mac) machine and looks like char type is a signed type. EOF on this machine is -1 1. How can you distinguish between the char in the file and EOF (-1)? 2. Is EOF reserved for all files on macOS?
- comex 3y agoWhen fgetc succeeds, the int it returns has a value based on interpreting the byte as an unsigned char. On a platform with an 8-bit char, that means it’s in the range 0 to 255. This is required by the C spec regardless of whether char is signed or unsigned. Meanwhile, EOF is required to be negative. Thus, you can always distinguish the cases as long as you look at the original int return value rather than casting to char. Fun fact: This approach causes trouble on obscure embedded platforms where char and int are the same size (and therefore an unsigned char value can’t fit inside a signed int). Such platforms are allowed by the C standard as freestanding implementations that don’t implement the full standard library, but they can’t conformantly implement fgetc. https://stackoverflow.com/questions/3860943/can-sizeofint-ever-be-1-on-a-hosted-implementation https://stackoverflow.com/questions/3860943/can-sizeofint-ev...
- Joker_vD 3y agoWhat's even with this style of code that seems to be trying to save up on local variables? It instead writes directly into the output buffer's pointer then immediately re-reads the value back from there and then performing the relevant logic (which may include un-writing the value from the buffer). Memory reads aren't that easily optimized away and this style easily leads to logic errors just like this one. Also, I think you got the "what will work" question backwards with signed/unsigned. MSVC on x86/x64 has signed char and so, e.g., "isupper(c = fgetc(f))" segfaults on reading a non-ASCII char; and similarly in this case reading "я" from a Win-1251-encoded file (or "ÿ" from a Win-1252-encoded file) will be treated as (premature) EOF.
- lelanthran 3y agoI dunno about this style of code, it looks hard to follow :/ > No newlines in keys, values, or section names. Empty values are not allowed. Comments only on their own lines (minus whitespace). Whitespace-insensitive (whitespace at the start of line, end of line, around the “=”, is all ignored). No need for a terminating newline either. Oh that's more than most C ini parsers do? Isn't that convenient Nonsense. My ini parser has fewer restrictions (only one, on line length) and looks nothing like that unreadable mess. Maybe unrelated, but I'm pretty certain I've seen gcc warnings when assigning ints to chars, unsigned or otherwise
- zh3 3y agoThat's a simple case, I've seen much worse because of endianess and different archs have different conventions for whether char is signed or unsigned by default (common these days with arm/X86). Any C programmer who's coded for X86 and ARM will know what the rules are and how not to break them (I'd like to hope, anyway).
- kabdib 3y agochar cannot legally represent EOF (as you point out). This doesn't really have anything to do with ARM, though; for instance, the ARM compiler I use has command-line options and pragmas that control whether char is signed or unsigned. Char is just too small. The type of 'out' is a different type than ssize_t, which may not matter for the practical ranges involved in parsing a file, but it certainly a bad sign. Writes to memory it doesn't own if maxlen == 0 (unlikely, but who knows?) Additionally: calling strchr on every input character is crazy. Frankly, the same applies to fgetc.
- sltkr 3y agoThe compiler is able to warn you about this: <source>:8:18: warning: comparison is always false due to limited range of data type [-Wtype-limits] 8 | if (*ptr == EOF) { Of course this warning only happens when you target ARM, so I can imagine it's still hard to catch if you do all your development on x86, and only occasionally cross-compile to ARM without heeding compiler warnings.
- achates 3y agoA good reason to always build with warnings as errors.
- mananaysiempre 3y agoAFAICS this is actually potential UB on most implementations: not only does fgetc() return -1 as an int in the failure case, it returns the byte as unsigned char in the success case. So on an implementation that defines plain char to have the same size and representation as signed char (as x86 does), reading a byte beyond SCHAR_MAX causes UB through signed overflow on the first line of the loop. (This is a close relative of the well-known footgun that the is*() functions from <ctype.h> accept an unsigned char value as an int, and it’s completely valid—though rare in practice—for them to blow up when passed, say, "\xFF"[0] instead of ((const unsigned char *)"\xFF")[0] on an implementation with CHAR_BIT==8 and SCHAR_MIN<0.)
- LegionMammal978 3y agoOverflow from signed arithmetic always results in UB. But an unsigned-to-signed conversion outside the range of the target type just results in implementation-defined behavior; see C17 §6.3.1.3 ("Signed and unsigned integers"). And most people these days (including me) take the simple approach of not supporting any implementation that doesn't just perform two's-complement wrapping for all conversions.
- jcranmer 3y ago> And most people these days (including me) take the simple approach of not supporting any implementation that doesn't just perform two's-complement wrapping for all conversions. C2x in fact requires 2's complement.
- LegionMammal978 3y agoNot quite; C23 requires a two's-complement representation, but it doesn't require two's-complement wrapping for unsigned-to-signed conversions. §6.3.1.3 hasn't been changed, so it's still left to the implementation to guarantee that. The only easily-visible effects of requiring a two's-complement representation are that X_MIN == -X_MAX - 1 for all signed integer types, and in general that the object representations of x and -x - 1 differ by only 1 bit, if the integer type has no padding bits.
- jstimpfle 3y agoThe bug is that it's missing abstractions. 1) reading directly from a FILE. 2) Saving local state through a pointer (ptr). 3) weird way to report back the out state, in case of EOF can't distinguish between ferror or not. 4) Probably it would be better to not check for errors right away -- just share a common path with EOF everywhere, and check for I/O errors only at the very end. 5) What is the use of this function? Not sure what's the point, it seems unergonomic. Probably better just code one loop for e.g. names ([_a-z][_a-z0-9]*) and integers / floats etc. 6) What's with the strange (*--ptr), overwriting previous work. Another bug waiting to happen. 7) Why are we returning out + skipped? Returning the consumed characters? That could just be coded in the parser input abstraction (where you would also keep track of the current file offset / possibly line and column (but you can compute those when you need them)). 8) What about the ferror(src) ? (skipped - out) : (out - skipped)? 9) 3 locations where a zero-terminator is written, when there should be _at most_ 1 location, but probably none. If the code wouldn't do 2), the bug you describe would probably never have happened. The return value from fgetc() is int, not char. It has to be larger than char to be able to return a char as well as EOF. I would probably code something along the lines of void identifier(Parser *parser) { My_String_Builder *builder = get_string_builder(parser); reset_string_builder(builder); for (;;) { string_builder_push(builder, c); if (! next_byte(parser)) break; int c = parser_get(parser); if (!(is_alpha(c) || c == '_' || ...)) break; } String *string = string_builder_finalize(builder); // check string builder "overflow" / too long / further input sanitization push_string_token(parser, TOKEN_IDENTIFIER, string); }
- mynegation 3y agoI almost got it. I was like “whoa wait, does not fgetc return int? You truncate it to char, can you even compare it to EOF constant now?”. I did not remember the actual EOF constant or that char is unsigned on ARM, but I think I would pay attention to the compiler throwing a warning at me (it does throw a warning, does it not?)
- WesolyKubeczek 3y agoOne thing I'm seeing: fgetc returns an int, so on a big-endian architecture the `*ptr = fgetc(src);` might yield even more interesting results. Then, that fgetc() returns EOF on either an EOF or error is its major weakness — mixing in data and control all in-band — one is supposed to check with feof() and ferror() which of those two have happened, and if neither is true, then it's a data byte that just happens to be equal to the EOF constant. (This makes it quite pessimal when you are reading in a file that happens to contain lots of bytes equal to EOF, though). > If instead you're on a system like ARM where char is unsigned, then you can never read EOF. Then you should probably use an intermediate variable which is a proper int, gives you a surefire way to tell 0x000000ff and 0xffffffff apart. But it's all gotchas like this which make me cringe every time someone suggests C is oh so very good and "simple", or even (gasp!) "convenient" to work with strings in general and text in particular. It's anything but. Hell, maybe it was better than alternatives back in some 1976, but then awk and Perl got invented, and Python followed soon after.
- dalke 3y agoYears ago I ported some software from IRIX to some Linux-based OS. There were a few differences in the result. I eventually tracked it down to how on IRIX char is unsigned while the Linux-based OS used signed char. The program used a 'char' to store the value of the formal atomic charge of an atom, which is typically 0, but for the sorts of chemistry I deal with can be +1, +2, -1, or -2, so it makes sense to allocate only 8 bits to store the 'char'ge. :) On IRIX, where they had deployed the code for years, the charges were actually being interpreted as 0, +1, +2, 255, and 254. Going back to the code example you gave, isn't there also an issue if read failure occurs on the first byte? Then `out` is 0, returning -0, which is 0, giving no way for the caller to distinguish between EOF and read failure.