Skip to content

Fix reply bytes calculation error on 32bit platform #7299

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.

Sign up for GitHub

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

Jump to bottom
Merged
antirez merged 1 commit into from
May 21, 2020
Merged

Fix reply bytes calculation error on 32bit platform #7299

antirez merged 1 commit into from
May 21, 2020

Conversation

@ShooterIT
Copy link
Member

@ShooterIT ShooterIT commented May 21, 2020

Fix #7275.
For this line https://github.com/antirez/redis/blob/unstable/src/networking.c#L491
On 32bit platform, the type of c->reply_bytes is unsigned long long, so it is 8 bytes,
the types of tail->size and old_size are size_t, so it is 4 bytes, old_size is exactly bigger that tail->size, so the result of tail->size - old_size is very big.

https://stackoverflow.com/questions/7221409/is-unsigned-integer-subtraction-defined-behavior

But c->reply_bytes is 8 bytes, the result will be add at last 4 bytes in c->reply_bytes, so c->reply_bytes is very big. Redis will coredump when execute serverAssert(c->reply_bytes < SIZE_MAX-(1024*64));

BTW, for current unstable branch, tests will exit with errors if we make 32bit and sh runtest.

There is a test, output is 4294967395.

#include <stdio.h>

int main(void) {
    unsigned long long a;
    unsigned int b, c;

    a = 100;
    b = 1;
    c = 2;
    a += b-c;
    printf("%llu\n", a);

    return 0;
}

@antirez @oranagra

@antirez antirez merged commit 9f932bc into redis:unstable May 21, 2020
@antirez
Copy link
Contributor

antirez commented May 21, 2020

Thank you @ShooterIT, I'll release a new Redis 6 version with this fix ASAP.

@oranagra
Copy link
Member

oranagra commented May 21, 2020

sorry about that. (second embarrassing bug in that simple function).

the lesson learned here (which is similar to the one i learned from the last fuckup in that function), is that our CI lucks coverage for 32bit builds (previous time was libc allocator).

for the record, i think the cleaner fix to keep using +=, but cast it to either unsigned long long or even just ssize_t (in which case there would be a sign extension when performing the +=).

@antirez
Copy link
Contributor

antirez commented May 21, 2020

@oranagra NP, bugs happen. Agreed with the CI. About the different fix, I kinda disagree because I think in this specific case, it is cleaner to avoid that at any given point we compute a negative value. With the proposed fix, when we do value = value + a - b, given that we are sure (value+a) itself will not overflow since it's 64 bits, and even so it's unsigned, so defined semantics, then we remove b and we are set. Your proposed fix works as well, but it is kinda counter intuitive that we compute a negative increment to start with, and then add it to our value. Here I assume that (a-b) is always <= 0.

@oranagra
Copy link
Member

oranagra commented May 21, 2020

well, maybe it's a matter of taste (i preferred to see a delta which i knew may be negative, and then add it).

anyway, i didn't mean we should add another commit, the one we have fixes the bug and that's enough, just wanted to comment.

i'll add CI for 32bit sometime next week.

@antirez
Copy link
Contributor

antirez commented May 21, 2020

@oranagra thanks, btw I think the current fix would be better if there was a comment about that.

@antirez
Copy link
Contributor

antirez commented May 21, 2020

Anyway you know what's fun? That I always thought that actually even if unsigned integers overflow are well defined in C, signed math is much more safe in general. This is a good example. I'll never understand why the ANSI C standard is so terrible about not well-defining signed math in C. It would be straight forward and what 99.99% of the widely used CPUs already do. They are handling C terribly IMHO :-(

@ShooterIT
Copy link
Member Author

ShooterIT commented May 22, 2020 •
edited
Loading

Hi @oranagra Adding CI for 32bit is definitely necessary. For my PR #7090 , I don't want to use temporary variable len at begin but the size is different between off_t and size_t on 32bit macOS, it is dangerous of dereferencing the pointer count with off_t *, so I occurred errors when I did make 32bit and sh runtest without temporary variable len . I think I make much sense to add CI for 32bit platform, that can help us find errors.

image

@ShooterIT
Copy link
Member Author

ShooterIT commented May 22, 2020

Hello @antirez For 64bit, the previous code result is correct yet, because it overflows twice. Just like you said, unsigned integers overflow are well defined in C, but that may hide errors logically.
Angels and demons are together in C language!

@ShooterIT ShooterIT deleted the reply-bytes branch May 22, 2020 03:29
@oranagra
Copy link
Member

oranagra commented May 24, 2020

CI for 32bit: #7315
indeed, when tested it on the version before the fix i got:

10082:M 24 May 2020 05:30:49.722 # Failed assertion: c->reply_bytes < SIZE_MAX-(1024*64) (networking.c:2749)

JackieXie168 pushed a commit to JackieXie168/redis that referenced this pull request Jun 18, 2020
@antirez
Merge pull request redis#7299 from ShooterIT/reply-bytes
ca03f85 
Fix reply bytes calculation error on 32bit platform
@oranagra oranagra mentioned this pull request Jul 28, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Redis 6.0.3 assertion on make test in networking.c: c->reply_bytes < SIZE_MAX-(1024*64)' is not true

3 participants

@ShooterIT @antirez @oranagra