Skip to content

Fix config rewrite producing negative values for unsigned memory configs#3440

Merged
enjoy-binbin merged 4 commits intovalkey-io:unstablefrom
enjoy-binbin:fix_overflow
Apr 9, 2026
Merged

Fix config rewrite producing negative values for unsigned memory configs#3440
enjoy-binbin merged 4 commits intovalkey-io:unstablefrom
enjoy-binbin:fix_overflow

Conversation

@enjoy-binbin
Copy link
Copy Markdown
Member

@enjoy-binbin enjoy-binbin commented Apr 3, 2026

rewriteConfigFormatMemory() and rewriteConfigBytesOption() used long long
parameters to represent memory bytes, but configs like maxmemory are stored
as unsigned long long and allow values up to ULLONG_MAX.

When the value exceeds LLONG_MAX (e.g. 9223372036854775808), it overflows to
negative when passed as long long, causing config rewrite to produce incorrect
output like "maxmemory -8589934592gb".

Change both functions to use unsigned long long, matching the actual semantics
of memory byte values. This is consistent with how numericConfigGet() already
handles MEMORY_CONFIG using ull2string().

There are some MEMORY_CONFIG are signed and can be negative:

  1. maxmemory-clients is a PERCENT_CONFIG
  2. slot-migration-max-failover-repl-bytes is a SIGNED_MEMORY_CONFIG (after Fix slot-migration-max-failover-repl-bytes unable to accept -1 #3443)

None of them were affected:

static void numericConfigRewrite(standardConfig *config, const char *name, struct rewriteConfigState *state) {
    ...
    if (config->data.numeric.flags & PERCENT_CONFIG && value < 0) {
        rewriteConfigPercentOption(state, name, -value, config->data.numeric.default_value);
    } else if (config->data.numeric.flags & SIGNED_MEMORY_CONFIG && value < 0) {
        rewriteConfigNumericalOption(state, name, value, config->data.numeric.default_value);
    } else if (config->data.numeric.flags & MEMORY_CONFIG) {
        rewriteConfigBytesOption(state, name, value, config->data.numeric.default_value);
    ...

rewriteConfigFormatMemory() and rewriteConfigBytesOption() used long long
parameters to represent memory bytes, but configs like maxmemory are stored
as unsigned long long and allow values up to ULLONG_MAX.

When the value exceeds LLONG_MAX (e.g. 9223372036854775808), it overflows to
negative when passed as long long, causing config rewrite to produce incorrect
output like "maxmemory -8589934592gb".

Change both functions to use unsigned long long, matching the actual semantics
of memory byte values. This is consistent with how numericConfigGet() already
handles MEMORY_CONFIG using ull2string().

Signed-off-by: Binbin <binloveplay1314@qq.com>
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 3, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.49%. Comparing base (dba128f) to head (ee075c1).
⚠️ Report is 4 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #3440      +/-   ##
============================================
- Coverage     76.52%   76.49%   -0.03%     
============================================
  Files           157      157              
  Lines         79035    79035              
============================================
- Hits          60478    60455      -23     
- Misses        18557    18580      +23     
Files with missing lines Coverage Δ
src/config.c 78.33% <100.00%> (+0.31%) ⬆️

... and 25 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

Some MEMORY_CONFIG are signed and can be negative:

src/config.c:    createSSizeTConfig("maxmemory-clients", NULL, MODIFIABLE_CONFIG, -100, SSIZE_MAX, server.maxmemory_clients, 0, MEMORY_CONFIG | PERCENT_CONFIG, NULL, applyClientMaxMemoryUsage),
src/config.c:    createSSizeTConfig("slot-migration-max-failover-repl-bytes", NULL, MODIFIABLE_CONFIG, -1, SSIZE_MAX, server.slot_migration_max_failover_repl_bytes, 0, MEMORY_CONFIG, NULL, NULL),

@enjoy-binbin
Copy link
Copy Markdown
Member Author

Some MEMORY_CONFIG are signed and can be negative:

@zuiderkwast Yes, but it does not affect since we will rewrite PERCENT_CONFIG first, see

static void numericConfigRewrite(standardConfig *config, const char *name, struct rewriteConfigState *state) {
    long long value = 0;

    GET_NUMERIC_TYPE(value)

    if (config->data.numeric.flags & PERCENT_CONFIG && value < 0) {
        rewriteConfigPercentOption(state, name, -value, config->data.numeric.default_value);
    } else if (config->data.numeric.flags & MEMORY_CONFIG) {
        rewriteConfigBytesOption(state, name, value, config->data.numeric.default_value);
    }
    ...
}

Signed-off-by: Binbin <binloveplay1314@qq.com>
Comment thread tests/unit/introspection.tcl Outdated
Signed-off-by: Binbin <binloveplay1314@qq.com>
@zuiderkwast
Copy link
Copy Markdown
Contributor

Some MEMORY_CONFIG are signed and can be negative:

@zuiderkwast Yes, but it does not affect since we will rewrite PERCENT_CONFIG first,

But slot-migration-max-failover-repl-bytes is not a percent config.

Signed-off-by: Binbin <binloveplay1314@qq.com>
@enjoy-binbin
Copy link
Copy Markdown
Member Author

But slot-migration-max-failover-repl-bytes is not a percent config.

@zuiderkwast So after #3443, does this slove you concern?

@enjoy-binbin
Copy link
Copy Markdown
Member Author

The other issue is:

127.0.0.1:6379> config set maxmemory 9223372036854775808
OK
127.0.0.1:6379> config set maxmemory-clients 100%
OK
127.0.0.1:6379> config set maxmemory-clients 9223372036854775808
(error) ERR CONFIG SET failed (possibly related to argument 'maxmemory-clients') - percentage argument must be less or equal to 100

I think we can leave it as it is for now.

@enjoy-binbin enjoy-binbin merged commit 890b82a into valkey-io:unstable Apr 9, 2026
57 of 58 checks passed
@enjoy-binbin enjoy-binbin deleted the fix_overflow branch April 9, 2026 02:43
@github-project-automation github-project-automation Bot moved this to To be backported in Valkey 9.1 Apr 9, 2026
@github-project-automation github-project-automation Bot moved this to To be backported in Valkey 9.0 Apr 9, 2026
@zuiderkwast zuiderkwast added the release-notes This issue should get a line item in the release notes label Apr 9, 2026
sarthakaggarwal97 pushed a commit to sarthakaggarwal97/valkey that referenced this pull request Apr 16, 2026
…igs (valkey-io#3440)

rewriteConfigFormatMemory() and rewriteConfigBytesOption() used long long
parameters to represent memory bytes, but configs like maxmemory are stored
as unsigned long long and allow values up to ULLONG_MAX.

When the value exceeds LLONG_MAX (e.g. 9223372036854775808), it overflows to
negative when passed as long long, causing config rewrite to produce incorrect
output like "maxmemory -8589934592gb".

Change both functions to use unsigned long long, matching the actual semantics
of memory byte values. This is consistent with how numericConfigGet() already
handles MEMORY_CONFIG using ull2string().

There are some MEMORY_CONFIG are signed and can be negative:
1. maxmemory-clients is a PERCENT_CONFIG
2. slot-migration-max-failover-repl-bytes is a SIGNED_MEMORY_CONFIG (after valkey-io#3443) 

None of them were affected:
```
static void numericConfigRewrite(standardConfig *config, const char *name, struct rewriteConfigState *state) {
    ...
    if (config->data.numeric.flags & PERCENT_CONFIG && value < 0) {
        rewriteConfigPercentOption(state, name, -value, config->data.numeric.default_value);
    } else if (config->data.numeric.flags & SIGNED_MEMORY_CONFIG && value < 0) {
        rewriteConfigNumericalOption(state, name, value, config->data.numeric.default_value);
    } else if (config->data.numeric.flags & MEMORY_CONFIG) {
        rewriteConfigBytesOption(state, name, value, config->data.numeric.default_value);
    ...
```

Signed-off-by: Binbin <binloveplay1314@qq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-notes This issue should get a line item in the release notes

Projects

Status: To be backported
Status: To be backported

Development

Successfully merging this pull request may close these issues.

3 participants