Skip to content

httpfs: fix null pointer dereference in AWSEnvironmentCredentialsProvider #9953

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
Merged
merged 1 commit into from
Dec 12, 2023
Merged

httpfs: fix null pointer dereference in AWSEnvironmentCredentialsProvider #9953

merged 1 commit into from
Dec 12, 2023

Conversation

mlafeldt
Copy link
Member

@mlafeldt mlafeldt commented Dec 11, 2023 •
edited
Loading

The unit tests of a project of mine that embeds duckdb-rs kept segfaulting randomly:

❯ cargo test
    Finished test [unoptimized + debuginfo] target(s) in 0.07s
     Running unittests main.rs (target/debug/deps/amend-98556005b6a72ddb)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running unittests lib.rs (target/debug/deps/amend_core-12cf0645e9308902)

running 6 tests
test serde_raw::tests::get_field ... ok
error: test failed, to rerun pass `-p amend_core --lib`

Caused by:
  process didn't exit successfully: `xxx/target/debug/deps/amend_core-12cf0645e9308902` (signal: 11, 
)

Using lldb, I found out that the problem is caused by SetExtensionOptionValue in s3fs.cpp (this line).

Here's a backtrace:

* thread #6, name = 'duckdb::tests::test_stream_items', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
    frame #0: 0x0000000190787e64 libsystem_platform.dylib`_platform_strlen + 4
libsystem_platform.dylib`_platform_strlen:
->  0x190787e64 <+4>:  ldr    q0, [x1]
    0x190787e68 <+8>:  adr    x3, #-200                 ; ___lldb_unnamed_symbol282
    0x190787e6c <+12>: ldr    q2, [x3], #0x10
    0x190787e70 <+16>: and    x2, x0, #0xf
(lldb) bt 10
* thread #6, name = 'duckdb::tests::test_stream_items', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x0000000190787e64 libsystem_platform.dylib`_platform_strlen + 4
    frame #1: 0x0000000100241c48 amend_core-12cf0645e9308902`std::__1::__constexpr_strlen[abi:v160006](__str=0x0000000000000000) at cstring:114:10
    frame #2: 0x00000001002414cc amend_core-12cf0645e9308902`std::__1::char_traits<char>::length(__s=0x0000000000000000) at char_traits.h:220:12
    frame #3: 0x00000001002579c4 amend_core-12cf0645e9308902`std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>::basic_string[abi:v160006]<std::nullptr_t>(this="", __s=0x0000000000000000) at string:882:17
    frame #4: 0x00000001002578ac amend_core-12cf0645e9308902`std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>::basic_string[abi:v160006]<std::nullptr_t>(this="", __s=0x0000000000000000) at string:880:58
    frame #5: 0x000000010104004c amend_core-12cf0645e9308902`duckdb::AWSEnvironmentCredentialsProvider::SetExtensionOptionValue(this=0x0000600000008b30, key="s3_region", env_var_name="AWS_DEFAULT_REGION") at s3fs.cpp:148:32
    frame #6: 0x00000001010401bc amend_core-12cf0645e9308902`duckdb::AWSEnvironmentCredentialsProvider::SetAll(this=0x0000600000008b30) at s3fs.cpp:157:8
    frame #7: 0x000000010142cd6c amend_core-12cf0645e9308902`duckdb::LoadInternal(instance=0x00000001059041d8) at httpfs_extension.cpp:55:12
    frame #8: 0x000000010142c33c amend_core-12cf0645e9308902`duckdb::HttpfsExtension::Load(this=0x0000000170833870, db=0x000060000000c030) at httpfs_extension.cpp:59:2
    frame #9: 0x000000010165bb24 amend_core-12cf0645e9308902`void duckdb::DuckDB::LoadExtension<duckdb::HttpfsExtension>(this=0x000060000000c030) at database.hpp:95:13

Turns out that using static char has some weird side effect leading to random crashes. Changing the declaration from static char to char ensures that the variable is reinitialized on each function call, which fixes the issue.

(TBH, I'm no expert in C++, and I'd love to learn more about the root cause here!)

@Mytherin
Copy link
Collaborator

Thanks for the PR! Indeed a static variable does not make much sense here. static variables are shared across all invocations of the function (even across threads) which can cause problems/race conditions when the function is used from multiple threads.

@Mytherin Mytherin merged commit 04570cb into duckdb:main Dec 12, 2023
@mlafeldt mlafeldt deleted the fix-httpfs-null-ptr branch December 12, 2023 09:02
krlmlr added a commit to duckdb/duckdb-r that referenced this pull request Dec 14, 2023
@krlmlr
chore: Update vendored sources to duckdb/duckdb@9f5e075
bd2c8fb 
Merge pull request duckdb/duckdb#9985 from hawkfish/summarize-overflow
Merge pull request duckdb/duckdb#9972 from yiyuanliu/lyy/fix-9949
Merge pull request duckdb/duckdb#9962 from Tishj/statement_error_expected_non_optional
Merge pull request duckdb/duckdb#9961 from Tishj/python_supply_config_to_connect
Merge pull request duckdb/duckdb#9947 from taniabogatsch/list-intersect
Merge pull request duckdb/duckdb#9568 from Jens-H/append-BigDecimal
Merge pull request duckdb/duckdb#9959 from Tishj/python_udf_arrow_side_effects
Merge pull request duckdb/duckdb#9855 from StarveZhou/issue_9795_arg
Merge pull request duckdb/duckdb#9944 from Tishj/pyproject_toml
Merge pull request duckdb/duckdb#9946 from lnkuiper/issue9718
Merge pull request duckdb/duckdb#9953 from mlafeldt/fix-httpfs-null-ptr
Merge pull request duckdb/duckdb#9897 from Tmonster/left_semi_anti_feature_rebased
Merge pull request duckdb/duckdb#9936 from hawkfish/empty-frames
Merge pull request duckdb/duckdb#9715 from Tishj/dependency_set
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mlafeldt @Mytherin