4 ms·
if (isdigit(*str)) { - io_tlb_nslabs = simple_strtoul(str, &str, 0); + str += parse_integer(str, 0, &io_tlb_nslabs); and obvi
by thomasahle 11y ago
if (isdigit(*str)) {
- io_tlb_nslabs = simple_strtoul(str, &str, 0);
+ str += parse_integer(str, 0, &io_tlb_nslabs);
and obviously nobody spent even a *second* asking themselves
"what if parse_integer returns an error".
Can anybody explain why parse_integer would return an error, when isdigit(*str)?
Also, why is the result (presumably integer) added back to the string?
EDIT: Regarding to "adding to the string" I realize that's just C pointer arithmetic.
- username3 11y agoMaybe under some race conditions.
- ajross 11y agoNot sure because I didn't read the parse_integer patches. But the converse is a problem. isdigit("-2") returns false but the value will parse correctly.
- monochromatic 11y agoHow is that a problem though?
- interdrift 11y agoparse_integer takes a string not a char, didn't look at the implementation either but it makes sense to check for the whole string doesnt it)
- deleted 11y ago[deleted]
- gpvos 11y agoI haven't seen the code of parse_integer, but it could be on overflow, i.e., a too large number.
- victorNicollet 11y agoAn overflow may happen if there is an overflow for the type (e.g. reading '300' into an unsigned char). The function returns either a positive number indicating the number of characters successfully read from the string (if it encountered a non-digit character) or a negative number indicating an error code. https://lwn.net/Articles/643159/ https://lwn.net/Articles/643159/
- thomasahle 11y agoI see. And the previous code didn't have this issue?
- deleted 11y ago[deleted]
- sanderjd 11y agoHaven't read any of the relevant code, but if the new code returns a negative number representing an error, it will decrement the `str` pointer, whereas I'm guessing `simple_strtoul` leaves its `str` pointer alone on error.
- metric10 11y agoI don't know about Linux's simple_strtoul(), but the C library's strtoul(3) will return ULONG_MAX and set the string pointer to a valid location. So the old code COULD cause an issue if "io_tlb_nslabs" being equal to ULONG_MAX isn't accounted for later on, but adding -1 (0xffffffffffffffff) to a pointer (as the new code does) is far worse.
- missblit 11y agoI didn't look too closely, but I think it will fail when the string is "0x".
- victorNicollet 11y agoIt only switches to base 16 if there's a digit after the "x". For "0x", it would just parse 0.
- missblit 11y agoAh thanks, I missed the isxdigit(s[2])
- bdarnell 11y agoI haven't looked at the patch, but the error is probably overflow: too many digits to fit in the integer variable. parse_integer() presumably returns the number of bytes consumed, so the str pointer is advanced by that many bytes to point to whatever is after the integer. But on error, it returns a negative number, so str is moved backwards into some nonsensical position.
- saurik 11y ago> And yes, parse_integer() really can return an error, even despite that "isdigit(*str)" check. Think about it. Or just read the source code. Actually taking the advice of the article and looking at the source code makes it clear that parse_integer is doing what we would hope such an API would: it guarantees that the resulting number fits into the provided integer. https://lwn.net/Articles/643159/ https://lwn.net/Articles/643159/
- mikeash 11y agoReturning an error is fine, but the code that calls the function doesn't check for errors, and just blindly applies the return value as an offset to a pointer. When the function returns -ERANGE to indicate that the value was too large for the requested data type, entertaining things will occur.
- detaro 11y agoEDIT: I'm wrong, victorNicollet spotted my mistake. I thought that: str == "09" fails. Leading zero indicates base 8 -> 9 is an invalid digit. But that is parsed as "0" and the 9 is left in the string.
- jandrese 11y agoIMHO, "leading zero indicates octal" is as old as it is wrong. There are a handful of cases where this is helpful, and an ocean of cases where it breaks things unexpectedly because someone tried inputting prettyprinted data. At least with Hex you can't mistake the 0x prefix for anything else. One of the best features of inet_pton over inet_addr is that the former doesn't do automatic conversions and instead assumes every field is a decimal number. The latter has a hidden trap where 192.168.0.11 is different than 192.168.000.011.
- victorNicollet 11y agoI agree. Still, that's the behaviour of parse_integer (specifically, base determination in _parse_integer_fixup_radix).
- detaro 11y agoIt is the specified behavior of the discussed code when not passing in a specified base to parse_integer. Now if that is a needed feature in the example context (or a fixed base of 10 would work as well) I don't know. It is an odd convention though, I agree. Hex I somewhat get, but octal?
- acveilleux 11y agoYeah, so it's a wrong thing in 2015 where the only significant use of octal is in unix permissions which themselves are only octal because of PDP-7/8. The 9/18/36-bits architectures have lost but octal conventions of C was made on the PDP-7 (18 bits) and until the early 1980s, 36-bit machines were commercially available (PDP-10.)
- SixSigma 11y ago
- raverbashing 11y agoBecause isdigit is checking only the first digit, and parse_integer is parsing not only that one, but the following digits as well
- victorNicollet 11y agoparse_integer stops reading when it encounters a non-digit character (in order to support reading multiple integers from the same string).
- jonathlee 11y agoIf isdigit() is the standard library function, that takes/checks a single character. However, parse_integer() takes a full string and parses until it reaches a null or newline. The first character could definitely be a valid digit but the rest of the string could cause a problem. However, I am not a C programmer, so my internet search-guided intuition may be wrong.
- deleted 11y ago[deleted]