Skip to content

Prevent negative expire parameter in HEXPIRE/HEXPIREAT/HPEXPIRE/HPEXPIREAT commands#13310

Merged
sundb merged 7 commits intoredis:unstablefrom
sundb:uncessary_expire_range_check
Jun 4, 2024
Merged

Prevent negative expire parameter in HEXPIRE/HEXPIREAT/HPEXPIRE/HPEXPIREAT commands#13310
sundb merged 7 commits intoredis:unstablefrom
sundb:uncessary_expire_range_check

Conversation

@sundb
Copy link
Collaborator

@sundb sundb commented Jun 2, 2024 •
edited
Loading

  1. Don't allow HEXPIRE/HEXPIREAT/HPEXPIRE/HPEXPIREAT command expire parameters is negative

  2. Remove a dead code reported from Coverity.
    when unit is not UNIT_SECONDS, the second if (expire > (long long) EB_EXPIRE_TIME_MAX) will be dead code.

# t_hash.c
2988    /* Check expire overflow */
      	cond_at_most: Condition expire > 281474976710655LL, taking false branch. Now the value of expire is at most 281474976710655.
2989    if (expire > (long long) EB_EXPIRE_TIME_MAX) {
2990        addReplyErrorExpireTime(c);
2991        return;
2992    }

2994    if (unit == UNIT_SECONDS) {
2995        if (expire > (long long) EB_EXPIRE_TIME_MAX / 1000) {
2996            addReplyErrorExpireTime(c);
2997            return;
2998        }
2999        expire *= 1000;
3000    } else {
      	at_most: At condition expire > 281474976710655LL, the value of expire must be at most 281474976710655.
      	dead_error_condition: The condition expire > 281474976710655LL cannot be true.
3001        if (expire > (long long) EB_EXPIRE_TIME_MAX) {
      	
CID 494223: (#1 of 1): Logically dead code (DEADCODE)
dead_error_begin: Execution cannot reach this statement: addReplyErrorExpireTime(c);.
3002            addReplyErrorExpireTime(c);
3003            return;
3004        }
3005    }

@sundb sundb requested a review from moticless June 2, 2024 03:37
@tezc
Copy link
Collaborator

tezc commented Jun 3, 2024

LGTM but there is one related issue. I see we allow negative values for expire time and don't do overflow check correctly.

@sundb @moticless Currently, expire command allows negative values. Do we want to replicate this behavior for hexpire? I see expire time was described as non-negative integer in PRD. Was there any discussion about it?

@moticless
Copy link
Collaborator

moticless commented Jun 3, 2024 •
edited
Loading

@tezc, unlike EXPIRE, I think we are free here to make it in the right way. Maximum expiration time is already not aligned, which is more important. @LiorKogan WDYT?

@sundb
Copy link
Collaborator Author

sundb commented Jun 3, 2024

@moticless the expireAt is uint64_t, if expire is negative, expireAt will overflow.

@LiorKogan
Copy link
Member

LiorKogan commented Jun 3, 2024

We should error on out-of-range values (negative values and values that translates to > 1/1/10000AD 00:00:00 UTC)

@tezc
Copy link
Collaborator

tezc commented Jun 3, 2024

@sundb do you feel like adding negative check as part of this PR? Otherwise, we'll fix on a follow up PR.

tezc
tezc previously approved these changes Jun 3, 2024
View reviewed changes
@sundb
Copy link
Collaborator Author

sundb commented Jun 3, 2024

@tezc yes, i'm working on it. this dead code cleanup has become secondary.

@sundb sundb changed the title Remove dead code for checking expire overflow Prevent negative expire parameters in HEXPIRE/HEXPIREAT/HPEXPIRE/HPEXPIREAT commands Jun 3, 2024
sundb
sundb commented Jun 3, 2024
View reviewed changes
tezc
tezc reviewed Jun 3, 2024
View reviewed changes
@sundb sundb changed the title Prevent negative expire parameters in HEXPIRE/HEXPIREAT/HPEXPIRE/HPEXPIREAT commands Prevent negative expire parameter in HEXPIRE/HEXPIREAT/HPEXPIRE/HPEXPIREAT commands Jun 4, 2024
tezc
tezc previously approved these changes Jun 4, 2024
View reviewed changes
moticless
moticless previously approved these changes Jun 4, 2024
View reviewed changes
@sundb sundb dismissed stale reviews from moticless and tezc via d54bb77 June 4, 2024 12:20
@sundb sundb requested a review from moticless June 4, 2024 12:21
@sundb sundb merged commit 9a2c6ba into redis:unstable Jun 4, 2024
@sundb sundb deleted the uncessary_expire_range_check branch June 4, 2024 12:35
funny-dog pushed a commit to funny-dog/redis that referenced this pull request Sep 17, 2025
@sundb @tezc
Prevent negative expire parameter in HEXPIRE/HEXPIREAT/HPEXPIRE/HPEXP…
98d092f 
…IREAT commands (redis#13310)

1. Don't allow HEXPIRE/HEXPIREAT/HPEXPIRE/HPEXPIREAT command expire
parameters is negative

2. Remove a dead code reported from Coverity.
when `unit` is not `UNIT_SECONDS`, the second `if (expire > (long long)
EB_EXPIRE_TIME_MAX)` will be dead code.
```c
# t_hash.c
2988    /* Check expire overflow */
      	cond_at_most: Condition expire > 281474976710655LL, taking false branch. Now the value of expire is at most 281474976710655.
2989    if (expire > (long long) EB_EXPIRE_TIME_MAX) {
2990        addReplyErrorExpireTime(c);
2991        return;
2992    }

2994    if (unit == UNIT_SECONDS) {
2995        if (expire > (long long) EB_EXPIRE_TIME_MAX / 1000) {
2996            addReplyErrorExpireTime(c);
2997            return;
2998        }
2999        expire *= 1000;
3000    } else {
      	at_most: At condition expire > 281474976710655LL, the value of expire must be at most 281474976710655.
      	dead_error_condition: The condition expire > 281474976710655LL cannot be true.
3001        if (expire > (long long) EB_EXPIRE_TIME_MAX) {
      	
CID 494223: (redis#1 of 1): Logically dead code (DEADCODE)
dead_error_begin: Execution cannot reach this statement: addReplyErrorExpireTime(c);.
3002            addReplyErrorExpireTime(c);
3003            return;
3004        }
3005    }
```

---------

Co-authored-by: Ozan Tezcan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@moticless moticless moticless approved these changes

@tezc tezc tezc left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sundb @tezc @moticless @LiorKogan