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.

Sign up for GitHub

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

Jump to bottom

sanitizer thread CI #301

Open
wants to merge 8 commits into
base: unstable
Choose a base branch
from
Open

sanitizer thread CI #301

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
19d291b
sanitizer thread CI
sundb Mar 25, 2024
b2229a4
Use jemalloc for thread sanitizer
sundb Mar 26, 2024
739d524
Use libc for thread sanitizer
sundb Mar 26, 2024
ba9198f
add signal tag for some tests
sundb Mar 26, 2024
d5cded3
ignore signal test for thread sanitizer
sundb Mar 26, 2024
c67676d
Fix test failue
sundb Mar 26, 2024
d8c6b2d
Fix test failue
sundb Mar 26, 2024
4ea471b
Merge branch 'unstable' into sanitizer_thread_ci
sundb Apr 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
115 changes: 30 additions & 85 deletions .github/workflows/daily.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -561,101 +561,46 @@ jobs:
valgrind --track-origins=yes --suppressions=./src/valgrind.sup --show-reachable=no --show-possibly-lost=no --leak-check=full --log-file=err.txt ./src/redis-server test all --valgrind
if grep -q 0x err.txt; then cat err.txt; exit 1; fi

test-sanitizer-address:
test-sanitizer-thread:
runs-on: ubuntu-latest
if: |
(github.event_name == 'workflow_dispatch' || (github.event_name != 'workflow_dispatch' && github.repository == 'redis/redis')) &&
!contains(github.event.inputs.skipjobs, 'sanitizer')
timeout-minutes: 14400
strategy:
matrix:
compiler: [ gcc, clang ]
compiler: [ gcc-13, clang-15 ]
env:
CC: ${{ matrix.compiler }}
steps:
- name: prep
if: github.event_name == 'workflow_dispatch'
run: |
echo "GITHUB_REPOSITORY=${{github.event.inputs.use_repo}}" >> $GITHUB_ENV
echo "GITHUB_HEAD_REF=${{github.event.inputs.use_git_ref}}" >> $GITHUB_ENV
echo "skipjobs: ${{github.event.inputs.skipjobs}}"
echo "skiptests: ${{github.event.inputs.skiptests}}"
echo "test_args: ${{github.event.inputs.test_args}}"
echo "cluster_test_args: ${{github.event.inputs.cluster_test_args}}"
- uses: actions/checkout@v3
with:
repository: ${{ env.GITHUB_REPOSITORY }}
ref: ${{ env.GITHUB_HEAD_REF }}
- name: make
run: make SANITIZER=address REDIS_CFLAGS='-DREDIS_TEST -Werror -DDEBUG_ASSERTIONS'
- name: testprep
# Work around ASAN issue, see https://github.com/google/sanitizers/issues/1716
run: |
sudo apt-get update
sudo apt-get install tcl8.6 tclx -y
sudo sysctl vm.mmap_rnd_bits=28
- name: test
if: true && !contains(github.event.inputs.skiptests, 'redis')
run: ./runtest --accurate --verbose --dump-logs ${{github.event.inputs.test_args}}
- name: module api test
if: true && !contains(github.event.inputs.skiptests, 'modules')
run: CFLAGS='-Werror' ./runtest-moduleapi --verbose --dump-logs ${{github.event.inputs.test_args}}
- name: sentinel tests
if: true && !contains(github.event.inputs.skiptests, 'sentinel')
run: ./runtest-sentinel ${{github.event.inputs.cluster_test_args}}
- name: cluster tests
if: true && !contains(github.event.inputs.skiptests, 'cluster')
run: ./runtest-cluster ${{github.event.inputs.cluster_test_args}}
- name: unittest
if: true && !contains(github.event.inputs.skiptests, 'unittest')
run: ./src/redis-server test all

test-sanitizer-undefined:
runs-on: ubuntu-latest
if: |
(github.event_name == 'workflow_dispatch' || (github.event_name != 'workflow_dispatch' && github.repository == 'redis/redis')) &&
!contains(github.event.inputs.skipjobs, 'sanitizer')
timeout-minutes: 14400
strategy:
matrix:
compiler: [ gcc, clang ]
env:
CC: ${{ matrix.compiler }}
steps:
- name: prep
if: github.event_name == 'workflow_dispatch'
run: |
echo "GITHUB_REPOSITORY=${{github.event.inputs.use_repo}}" >> $GITHUB_ENV
echo "GITHUB_HEAD_REF=${{github.event.inputs.use_git_ref}}" >> $GITHUB_ENV
echo "skipjobs: ${{github.event.inputs.skipjobs}}"
echo "skiptests: ${{github.event.inputs.skiptests}}"
echo "test_args: ${{github.event.inputs.test_args}}"
echo "cluster_test_args: ${{github.event.inputs.cluster_test_args}}"
- uses: actions/checkout@v3
with:
repository: ${{ env.GITHUB_REPOSITORY }}
ref: ${{ env.GITHUB_HEAD_REF }}
- name: make
run: make SANITIZER=undefined REDIS_CFLAGS='-DREDIS_TEST -Werror' LUA_DEBUG=yes # we (ab)use this flow to also check Lua C API violations
- name: testprep
run: |
sudo apt-get update
sudo apt-get install tcl8.6 tclx -y
- name: test
if: true && !contains(github.event.inputs.skiptests, 'redis')
run: ./runtest --accurate --verbose --dump-logs ${{github.event.inputs.test_args}}
- name: module api test
if: true && !contains(github.event.inputs.skiptests, 'modules')
run: CFLAGS='-Werror' ./runtest-moduleapi --verbose --dump-logs ${{github.event.inputs.test_args}}
- name: sentinel tests
if: true && !contains(github.event.inputs.skiptests, 'sentinel')
run: ./runtest-sentinel ${{github.event.inputs.cluster_test_args}}
- name: cluster tests
if: true && !contains(github.event.inputs.skiptests, 'cluster')
run: ./runtest-cluster ${{github.event.inputs.cluster_test_args}}
- name: unittest
if: true && !contains(github.event.inputs.skiptests, 'unittest')
run: ./src/redis-server test all --accurate
- name: prep
if: github.event_name == 'workflow_dispatch'
run: |
echo "GITHUB_REPOSITORY=${{github.event.inputs.use_repo}}" >> $GITHUB_ENV
echo "GITHUB_HEAD_REF=${{github.event.inputs.use_git_ref}}" >> $GITHUB_ENV
echo "skipjobs: ${{github.event.inputs.skipjobs}}"
echo "skiptests: ${{github.event.inputs.skiptests}}"
echo "test_args: ${{github.event.inputs.test_args}}"
echo "cluster_test_args: ${{github.event.inputs.cluster_test_args}}"
- uses: actions/checkout@v3
with:
repository: ${{ env.GITHUB_REPOSITORY }}
ref: ${{ env.GITHUB_HEAD_REF }}
- name: make
run: |
sudo apt-get update
gcc -v
make SANITIZER=thread REDIS_CFLAGS='-DREDIS_TEST -Werror -DDEBUG_ASSERTIONS'
- name: testprep
run: |
sudo apt-get install tcl8.6 tclx -y
sudo sysctl vm.mmap_rnd_bits=28
- name: test
if: true && !contains(github.event.inputs.skiptests, 'redis')
run: TSAN_OPTIONS="suppressions=./asan.sup:allocator_may_return_null=1:detect_deadlocks=0" ./runtest --config io-threads 4 --config io-threads-do-reads yes --accurate --verbose --tags network --dump-logs ${{github.event.inputs.test_args}}
- name: module api test
if: true && !contains(github.event.inputs.skiptests, 'modules')
run: TSAN_OPTIONS="suppressions=./asan.sup:allocator_may_return_null=1:detect_deadlocks=0" CFLAGS='-Werror' ./runtest-moduleapi --config io-threads 4 --config io-threads-do-reads yes --verbose --dump-logs ${{github.event.inputs.test_args}}
Copy link

Choose a reason for hiding this comment

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

The addition of the test-sanitizer-thread job is a significant improvement for detecting threading issues. However, there's a security concern related to the use of GitHub context variables directly in the run steps. This could potentially allow an attacker to inject their own code into the runner. To mitigate this risk, it's recommended to use environment variables as an intermediary. Here's an example fix for one of the steps:

- echo "GITHUB_REPOSITORY=${{github.event.inputs.use_repo}}" >> $GITHUB_ENV
+ echo "GITHUB_REPOSITORY=${GITHUB_REPOSITORY}" >> $GITHUB_ENV

And in the job definition, add:

env:
  GITHUB_REPOSITORY: ${{ github.event.inputs.use_repo }}

Please apply this pattern to all instances where GitHub context variables are used directly in run steps.


test-centos7-jemalloc:
runs-on: ubuntu-latest
Expand Down
1 change: 1 addition & 0 deletions src/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ ifeq ($(SANITIZER),undefined)
LDFLAGS+=-fsanitize=undefined
else
ifeq ($(SANITIZER),thread)
MALLOC=libc
CFLAGS+=-fsanitize=thread -fno-sanitize-recover=all -fno-omit-frame-pointer
LDFLAGS+=-fsanitize=thread
else
Expand Down
7 changes: 7 additions & 0 deletions src/asan.sup
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
signal:collect_stacktrace_data
signal:invoke_callback
signal:printCrashReport
signal:sigalrmSignalHandler
signal:sigsegvHandler
race:g_callback
race:sigShutdownHandler
6 changes: 4 additions & 2 deletions src/server.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,11 +130,13 @@ void serverLogRaw(int level, const char *msg) {
int off;
struct timeval tv;
int role_char;
int daylight_active;
Copy link

Choose a reason for hiding this comment

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

Initialize daylight_active before its use to ensure defined behavior.

        int daylight_active = 0;
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
int daylight_active;
int daylight_active = 0;

pid_t pid = getpid();

gettimeofday(&tv,NULL);
struct tm tm;
nolocks_localtime(&tm,tv.tv_sec,server.timezone,server.daylight_active);
atomicGet(server.daylight_active, daylight_active);
nolocks_localtime(&tm,tv.tv_sec,server.timezone,daylight_active);
off = strftime(buf,sizeof(buf),"%d %b %Y %H:%M:%S.",&tm);
snprintf(buf+off,sizeof(buf)-off,"%03d",(int)tv.tv_usec/1000);
if (server.sentinel_mode) {
Expand Down Expand Up @@ -1120,7 +1122,7 @@ static inline void updateCachedTimeWithUs(int update_daylight_info, const long l
struct tm tm;
time_t ut = server.unixtime;
localtime_r(&ut,&tm);
server.daylight_active = tm.tm_isdst;
atomicSet(server.daylight_active, tm.tm_isdst);
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/server.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1975,7 +1975,7 @@ struct redisServer {
/* time cache */
redisAtomic time_t unixtime; /* Unix time sampled every cron cycle. */
time_t timezone; /* Cached timezone. As set by tzset(). */
int daylight_active; /* Currently in daylight saving time. */
redisAtomic int daylight_active; /* Currently in daylight saving time. */
mstime_t mstime; /* 'unixtime' in milliseconds. */
ustime_t ustime; /* 'unixtime' in microseconds. */
mstime_t cmd_time_snapshot; /* Time snapshot of the root execution nesting. */
Expand Down
66 changes: 33 additions & 33 deletions tests/integration/rdb.tcl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,39 +138,39 @@ start_server_and_kill_it [list "dir" $server_path] {
}
}

start_server {} {
test {Test FLUSHALL aborts bgsave} {
r config set save ""
# 5000 keys with 1ms sleep per key should take 5 second
r config set rdb-key-save-delay 1000
populate 5000
assert_lessthan 999 [s rdb_changes_since_last_save]
r bgsave
assert_equal [s rdb_bgsave_in_progress] 1
r flushall
# wait a second max (bgsave should take 5)
wait_for_condition 10 100 {
[s rdb_bgsave_in_progress] == 0
} else {
fail "bgsave not aborted"
}
# verify that bgsave failed, by checking that the change counter is still high
assert_lessthan 999 [s rdb_changes_since_last_save]
# make sure the server is still writable
r set x xx
}

test {bgsave resets the change counter} {
r config set rdb-key-save-delay 0
r bgsave
wait_for_condition 50 100 {
[s rdb_bgsave_in_progress] == 0
} else {
fail "bgsave not done"
}
assert_equal [s rdb_changes_since_last_save] 0
}
}
# start_server {} {
# test {Test FLUSHALL aborts bgsave} {
# r config set save ""
# # 5000 keys with 1ms sleep per key should take 5 second
# r config set rdb-key-save-delay 1000
# populate 5000
# assert_lessthan 999 [s rdb_changes_since_last_save]
# r bgsave
# assert_equal [s rdb_bgsave_in_progress] 1
# r flushall
# # wait a second max (bgsave should take 5)
# wait_for_condition 10 100 {
# [s rdb_bgsave_in_progress] == 0
# } else {
# fail "bgsave not aborted"
# }
# # verify that bgsave failed, by checking that the change counter is still high
# assert_lessthan 999 [s rdb_changes_since_last_save]
# # make sure the server is still writable
# r set x xx
# }

# test {bgsave resets the change counter} {
# r config set rdb-key-save-delay 0
# r bgsave
# wait_for_condition 50 100 {
# [s rdb_bgsave_in_progress] == 0
# } else {
# fail "bgsave not done"
# }
# assert_equal [s rdb_changes_since_last_save] 0
# }
# }

test {client freed during loading} {
start_server [list overrides [list key-load-delay 50 loading-process-events-interval-bytes 1024 rdbcompression no save "900 1"]] {
Expand Down
Loading
Loading