Skip to content

Conversation

@moticless
Copy link
Collaborator

@moticless moticless commented Oct 8, 2025

This PR is based on:
valkey-io/valkey#2347

This was introduced in #13512

The server crashes with a null pointer dereference when lookupKey() is called from handleClientsBlockedOnKey(). The crash occurs because server.executing_client is NULL, but the code attempts to access server.executing_client->cmd->proc without checking.

Crash scenario:
Client 1 enables CLIENT NO-TOUCH
Client 2 blocks on BRPOP mylist 0
Client 1 executes RPUSH mylist elem
When unblocking Client 2, lookupKey() dereferences NULL server.executing_client → crash

Solution
Added proper null checks before dereferencing server.executing_client:
Check if LOOKUP_NOTOUCH flag is already set before attempting to modify it
Verify both server.current_client and server.executing_client are not NULL before accessing their members
Maintain the TOUCH command exception for scripts

Testing
Added regression test in tests/unit/type/list.tcl that reproduces and verifies the fix for this crash scenario.

This fix is based on valkey-io/valkey#2347

Co-authored-by: Uri Yagelnik uriy@amazon.com
Co-authored-by: Ran Shidlansik ranshid@amazon.com

@jit-ci
Copy link

jit-ci bot commented Oct 8, 2025

Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset.

In case there are security findings, they will be communicated to you as a comment inside the PR.

Hope you’ll enjoy using Jit.

Questions? Comments? Want to learn more? Get in touch with us.

@sundb sundb added the release-notes indication that this issue needs to be mentioned in the release notes label Oct 10, 2025
@sundb
Copy link
Collaborator

sundb commented Oct 11, 2025

Why can't I see the test crashes when i reverted the code in db.c?

@moticless
Copy link
Collaborator Author

@sundb , it is reproduced easily on my machine. just by running the test with previous code:

./runtest --single unit/type/list --only "CLIENT NO-TOUCH with BRPOP and RPUSH regression test"

Copy link
Collaborator

@sundb sundb left a comment

Choose a reason for hiding this comment

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

@moticless I updated the top comment. Please confirm whether the version of backport is correct.

@moticless
Copy link
Collaborator Author

@sundb ,
i prompted my "assistant" to provide a dockerfile for reproduction:

# Dockerfile
# Verifies PR #14415
# 1) Test should PASS on the PR branch
# 2) Revert only src/db.c to origin/unstable; test should then FAIL
#
# Usage:
#   docker build -t redis-pr-14415-check .
#   docker run --rm -it redis-pr-14415-check
#
# Notes:
# - This container prints a step-by-step summary and exits 0 only if outcomes match expectations.
# - To explore manually, override the CMD with /bin/bash.

FROM ubuntu:focal

ENV DEBIAN_FRONTEND=noninteractive
SHELL ["/bin/bash", "-o", "pipefail", "-lc"]

# Core deps
RUN apt-get update && apt-get install -y --no-install-recommends \
    git build-essential pkg-config tcl procps ca-certificates \
 && rm -rf /var/lib/apt/lists/*

# Where we'll place sources
WORKDIR /opt

# Small helper script that performs the entire verification at container runtime
RUN cat > /usr/local/bin/verify.sh <<'EOF'
#!/usr/bin/env bash
set -Eeuo pipefail

REPO_URL="https://github.com/redis/redis.git"
WORKDIR="/opt/redis"
PR_NUM="14415"
PR_LOCAL_BRANCH="pr-${PR_NUM}"
BASE_BRANCH="origin/unstable"
EXACT_TEST_NAME="CLIENT NO-TOUCH with BRPOP and RPUSH regression test"
TEST_FILE="tests/unit/type/list.tcl"
SINGLE_SUITE="unit/type/list"

log() { printf "\n\033[1;34m[INFO]\033[0m %s\n" "$*"; }
ok()  { printf "\033[1;32m[ OK ]\033[0m %s\n" "$*"; }
err() { printf "\033[1;31m[ERR]\033[0m %s\n" "$*" >&2; }

# Fresh clone
log "Cloning Redis into ${WORKDIR}…"
rm -rf "${WORKDIR}"
git clone --depth=1 "${REPO_URL}" "${WORKDIR}"
cd "${WORKDIR}"

# Fetch PR and check it out locally
log "Fetching PR #${PR_NUM} into local branch ${PR_LOCAL_BRANCH}…"
git fetch origin "pull/${PR_NUM}/head:${PR_LOCAL_BRANCH}"
git checkout "${PR_LOCAL_BRANCH}"
ok "Now on commit: $(git rev-parse --short HEAD)"

# Build on PR branch
log "Building Redis (PR branch)…"
make -j"$(nproc)" > /tmp/build_pr.log 2>&1 || { err "Build failed on PR branch. See /tmp/build_pr.log"; exit 11; }
ok "Build completed on PR branch."

# Verify test file and exact test name exist
log "Verifying ${TEST_FILE} exists and contains the exact test name…"
if [[ -f "${TEST_FILE}" ]]; then
  ok "Found ${TEST_FILE}"
else
  err "Missing ${TEST_FILE}"; exit 21
fi

if grep -Fq "${EXACT_TEST_NAME}" "${TEST_FILE}"; then
  ok "Found test name: ${EXACT_TEST_NAME}"
else
  err "Exact test name not found in ${TEST_FILE}"; exit 22
fi

# Run the test: expect PASS
log "Running test (expecting PASS on PR branch)…"
set +e
./runtest --single "${SINGLE_SUITE}" --only "${EXACT_TEST_NAME}"
rc_pass=$?
set -e

if [[ ${rc_pass} -eq 0 ]]; then
  ok "PASS as expected on PR branch."
else
  err "Test failed on PR branch but expected PASS (rc=${rc_pass})."; exit 31
fi

# Revert only src/db.c to the base branch, keep the test changes
log "Reverting ONLY src/db.c to ${BASE_BRANCH} while staying on PR branch…"
git fetch origin
git checkout "${PR_LOCAL_BRANCH}"
git checkout "${BASE_BRANCH}" -- src/db.c
ok "Reverted src/db.c from ${BASE_BRANCH}. (Tests and other files remain from PR.)"

# Rebuild after selective revert
log "Rebuilding after reverting src/db.c…"
make -j"$(nproc)" clean > /dev/null 2>&1 || true
make -j"$(nproc)" > /tmp/build_revert.log 2>&1 || { err "Rebuild failed after revert. See /tmp/build_revert.log"; exit 41; }
ok "Rebuild completed."

# Run the test again: expect FAIL now
log "Running test again (expecting FAIL after reverting src/db.c)…"
set +e
./runtest --single "${SINGLE_SUITE}" --only "${EXACT_TEST_NAME}"
rc_fail=$?
set -e

if [[ ${rc_fail} -ne 0 ]]; then
  ok "FAIL as expected after reverting src/db.c."
else
  err "Test PASSED but was expected to FAIL after revert."; exit 51
fi

log "All checks behaved as expected 🎉"
exit 0
EOF
RUN chmod +x /usr/local/bin/verify.sh

# Default behavior: run verification and print results
CMD ["/usr/local/bin/verify.sh"]

@sundb
Copy link
Collaborator

sundb commented Oct 13, 2025

@moticless I can reproduce it now, but I need to use tricks to avoid the compiler optimizing this code or use make noopt. Anyway, I think this fix is reasonable.

@moticless
Copy link
Collaborator Author

Verified it is reproduced at 8.0 and 8.2. Thanks.

@moticless moticless merged commit 5b49119 into redis:unstable Oct 13, 2025
19 checks passed
@github-project-automation github-project-automation bot moved this from Todo to Done in Redis 8.4 Oct 13, 2025
@moticless moticless deleted the fix-notouch-cond branch October 13, 2025 09:12
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

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants