Skip to content

sds.c: Fix potential overflow in sdsll2str.#8910

Merged
oranagra merged 1 commit intoredis:unstablefrom
enjoy-binbin:fix_potential_overflow
Aug 8, 2021
Merged

sds.c: Fix potential overflow in sdsll2str.#8910
oranagra merged 1 commit intoredis:unstablefrom
enjoy-binbin:fix_potential_overflow

Conversation

@enjoy-binbin
Copy link
Contributor

@enjoy-binbin enjoy-binbin commented May 5, 2021

i saw a ref issue: #4110

I did some tests, the test code:

    sds max_s = sdsfromlonglong(LLONG_MAX);
    printf("LLONG_MAX: %lld\n", LLONG_MAX);
    printf("LLONG_MAX to sds: %s\n", max_s);

    sds min_s = sdsfromlonglong(LLONG_MIN);
    printf("LLONG_MIN: %lld\n", LLONG_MIN);
    printf("LLONG_MIN to sds: %s\n", min_s);

    unsigned long long v;
    v = -LLONG_MIN;
    printf("-LLONG_MIN %llu\n", v);

output: The output is fine

LLONG_MAX: 9223372036854775807
LLONG_MAX to sds: 9223372036854775807
LLONG_MIN: -9223372036854775808
LLONG_MIN to sds: -9223372036854775808
-LLONG_MIN 9223372036854775808

But there will be a warning when compiling. So i think better have this fixed?

warning: integer overflow in expression ‘-9223372036854775808’ of type ‘long long int’ results in ‘-9223372036854775808’ [-Woverflow]

unsigned long long v;
v = -LLONG_MIN 

Maybe. I realize it's not very needed or necessary... Feel free to close it and the ref issue

@enjoy-binbin
Copy link
Contributor Author

from #9310 ping @oranagra

@oranagra oranagra linked an issue Aug 5, 2021 that may be closed by this pull request
Copy link
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm on the fence here.

on one hand, having code that relies on undefined behavior is very dangerous.
even if the compiler does the right thing usually, the optimizer can make some assumption and mess up.

on the other hand, i'm quite certain that all compilers handle this right, and such "fixes" make the code harder to read.

I do see that Antirez added similar fix in the past (c951c3e), so i suppose he would have merged this.
@yossigo WDYT?

@oranagra oranagra added the state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten label Aug 5, 2021
Copy link
Collaborator

@yossigo yossigo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oranagra LGTM, maybe we want this as a utility function to address code readability concerns?

@oranagra oranagra merged commit e1dc979 into redis:unstable Aug 8, 2021
@enjoy-binbin enjoy-binbin deleted the fix_potential_overflow branch August 8, 2021 11:56
JackieXie168 pushed a commit to JackieXie168/redis that referenced this pull request Sep 8, 2021
Fixes an undefined behavior, same way as our `ll2string` does.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Possible undefined behavior in sdsll2str() implementation in sds.c.

3 participants