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 overflow of rdbWriteRaw return value #8306

Merged
merged 1 commit into from
Jan 12, 2021

Conversation

liguangbo
Copy link
Contributor

Fixes #8299

Just change the rdbWriteRaw return value type from int to ssize_t.

This function is called about 20 places. Most of caller's input parameter len is in the range of int, so this change will just act on the function rdbSaveLzfBlob and function rdbSaveRawString when a string(>2GB) need to be written to rdb.

src/rdb.c Outdated Show resolved Hide resolved
@ShooterIT
Copy link
Collaborator

Hi @oranagra I think it makes sense, and we need to backport it to 6.0 because we already allow users to set proto-max-bulk-len more than 512MB https://github.com/redis/redis/blob/unstable/src/config.c#L2490

@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 Jan 11, 2021
@oranagra
Copy link
Member

yes, let's fix it.
but first i wanna make sure we understand the implications of the bug.
all of this will eventually propagate to the call to rdbSaveObject which only checks if the return value is -1, and ignores any other value.
so the only bug here is that if the user saved exactly 4gb, the int return value would be -1, and fail the rdb saving.
is that right?

@sundb
Copy link
Collaborator

sundb commented Jan 11, 2021

@ShooterIT Redis officially supports a maximum key and value of 512M, I think int(>2G) should be enough.

@liguangbo
Copy link
Contributor Author

liguangbo commented Jan 12, 2021

@oranagra if the user saved exactly 4G, the int return value would be -1, that would fail the rdb saving directly.
If the user saved >=2G and <4G(be inaccurate because some len and states code need saving together) after lzf compressed, int return value would be another negative number, the caller of rdbSaveLzfStringObject would treat save result as data can't be compressed. Then save the data on the old way, that will broken rdb.

/* Try LZF compression - under 20 bytes it's unable to compress even
     * aaaaaaaaaaaaaaaaaa so skip it */
    if (server.rdb_compression && len > 20) {
        n = rdbSaveLzfStringObject(rdb,s,len);
        if (n == -1) return -1;
        if (n > 0) return n;
        /* Return value of 0 means data can't be compressed, save the old way */
    }

@oranagra oranagra merged commit 542455c into redis:unstable Jan 12, 2021
@ShooterIT
Copy link
Collaborator

negative number is represented by complement in computer system, so the length of data is (1UL<<N)-1 (N>=32), the return value will be -1.

#include <stdio.h>
int main(void) {
    printf("%d\r\n",(int)((1UL<<32)-1));
}

./a.out
-1

The return value of rdbSaveLzfBlob is not right when length of lzf compressed data is more than 2GB because
of rdbWriteRaw(rdb,data,compress_len)

ssize_t rdbSaveLzfBlob(rio *rdb, void *data, size_t compress_len,
                       size_t original_len) {
    unsigned char byte;
    ssize_t n, nwritten = 0;

    /* Data compressed! Let's save it on disk */
    byte = (RDB_ENCVAL<<6)|RDB_ENC_LZF;
    if ((n = rdbWriteRaw(rdb,&byte,1)) == -1) goto writeerr;
    nwritten += n;

    if ((n = rdbSaveLen(rdb,compress_len)) == -1) goto writeerr;
    nwritten += n;

    if ((n = rdbSaveLen(rdb,original_len)) == -1) goto writeerr;
    nwritten += n;

    if ((n = rdbWriteRaw(rdb,data,compress_len)) == -1) goto writeerr;
    nwritten += n;

    return nwritten;

writeerr:
    return -1;
}

@oranagra oranagra added the release-notes indication that this issue needs to be mentioned in the release notes label Jan 12, 2021
This was referenced Jan 12, 2021
oranagra pushed a commit that referenced this pull request Jan 12, 2021
Saving string of more than 2GB to the RDB file, can result in corrupt RDB, or failure in rdbSave.
S

(cherry picked from commit 542455c)
JackieXie168 pushed a commit to JackieXie168/redis that referenced this pull request Mar 2, 2021
Saving string of more than 2GB to the RDB file, can result in corrupt RDB, or failure in rdbSave.
S
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes indication that this issue needs to be mentioned in the release notes 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.

[BUG] rdbWriteRaw retval overflow
4 participants