Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Saving SDT_INTLIST handle unsigned values properly #7396

Merged
merged 3 commits into from Feb 6, 2020

Conversation

@Eddi-z
Copy link
Contributor

Eddi-z commented Mar 22, 2019

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:

[22.03.19 00:49] <peter1138> colour_presets = -1957633922,-1336168961,-1790426469,-1784986981,1451692562,0,0,2077322240,0,0,0,0,0,0,0,0

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)

@Eddi-z Eddi-z force-pushed the Eddi-z:settingslist branch 2 times, most recently from 2bfcd9e to 052f539 Mar 22, 2019
if (p == end) return -1; // invalid character (not a number)
if (sizeof(int) < sizeof(long)) v = ClampToI32(v);

This comment has been minimized.

Copy link
@Eddi-z

Eddi-z Mar 22, 2019

Author Contributor

i'm unsure about this change. it now mimics the behaviour in StringToVal, however i feel like some validation of range should happen.

This comment has been minimized.

Copy link
@LordAro

LordAro Apr 14, 2019

Member

Hmm yes, bit weird to be removing this. Perhaps it should be added to StringToVal?
Condition could probably be sanely removed though..

src/settings.cpp Outdated Show resolved Hide resolved
@Eddi-z Eddi-z force-pushed the Eddi-z:settingslist branch 2 times, most recently from dde90cc to e2ca55d Mar 24, 2019
@@ -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);

This comment has been minimized.

Copy link
@LordAro

LordAro Apr 14, 2019

Member

surprised this doesn't cause a warning elsewhere (e.g. line192), given this has gone from signed to unsigned...

This comment has been minimized.

Copy link
@LordAro

LordAro Apr 14, 2019

Member

Also, shouldn't this be unsigned long instead of size_t ? They're not necessarily the same...

This comment has been minimized.

Copy link
@nielsmh

nielsmh Feb 6, 2020

Contributor

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);

This comment has been minimized.

Copy link
@LordAro

LordAro Apr 14, 2019

Member

Hmm yes, bit weird to be removing this. Perhaps it should be added to StringToVal?
Condition could probably be sanely removed though..

@stale

This comment has been minimized.

Copy link

stale bot commented May 14, 2019

This pull request has been automatically marked as stale because it has not had any activity in the last month.
Please feel free to give a status update now, ping for review, or re-open when it's ready.
It will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@stale stale bot added the stale label May 14, 2019
@LordAro LordAro added waiting on author and removed stale labels Aug 17, 2019
@LordAro LordAro added this to the 1.10.0 milestone Nov 23, 2019
@nielsmh nielsmh force-pushed the Eddi-z:settingslist branch from e2ca55d to 39dc79c Feb 6, 2020
@nielsmh

This comment has been minimized.

Copy link
Contributor

nielsmh commented Feb 6, 2020

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.

@nielsmh nielsmh force-pushed the Eddi-z:settingslist branch from 39dc79c to ee7b85e Feb 6, 2020
@nielsmh nielsmh requested a review from LordAro Feb 6, 2020
@LordAro
LordAro approved these changes Feb 6, 2020
@LordAro LordAro merged commit 1072837 into OpenTTD:master Feb 6, 2020
8 checks passed
8 checks passed
Commit checker
Details
OpenTTD CI Build #20200206.9 succeeded
Details
OpenTTD CI (Linux linux-amd64-clang-3.9) Linux linux-amd64-clang-3.9 succeeded
Details
OpenTTD CI (Linux linux-amd64-gcc-6) Linux linux-amd64-gcc-6 succeeded
Details
OpenTTD CI (Linux linux-i386-gcc-6) Linux linux-i386-gcc-6 succeeded
Details
OpenTTD CI (MacOS) MacOS succeeded
Details
OpenTTD CI (Windows Win32) Windows Win32 succeeded
Details
OpenTTD CI (Windows Win64) Windows Win64 succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.