Fix strto* usage by setting errno before-hand and interpreting EINVAL properly #4598
Conversation
EINVAL may be returned when a conversion cannot be performed. We should still go on to try and validate the value as a string-option, if it's one of those kinds of options.
|
Can one of the admins verify this patch? |
…ng errno Leave the declarations behind at the beginning of scope so we can reset errno just before calling the strto* functions without breaking older compilers.
|
@akallabeth Thanks! I've corrected all instances of mid-scope declaration, I believe. |
| type = strtol(arg->Value, &pEnd, 10); | ||
|
|
||
| if (errno != 0) | ||
| if ((errno != 0) && (errno != EINVAL)) |
There was a problem hiding this comment.
It seems simpler to check against known string values first and use number conversion as a fallback. That way EINVAL doesn't need to be excluded, so something like "invalid" won't be treated as "none".
There was a problem hiding this comment.
I wasn't aiming to make any functional changes here- just to make it work with a more POSIX-compliant (in this regard) libc.
I can make this change if you'd like, though- the original submitter of the previously mentioned issue had a patch that also validated type before it was later passed to a function in case it failed both numeric parsing and wasn't one of the present values, but I had intentionally left that out as I considered it ESCOPE for the moment.
| unsigned long val = strtoul(&p[1], NULL, 0); | ||
| unsigned long val; | ||
| errno = 0; | ||
| val = strtoul(&p[1], NULL, 0); |
There was a problem hiding this comment.
I'd suggest to introduce wrapper around strtoul() instead, something like:
BOOL parse_ulong(const char* str, unsigned long* result);
That way errno handling can be kept in one place.
| @@ -659,6 +659,7 @@ get_elf_hwcap_from_proc_cpuinfo(const char* cpuinfo, int cpuinfo_len) | |||
|
|
|||
| if (cpuArch) | |||
| { | |||
| errno = 0; | |||
There was a problem hiding this comment.
No one is checking "errno" after "strtol" here.
| @@ -454,6 +454,7 @@ static BOOL http_response_parse_header_status_line(HttpResponse* response, char* | |||
| if ((errno != 0) || (val < 0) || (val > INT16_MAX)) | |||
| return FALSE; | |||
|
|
|||
| errno = 0; | |||
| response->StatusCode = strtol(status_code, NULL, 0); | |||
There was a problem hiding this comment.
It should be "response->StatusCode = val;" instead, "status_code" was already parsed into "val" on line 452.
There was a problem hiding this comment.
Right, I didn't apply any thought to whether the context made any sense- this was kind of a low priority for me as we're preparing for an upcoming release over in our world. =) ETIME, as it goes. =(
There was a problem hiding this comment.
Oh, missed that one, thx for pointing that out @p-pautov
|
@freerdp-bot test |
|
Refer to this link for build results (access rights to CI server needed): |
| @@ -454,6 +454,7 @@ static BOOL http_response_parse_header_status_line(HttpResponse* response, char* | |||
| if ((errno != 0) || (val < 0) || (val > INT16_MAX)) | |||
| return FALSE; | |||
|
|
|||
| errno = 0; | |||
| response->StatusCode = strtol(status_code, NULL, 0); | |||
There was a problem hiding this comment.
Oh, missed that one, thx for pointing that out @p-pautov
For glibc, the previous usage of these things all worked. As pointed out in #4597, these are not POSIXly correct invocations. (See the first two comments for a description of the problem and further diagnosis)
Further examination after issue #4597 revealed that not all invocations were incorrect; some were assumed to be on numeric strings and thus didn't check errno, while others were properly setting errno beforehand. This PR adds more errno resets and fixes the two instances of not accepting EINVAL that existed in the cmdline parsing.
I wrote a small Coccinelle patch (available here [1]) to find some of these, while others I found by hand. My Coccinelle skills suck, though, so I ended up with some false positives that I had to correct.
[1] https://people.freebsd.org/~kevans/freerdp-strto.cocci