Fix issues with server.allow_access_expired by Jam804 · Pull Request #14262 · redis/redis

Jam804 · 2025-08-08T09:34:42Z

Close #14214

When the server.allow_access_expired flag is set to 1, it allows access to expired keys that have not yet been evicted. All places involving access to expired keys should consider the impact of this parameter.
The modifications involve five methods: hfieldIsExpired, hashTypeNext, hashTypeLength, keyIsExpired, and hashTypeIsExpired. When the server.allow_access_expired flag is set to 1, these methods will not skip expired keys, otherwise they follow the normal logic execution.

CLAassistant · 2025-08-08T09:34:50Z

All committers have signed the CLA.

snyk-io · 2025-08-08T09:34:55Z

🎉 Snyk checks have passed. No issues have been found so far.

✅ security/snyk check is complete. No issues have been found. (View Details)

✅ license/snyk check is complete. No issues have been found. (View Details)

kaplanben · 2025-08-08T09:46:30Z

Checkmarx One – Scan Summary & Details – 4622f44d-d4c1-4c64-a4e1-395029a501bf

New Issues (5)

Checkmarx found the following issues in this Pull Request

Issue	Source File / Package	Checkmarx Insight
Buffer_Improper_Index_Access	/src/dict.c: 1412	details The array index entries at /src/dict.c in line 1412 is used to reference an index of a cell of the array entries at /src/dict.c in line 1412 in a... `ID: 4FV2BYbldlpSrbF4lW7Dv2o9Dx4%3D` Attack Vector
Divide_By_Zero	/src/object.c: 1236	details The application performs an illegal operation in objectComputeSize, in /src/object.c. In line 1236, the program attempts to divide by elecount, w... `ID: Blm6eEEj9vF5UfiAV2Vg9JeRDf4%3D` Attack Vector
Unpinned Actions Full Length Commit SHA	/daily.yml: 973	details Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA help... `ID: K90d22j%2B5y9BqWICFgc5IB5GJmI%3D`
Unpinned Actions Full Length Commit SHA	/daily.yml: 1012	details Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA help... `ID: ngpCPc8xRqiGpjip1VdNRg0fOyg%3D`
Unpinned Actions Full Length Commit SHA	/daily.yml: 1138	details Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA help... `ID: g8ggXzSad9Bi6hVITKKmTVlxfL4%3D`

Fixed Issues (5)

Great job! The following issues were fixed in this Pull Request

Severity	Issue	Source File / Package
	~~Use_After_Free~~	/deps/lua/src/strbuf.c: 104
	~~Divide_By_Zero~~	/src/object.c: 1239
	~~Unpinned Actions Full Length Commit SHA~~	/daily.yml: 1073
	~~Unpinned Actions Full Length Commit SHA~~	/daily.yml: 917
	~~Unpinned Actions Full Length Commit SHA~~	/daily.yml: 956

guybe7 · 2025-08-13T09:08:22Z

src/db.c

@@ -1275,7 +1275,7 @@ void keysCommand(client *c) {
        sds key = kvobjGetKey(kv);

        if (allkeys || stringmatchlen(pattern,plen,key,sdslen(key),0)) {
-            if (!keyIsExpired(c->db, NULL, kv)) {
+            if (server.allow_access_expired || !keyIsExpired(c->db, NULL, kv)) {


why not add server.allow_access_expired to keyIsExpired?

That's a good idea, but since keyIsExpired is used in many places, I need to test whether this change will cause any bugs.

Jam804 · 2025-08-14T13:08:31Z

@sundb Would you mind taking a look at this? Thank you so much!

sundb

please also add a module test to cover it, thx.

src/db.c

sundb · 2025-08-15T04:03:07Z

@Jam804 why not fix another issue(hashTypeGetValue doesn't consider the flag, returning GETF_EXPIRED instead of GETF_OK ) mentioned in #14214?

Jam804 · 2025-08-15T04:06:42Z

@Jam804为什么不修复#14214hashTypeGetValue doesn't consider the flag, returning GETF_EXPIRED instead of GETF_OK 中提到的另一个问题（）？

I'm currently researching, and I plan to tackle them one by one. I'll complete the unit tests later.

Jam804 · 2025-08-16T04:13:11Z

@Jam804为什么不修复#14214hashTypeGetValue doesn't consider the flag, returning GETF_EXPIRED instead of GETF_OK 中提到的另一个问题（）？

I'm currently researching, and I plan to tackle them one by one. I'll complete the unit tests later.

done

src/debug.c

src/db.c

tests/unit/moduleapi/getkeys.tcl

Jam804 · 2025-08-18T16:21:12Z

@sundb Thank you very much for your corrections. This is my first time contributing code to Redis, please be lenient with me. I will continue to study Redis in depth and pay more attention in future PRs.

sundb · 2025-08-19T01:38:02Z

@Jam804 please don't use force push

src/debug.c

tests/unit/moduleapi/getkeys.tcl

sundb · 2025-08-19T01:46:07Z

I'm currently researching, and I plan to tackle them one by one. I'll complete the unit tests later.

done

but i don't see any change you made for this fix.

moticless · 2025-09-14T07:08:37Z

@Jam804, thanks for the updates 🙏. Please avoid git push --force on PRs that others need to follow. It rewrites history, forces reviewers to start over, and makes comments/diffs harder to track. A normal push works best, and cleanup can wait until just before merge. Thanks!

Jam804 · 2025-09-23T14:41:41Z

@moticless Could you please advise if any other modifications are needed in this Pull Request? I'll diligently work on resolving all identified issues.

guybe7 · 2025-09-23T23:11:51Z

src/t_hash.c

 /* Move to the next entry in the hash. Return C_OK when the next entry
 * could be found and C_ERR when the iterator reaches the end. */
 int hashTypeNext(hashTypeIterator *hi, int skipExpiredFields) {
+    skipExpiredFields &= !server.allow_access_expired;


i don't like this &=, it should be just =
(same above)

it's not logically wrong, just looks weird

src/t_hash.c

tests/unit/type/hash-field-expire.tcl

sundb · 2025-10-11T07:52:23Z

@Jam804 can you take a look at this failure, but I'm not sure it's related to this PR.

Jam804 · 2025-10-11T08:16:44Z

@Jam804 can you take a look at this failure, but I'm not sure it's related to this PR.

After reviewing both failing tests, I can confirm they are unrelated to this commit. These appear to be test environment instability issues.

sundb · 2025-10-11T08:22:56Z

@Jam804 please merge the latest unstable and let's try again. thx.

Jam804 · 2025-10-11T08:32:06Z

@Jam804 please merge the latest unstable and let's try again. thx.

done

sundb · 2025-10-12T02:26:10Z

@Jam804 please update the top comment, maybe list all the modifications that need to be noticed.

Jam804 · 2025-10-12T02:54:21Z

@Jam804请更新顶部评论，也许列出所有需要注意的修改。

done

sundb · 2025-10-12T03:02:10Z

@Jam804 merged, thx.

Jam804 · 2025-10-12T03:05:46Z

Thank you for your patient review and assistance.

Jam804 force-pushed the unstable branch 2 times, most recently from 8d57e8d to b94faef Compare August 13, 2025 07:48

guybe7 reviewed Aug 13, 2025

View reviewed changes

Jam804 force-pushed the unstable branch from b94faef to e92e8e4 Compare August 13, 2025 09:35

sundb reviewed Aug 15, 2025

View reviewed changes

src/db.c Outdated Show resolved Hide resolved

sundb reviewed Aug 15, 2025

View reviewed changes

src/db.c Outdated Show resolved Hide resolved

Jam804 force-pushed the unstable branch from e92e8e4 to 4ced453 Compare August 15, 2025 03:58

Jam804 force-pushed the unstable branch from 4ced453 to 1401d92 Compare August 16, 2025 04:08

sundb added this to Redis 8.2 Aug 18, 2025

github-project-automation bot moved this to Todo in Redis 8.2 Aug 18, 2025

sundb added the release-notes indication that this issue needs to be mentioned in the release notes label Aug 18, 2025

sundb reviewed Aug 18, 2025

View reviewed changes

src/debug.c Outdated Show resolved Hide resolved

src/debug.c Outdated Show resolved Hide resolved

src/db.c Show resolved Hide resolved

tests/unit/moduleapi/getkeys.tcl Outdated Show resolved Hide resolved

Jam804 force-pushed the unstable branch 2 times, most recently from 9c9e9d5 to 23ae347 Compare August 18, 2025 16:04

Jam804 force-pushed the unstable branch from 23ae347 to 97bf8f7 Compare August 18, 2025 16:29

sundb reviewed Aug 19, 2025

View reviewed changes

src/debug.c Outdated Show resolved Hide resolved

tests/unit/moduleapi/getkeys.tcl Outdated Show resolved Hide resolved

tests/unit/moduleapi/getkeys.tcl Outdated Show resolved Hide resolved

sundb requested a review from moticless August 19, 2025 07:16

Jam804 force-pushed the unstable branch from 565f1e1 to 3d07248 Compare August 19, 2025 07:24

Jam804 closed this Aug 19, 2025

Jam804 force-pushed the unstable branch from d9c9e60 to 3fa7a65 Compare August 19, 2025 07:26

Jam804 force-pushed the unstable branch from fa7d722 to 9b63e99 Compare September 13, 2025 04:16

Jam804 reopened this Sep 13, 2025

Jam804 force-pushed the unstable branch 2 times, most recently from 6ebc948 to af7c952 Compare September 13, 2025 14:16

Fix issues with server.allow_access_expired(redis#14214)

9a20d9f

Jam804 force-pushed the unstable branch from af7c952 to 9a20d9f Compare September 13, 2025 15:42

sundb moved this from Done to Awaits Merge in Redis 8.4 Sep 23, 2025

guybe7 reviewed Sep 23, 2025

View reviewed changes

Fix issues with server.allow_access_expired(redis#14214)

b266e4b

sundb approved these changes Oct 10, 2025

View reviewed changes

sundb reviewed Oct 11, 2025

View reviewed changes

src/t_hash.c Outdated Show resolved Hide resolved

src/t_hash.c Outdated Show resolved Hide resolved

sundb added 2 commits October 11, 2025 14:43

Update src/t_hash.c

c745b9c

Update src/t_hash.c

a90d131

sundb reviewed Oct 11, 2025

View reviewed changes

tests/unit/type/hash-field-expire.tcl Outdated Show resolved Hide resolved

Update tests/unit/type/hash-field-expire.tcl

9912158

Jam804 added 2 commits October 11, 2025 16:26

Fix issues with server.allow_access_expired(redis#14214)

8a0214f

Merge remote-tracking branch 'origin/unstable' into unstable

6db3ac5

sundb merged commit 083f38e into redis:unstable Oct 12, 2025
19 checks passed

github-project-automation bot moved this from Awaits Merge to Done in Redis 8.4 Oct 12, 2025

Conversation

Jam804 commented Aug 8, 2025 • edited by sundb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CLAassistant commented Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snyk-io bot commented Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎉 Snyk checks have passed. No issues have been found so far.

Uh oh!

kaplanben commented Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guybe7 Aug 13, 2025

Choose a reason for hiding this comment

Uh oh!

Jam804 Aug 13, 2025

Choose a reason for hiding this comment

Uh oh!

Jam804 commented Aug 14, 2025

Uh oh!

sundb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sundb commented Aug 15, 2025

Uh oh!

Jam804 commented Aug 15, 2025

Uh oh!

Jam804 commented Aug 16, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Jam804 commented Aug 18, 2025

Uh oh!

sundb commented Aug 19, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sundb commented Aug 19, 2025

Uh oh!

moticless commented Sep 14, 2025

Uh oh!

Jam804 commented Sep 23, 2025

Uh oh!

guybe7 Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

guybe7 Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

Jam804 Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sundb commented Oct 11, 2025

Uh oh!

Jam804 commented Oct 11, 2025

Uh oh!

sundb commented Oct 11, 2025

Uh oh!

Jam804 commented Oct 11, 2025

Uh oh!

sundb commented Oct 12, 2025

Uh oh!

Jam804 commented Oct 12, 2025

Uh oh!

Uh oh!

sundb commented Oct 12, 2025

Uh oh!

Jam804 commented Oct 12, 2025

Uh oh!

Reviewers

Jam804 commented Aug 8, 2025 •

edited by sundb

Loading

CLAassistant commented Aug 8, 2025 •

edited

Loading

snyk-io bot commented Aug 8, 2025 •

edited

Loading

kaplanben commented Aug 8, 2025 •

edited

Loading