Add checks for module initialisation. #336

iddm · 2023-05-22T16:28:17Z

This should resolve the debug assertion and allow only safe code to continue.

https://app.circleci.com/pipelines/github/RedisJSON/RedisJSON/6447/workflows/34fa491b-ece8-4dd5-a5ad-3d5c6df9e024/jobs/25181/artifacts

==25533==ERROR: AddressSanitizer: unknown-crash on address 0x0000800f7000 at pc 0x561d15182f77 bp 0x7ffeccf9cd10 sp 0x7ffeccf9c4e0
READ of size 1048576 at 0x0000800f7000 thread T0
    #0 0x561d15182f76 in __asan_memcpy (/usr/local/bin/redis-server-asan-7.0+0x2cff76) (BuildId: ffa80d1529a582d69067a6c3e5462cd60ab42fe2)
    #1 0x561d15369087 in memtest_preserving_test /tmp/redis.j6zm0e9t/redis/src/memtest.c:317:9
    #2 0x561d1531d4c9 in memtest_test_linux_anonymous_maps /tmp/redis.j6zm0e9t/redis/src/debug.c:1896:19
    #3 0x561d1531af24 in doFastMemoryTest /tmp/redis.j6zm0e9t/redis/src/debug.c:1937:13
    #4 0x561d1531af24 in printCrashReport /tmp/redis.j6zm0e9t/redis/src/debug.c:2080:5
    #5 0x561d1531e8e3 in sigsegvHandler /tmp/redis.j6zm0e9t/redis/src/debug.c:2055:5
    #6 0x7ff34038b41f  (/lib/x86_64-linux-gnu/libpthread.so.0+0x1441f) (BuildId: 7b4536f41cdaa5888408e82d0836e33dcf436466)
    #7 0x7ff34018f00a in raise (/lib/x86_64-linux-gnu/libc.so.6+0x4300a) (BuildId: 1878e6b475720c7c51969e69ab2d276fae6d1dee)
    #8 0x7ff34016e858 in abort (/lib/x86_64-linux-gnu/libc.so.6+0x22858) (BuildId: 1878e6b475720c7c51969e69ab2d276fae6d1dee)
    #9 0x7ff33c434d5e in std::sys::unix::abort_internal::h64bfc8ada37afee6 /root/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys/unix/mod.rs:359:14
    #10 0x7ff33c5535f3 in std::panicking::rust_panic_with_hook::h6fadecca9aaff204 /root/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:710:9
    #11 0x7ff33c552686 in std::panicking::begin_panic_handler::_$u7b$$u7b$closure$u7d$$u7d$::h198454f31fbc2d5c /root/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:580:13
    #12 0x7ff33c47a835 in std::sys_common::backtrace::__rust_end_short_backtrace::he13cfb2667f04a44 /root/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys_common/backtrace.rs:151:18
    #13 0x7ff33c55140e in rust_begin_unwind /root/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:578:5
    #14 0x7ff33c8834fa in core::panicking::panic_nounwind_fmt::h415ef816351f00b7 /root/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/panicking.rs:96:14
    #15 0x7ff33c8838af in core::panicking::panic_nounwind::h28910cef51d4125b /root/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/panicking.rs:126:5
    #16 0x7ff33b827ed1 in core::slice::raw::from_raw_parts::runtime::h0076baf8f5eab5a8 /root/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/intrinsics.rs:2520:21
    #17 0x7ff33b827ed1 in core::slice::raw::from_raw_parts::hf93a4401b1aee34e /root/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/slice/raw.rs:95:9
    #18 0x7ff33b7f9e06 in redis_module::redismodule::decode_args::h26526219f562d7cf /root/.cargo/registry/src/index.crates.io-6f17d22bba15001f/redis-module-2.0.3/src/redismodule.rs:100:14
    #19 0x7ff33b28c843 in RedisModule_OnLoad /root/.cargo/registry/src/index.crates.io-6f17d22bba15001f/redis-module-2.0.3/src/macros.rs:215:24
    #20 0x561d153e684c in moduleLoad /tmp/redis.j6zm0e9t/redis/src/module.c:11288:9
    #21 0x561d153e5fde in moduleLoadFromQueue /tmp/redis.j6zm0e9t/redis/src/module.c:11096:13
    #22 0x561d151f4884 in main /tmp/redis.j6zm0e9t/redis/src/server.c:7123:9
    #23 0x7ff340170082 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x24082) (BuildId: 1878e6b475720c7c51969e69ab2d276fae6d1dee)
    #24 0x561d150ff0ad in _start (/usr/local/bin/redis-server-asan-7.0+0x24c0ad) (BuildId: ffa80d1529a582d69067a6c3e5462cd60ab42fe2)

Address 0x0000800f7000 is located in the low shadow area.
SUMMARY: AddressSanitizer: unknown-crash (/usr/local/bin/redis-server-asan-7.0+0x2cff76) (BuildId: ffa80d1529a582d69067a6c3e5462cd60ab42fe2) in __asan_memcpy
==25533==ABORTING

iddm · 2023-05-22T16:37:05Z

src/macros.rs

+            if ctx.is_null() {
+                return raw::Status::Err as _;
+            }
+
+            if argv.is_null() && argc != 0 {
+                return raw::Status::Err as _;
+            }
+
+            if !argv.is_null() && unsafe { (*argv).is_null() } {
+                return raw::Status::Err as _;
+            }
+
+            if !argv.is_null() && unsafe { !(*argv).is_null() } && (argc.is_negative() || argc == 0) {
+                return raw::Status::Err as _;
+            }
+


This can be collapsed into

if ctx.is_null() || (argv.is_null() && argc != 0) || (!argv.is_null() && unsafe { (*argv).is_null() }) || (!argv.is_null() && unsafe { !(*argv).is_null() } && (argc.is_negative() || argc == 0)) { return raw::Status::Err as _; }

But doesn't look good.

MeirShpilraien · 2023-05-22T17:00:44Z

src/macros.rs

@@ -190,6 +190,22 @@ macro_rules! redis_module {
            use $crate::configuration::get_bool_default_config_value;
            use $crate::configuration::get_enum_default_config_value;

+            if ctx.is_null() {
+                return raw::Status::Err as _;


Maybe we should log why we did not start

Log with what? You mean just eprintln!?

We can log even with NULL context. If we can not log its better to crash with a huge panic here. If this will happened and exit silently we will spend hours trying to guess why the module did not start.

Sure, let's log something then.

Logging now.

MeirShpilraien · 2023-05-22T17:02:36Z

src/macros.rs

+                return raw::Status::Err as _;
+            }
+
+            if !argv.is_null() && unsafe { (*argv).is_null() } {


You assume here that none NULL argv means that argc > 0. I am not sure this is promise.

Yes, I am also not sure.

I am also not sure if the negative values of argc are ever allowed.

No, we should not get a negative value.

Checking for it now too.

MeirShpilraien

I think we should also check for pointer alignment, at least for argv.

MeirShpilraien · 2023-05-22T17:04:55Z

This should fix the ASAN issues and possible issues at runtime.

I disagree it will fix the issue, it might give us a better clue where to search for the issue. The real issue is that we got a NULL or unaligned pointer from Redis. I also suspect it might be related to the sanitiser itself.

iddm · 2023-05-22T17:05:48Z

This should fix the ASAN issues and possible issues at runtime.

I disagree it will fix the issue, it might give us a better clue where to search for the issue. The real issue is that we got a NULL or unaligned pointer from Redis. I also suspect it might be related to the sanitiser itself.

Indeed, it won't ever fix any issues, it will solve the debug assertion and will only allow the correct code though.

This should resolve the debug assertion and allow only safe code to continue.

iddm self-assigned this May 22, 2023

iddm changed the title ~~Add checks for arguments parsing.~~ Add checks for module initialisation. May 22, 2023

iddm force-pushed the fix-asan-issue branch 3 times, most recently from b076313 to 9b8d3dc Compare May 22, 2023 16:35

iddm commented May 22, 2023

View reviewed changes

iddm requested a review from MeirShpilraien May 22, 2023 16:37

iddm force-pushed the fix-asan-issue branch from 9b8d3dc to a069a75 Compare May 22, 2023 16:54

MeirShpilraien reviewed May 22, 2023

View reviewed changes

iddm force-pushed the fix-asan-issue branch from a069a75 to 500703a Compare May 22, 2023 17:07

Add checks for the module initialisation.

2d96b60

This should resolve the debug assertion and allow only safe code to continue.

iddm force-pushed the fix-asan-issue branch from 500703a to 2d96b60 Compare May 23, 2023 05:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add checks for module initialisation. #336

Add checks for module initialisation. #336

iddm commented May 22, 2023 •

edited

Loading

iddm May 22, 2023

MeirShpilraien May 22, 2023

iddm May 22, 2023

MeirShpilraien May 22, 2023

iddm May 22, 2023

iddm May 23, 2023

MeirShpilraien May 22, 2023

iddm May 22, 2023

iddm May 22, 2023

MeirShpilraien May 22, 2023

iddm May 23, 2023

MeirShpilraien left a comment

MeirShpilraien commented May 22, 2023

iddm commented May 22, 2023

Add checks for module initialisation. #336

Are you sure you want to change the base?

Add checks for module initialisation. #336

Conversation

iddm commented May 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MeirShpilraien left a comment

Choose a reason for hiding this comment

MeirShpilraien commented May 22, 2023

iddm commented May 22, 2023

iddm commented May 22, 2023 •

edited

Loading