Fix: Saving SDT_INTLIST handle unsigned values properly #7396
Conversation
Eddi-z
force-pushed
the
settingslist
branch
2 times, most recently
from
2bfcd9e to
052f539
Compare
| if (p == end) return -1; // invalid character (not a number) | ||
| if (sizeof(int) < sizeof(long)) v = ClampToI32(v); |
There was a problem hiding this comment.
i'm unsure about this change. it now mimics the behaviour in StringToVal, however i feel like some validation of range should happen.
There was a problem hiding this comment.
Hmm yes, bit weird to be removing this. Perhaps it should be added to StringToVal?
Condition could probably be sanely removed though..
Eddi-z
force-pushed
the
settingslist
branch
2 times, most recently
from
dde90cc to
e2ca55d
Compare
src/settings.cpp
Outdated
| @@ -187,9 +187,8 @@ static int ParseIntList(const char *p, int *items, int maxitems) | |||
| default: { | |||
| if (n == maxitems) return -1; // we don't accept that many numbers | |||
| char *end; | |||
| long v = strtol(p, &end, 0); | |||
| size_t v = strtoul(p, &end, 0); | |||
There was a problem hiding this comment.
surprised this doesn't cause a warning elsewhere (e.g. line192), given this has gone from signed to unsigned...
There was a problem hiding this comment.
Also, shouldn't this be unsigned long instead of size_t ? They're not necessarily the same...
There was a problem hiding this comment.
I think the entire SDT_INTLIST handling might just as well be changed to only do unsigned. As far as I can tell no uses of it need signed values anyway.
| if (p == end) return -1; // invalid character (not a number) | ||
| if (sizeof(int) < sizeof(long)) v = ClampToI32(v); |
There was a problem hiding this comment.
Hmm yes, bit weird to be removing this. Perhaps it should be added to StringToVal?
Condition could probably be sanely removed though..
|
This pull request has been automatically marked as stale because it has not had any activity in the last month. |
|
I made a template out of ParseIntList since it was being used with two different array types, and changed it to be uinsigned almost all the way through. I don't think there's any cases where it's used with signed values, so that's probably safer regardless. Overall, this should have less weird casting and have less potential UB. |
This PR addresses a line that @PeterN wrote in the chat when he was trying out code to save a colour preset to the config file:
The first issue is that the values are written as signed, even though they are unsigned in the code. The first commit handles writing these values correctly, the second is about correctly reading the (now out-of-range) values back in
the third commit is a feature to write long numbers like this as hexadecimal (reading should already be possible)