Skip to content

Commit 22a41e6

Browse files
COM8COM8
authored
Fixed curlholder Double Free (#1289)
* Fixed double free inside curl holder * Fixed sanitizer activation * Added curl holder double free tests
1 parent 52e20d4 commit 22a41e6

File tree

5 files changed

+92
-43
lines changed

5 files changed

+92
-43
lines changed

‎cmake/sanitizer.cmake‎

Lines changed: 36 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1,67 +1,69 @@
1-
include(CheckCXXSourceRuns)
1+
include(CheckCXXSourceCompiles)
22

3-
set(ALL_SAN_FLAGS "")
3+
set(ALL_ACTIVE_SAN_FLAGS "")
44

55
# No sanitizers when cross compiling to prevent stuff like this: https://github.com/whoshuu/cpr/issues/582
66
if(NOT CMAKE_CROSSCOMPILING)
7-
# Thread sanitizer
8-
if(CPR_DEBUG_SANITIZER_FLAG_THREAD)
9-
set(THREAD_SAN_FLAGS "-fsanitize=thread")
7+
function(cpr_check_sanitizer_compile flags result_var)
108
set(PREV_FLAG ${CMAKE_REQUIRED_FLAGS})
11-
set(CMAKE_REQUIRED_FLAGS "${THREAD_SAN_FLAGS}")
12-
check_cxx_source_runs("int main() { return 0; }" THREAD_SANITIZER_AVAILABLE_AND_ENABLED)
9+
set(CMAKE_REQUIRED_FLAGS "${flags}")
10+
check_cxx_source_compiles("int main() { return 0; }" ${result_var})
1311
set(CMAKE_REQUIRED_FLAGS ${PREV_FLAG})
14-
# Do not add the ThreadSanitizer for builds with all sanitizers enabled because it is incompatible with other sanitizers.
12+
endfunction()
13+
14+
# Thread sanitizer
15+
set(THREAD_SAN_FLAGS "-fsanitize=thread")
16+
if(CPR_DEBUG_SANITIZER_FLAG_THREAD)
17+
cpr_check_sanitizer_compile("${THREAD_SAN_FLAGS}" THREAD_SANITIZER_AVAILABLE_AND_ENABLED)
18+
if(NOT THREAD_SANITIZER_AVAILABLE_AND_ENABLED)
19+
message(FATAL_ERROR "ThreadSanitizer requested but the test program failed to compile with ${THREAD_SAN_FLAGS}.")
20+
endif()
1521
endif()
1622

1723
# Address sanitizer
24+
set(ADDR_SAN_FLAGS "-fsanitize=address")
1825
if(CPR_DEBUG_SANITIZER_FLAG_ADDR)
19-
set(ADDR_SAN_FLAGS "-fsanitize=address")
20-
set(PREV_FLAG ${CMAKE_REQUIRED_FLAGS})
21-
set(CMAKE_REQUIRED_FLAGS "${ADDR_SAN_FLAGS}")
22-
check_cxx_source_runs("int main() { return 0; }" ADDRESS_SANITIZER_AVAILABLE_AND_ENABLED)
23-
set(CMAKE_REQUIRED_FLAGS ${PREV_FLAG})
24-
if(ADDRESS_SANITIZER_AVAILABLE_AND_ENABLED)
25-
set(ALL_SAN_FLAGS "${ALL_SAN_FLAGS} ${ADDR_SAN_FLAGS}")
26+
cpr_check_sanitizer_compile("${ADDR_SAN_FLAGS}" ADDRESS_SANITIZER_AVAILABLE_AND_ENABLED)
27+
if(NOT ADDRESS_SANITIZER_AVAILABLE_AND_ENABLED)
28+
message(FATAL_ERROR "AddressSanitizer requested but the test program failed to compile with ${ADDR_SAN_FLAGS}.")
2629
endif()
2730
endif()
2831

2932
# Leak sanitizer
33+
set(LEAK_SAN_FLAGS "-fsanitize=leak")
3034
if(CPR_DEBUG_SANITIZER_FLAG_LEAK)
31-
set(LEAK_SAN_FLAGS "-fsanitize=leak")
32-
set(PREV_FLAG ${CMAKE_REQUIRED_FLAGS})
33-
set(CMAKE_REQUIRED_FLAGS "${LEAK_SAN_FLAGS}")
34-
check_cxx_source_runs("int main() { return 0; }" LEAK_SANITIZER_AVAILABLE_AND_ENABLED)
35-
set(CMAKE_REQUIRED_FLAGS ${PREV_FLAG})
36-
if(LEAK_SANITIZER_AVAILABLE_AND_ENABLED)
37-
set(ALL_SAN_FLAGS "${ALL_SAN_FLAGS} ${LEAK_SAN_FLAGS}")
35+
cpr_check_sanitizer_compile("${LEAK_SAN_FLAGS}" LEAK_SANITIZER_AVAILABLE_AND_ENABLED)
36+
if(NOT LEAK_SANITIZER_AVAILABLE_AND_ENABLED)
37+
message(FATAL_ERROR "LeakSanitizer requested but the test program failed to compile with ${LEAK_SAN_FLAGS}.")
3838
endif()
3939
endif()
4040

4141
# Undefined behavior sanitizer
42+
set(UDEF_SAN_FLAGS "-fsanitize=undefined")
4243
if(CPR_DEBUG_SANITIZER_FLAG_UB)
43-
set(UDEF_SAN_FLAGS "-fsanitize=undefined")
44-
set(PREV_FLAG ${CMAKE_REQUIRED_FLAGS})
45-
set(CMAKE_REQUIRED_FLAGS "${UDEF_SAN_FLAGS}")
46-
check_cxx_source_runs("int main() { return 0; }" UNDEFINED_BEHAVIOR_SANITIZER_AVAILABLE_AND_ENABLED)
47-
set(CMAKE_REQUIRED_FLAGS ${PREV_FLAG})
48-
if(UNDEFINED_BEHAVIOR_SANITIZER_AVAILABLE_AND_ENABLED)
49-
set(ALL_SAN_FLAGS "${ALL_SAN_FLAGS} ${UDEF_SAN_FLAGS}")
44+
cpr_check_sanitizer_compile("${UDEF_SAN_FLAGS}" UNDEFINED_BEHAVIOR_SANITIZER_AVAILABLE_AND_ENABLED)
45+
if(NOT UNDEFINED_BEHAVIOR_SANITIZER_AVAILABLE_AND_ENABLED)
46+
message(FATAL_ERROR "UndefinedBehaviorSanitizer requested but the test program failed to compile with ${UDEF_SAN_FLAGS}.")
5047
endif()
5148
endif()
5249

5350
# All sanitizer (without thread sanitizer)
54-
if(CPR_DEBUG_SANITIZER_FLAG_ALL AND NOT ALL_SAN_FLAGS STREQUAL "")
55-
set(PREV_FLAG ${CMAKE_REQUIRED_FLAGS})
56-
set(CMAKE_REQUIRED_FLAGS "${ALL_SAN_FLAGS}")
57-
check_cxx_source_runs("int main() { return 0; }" ALL_SANITIZERS_AVAILABLE_AND_ENABLED)
58-
set(CMAKE_REQUIRED_FLAGS ${PREV_FLAG})
51+
if(CPR_DEBUG_SANITIZER_FLAG_ALL)
52+
cpr_check_sanitizer_compile("${ADDR_SAN_FLAGS} ${UDEF_SAN_FLAGS} ${LEAK_SAN_FLAGS}" ALL_SANITIZERS_AVAILABLE_AND_ENABLED)
53+
if(NOT ALL_SANITIZERS_AVAILABLE_AND_ENABLED)
54+
message(FATAL_ERROR "All sanitizers requested but the test program failed to compile with ${ADDR_SAN_FLAGS} ${UDEF_SAN_FLAGS} ${LEAK_SAN_FLAGS}.")
55+
endif()
56+
set(ALL_ACTIVE_SAN_FLAGS "${ADDR_SAN_FLAGS} ${UDEF_SAN_FLAGS} ${LEAK_SAN_FLAGS}")
5957
endif()
6058

6159
if(THREAD_SANITIZER_AVAILABLE_AND_ENABLED)
6260
set(CMAKE_C_FLAGS_DEBUG "${CMAKE_C_FLAGS_DEBUG} ${THREAD_SAN_FLAGS}" CACHE INTERNAL "Flags used by the C compiler during thread sanitizer builds." FORCE)
6361
set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} ${THREAD_SAN_FLAGS}" CACHE INTERNAL "Flags used by the C++ compiler during thread sanitizer builds." FORCE)
6462
set(CMAKE_SHARED_LINKER_FLAGS_DEBUG "${CMAKE_SHARED_LINKER_FLAGS_DEBUG}" CACHE INTERNAL "Flags used for the linker during thread sanitizer builds" FORCE)
63+
elseif(ALL_SANITIZERS_AVAILABLE_AND_ENABLED)
64+
set(CMAKE_C_FLAGS_DEBUG "${CMAKE_C_FLAGS_DEBUG} ${ALL_ACTIVE_SAN_FLAGS} -fno-omit-frame-pointer -fno-optimize-sibling-calls" CACHE INTERNAL "Flags used by the C compiler during most possible sanitizer builds." FORCE)
65+
set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} ${ALL_ACTIVE_SAN_FLAGS} -fno-omit-frame-pointer -fno-optimize-sibling-calls" CACHE INTERNAL "Flags used by the C++ compiler during most possible sanitizer builds." FORCE)
66+
set(CMAKE_SHARED_LINKER_FLAGS_DEBUG "${CMAKE_SHARED_LINKER_FLAGS_DEBUG}" CACHE INTERNAL "Flags used for the linker during most possible sanitizer builds" FORCE)
6567
elseif(ADDRESS_SANITIZER_AVAILABLE_AND_ENABLED)
6668
set(CMAKE_C_FLAGS_DEBUG "${CMAKE_C_FLAGS_DEBUG} ${ADDR_SAN_FLAGS} -fno-omit-frame-pointer -fno-optimize-sibling-calls" CACHE INTERNAL "Flags used by the C compiler during address sanitizer builds." FORCE)
6769
set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} ${ADDR_SAN_FLAGS} -fno-omit-frame-pointer -fno-optimize-sibling-calls" CACHE INTERNAL "Flags used by the C++ compiler during address sanitizer builds." FORCE)
@@ -74,9 +76,5 @@ if(NOT CMAKE_CROSSCOMPILING)
7476
set(CMAKE_C_FLAGS_DEBUG "${CMAKE_C_FLAGS_DEBUG} ${UDEF_SAN_FLAGS}" CACHE INTERNAL "Flags used by the C compiler during undefined behavior sanitizer builds." FORCE)
7577
set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} ${UDEF_SAN_FLAGS}" CACHE INTERNAL "Flags used by the C++ compiler during undefined behavior sanitizer builds." FORCE)
7678
set(CMAKE_SHARED_LINKER_FLAGS_DEBUG "${CMAKE_SHARED_LINKER_FLAGS_DEBUG}" CACHE INTERNAL "Flags used for the linker during undefined behavior sanitizer builds" FORCE)
77-
elseif(ALL_SANITIZERS_AVAILABLE_AND_ENABLED)
78-
set(CMAKE_C_FLAGS_DEBUG "${CMAKE_C_FLAGS_DEBUG} ${ALL_SAN_FLAGS} -fno-omit-frame-pointer -fno-optimize-sibling-calls" CACHE INTERNAL "Flags used by the C compiler during most possible sanitizer builds." FORCE)
79-
set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} ${ALL_SAN_FLAGS} -fno-omit-frame-pointer -fno-optimize-sibling-calls" CACHE INTERNAL "Flags used by the C++ compiler during most possible sanitizer builds." FORCE)
80-
set(CMAKE_SHARED_LINKER_FLAGS_DEBUG "${CMAKE_SHARED_LINKER_FLAGS_DEBUG}" CACHE INTERNAL "Flags used for the linker during most possible sanitizer builds" FORCE)
8179
endif()
8280
endif()

‎cpr/curlholder.cpp‎

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,15 @@ CurlHolder::CurlHolder() {
2020
curl_easy_init_mutex_().unlock();
2121

2222
assert(handle);
23-
} // namespace cpr
23+
}
24+
25+
CurlHolder::CurlHolder(CurlHolder&& old) noexcept : handle(old.handle), chunk(old.chunk), resolveCurlList(old.resolveCurlList), multipart(old.multipart), error(std::move(old.error)) {
26+
// Avoid double free
27+
old.handle = nullptr;
28+
old.chunk = nullptr;
29+
old.resolveCurlList = nullptr;
30+
old.multipart = nullptr;
31+
}
2432

2533
CurlHolder::~CurlHolder() {
2634
curl_slist_free_all(chunk);
@@ -29,6 +37,28 @@ CurlHolder::~CurlHolder() {
2937
curl_easy_cleanup(handle);
3038
}
3139

40+
CurlHolder& CurlHolder::operator=(CurlHolder&& old) noexcept {
41+
// Free the previous stuff
42+
curl_slist_free_all(chunk);
43+
curl_slist_free_all(resolveCurlList);
44+
curl_mime_free(multipart);
45+
curl_easy_cleanup(handle);
46+
47+
// Move
48+
handle = old.handle;
49+
chunk = old.chunk;
50+
resolveCurlList = old.resolveCurlList;
51+
multipart = old.multipart;
52+
error = std::move(old.error);
53+
54+
// Avoid double free
55+
old.handle = nullptr;
56+
old.chunk = nullptr;
57+
old.resolveCurlList = nullptr;
58+
old.multipart = nullptr;
59+
return *this;
60+
}
61+
3262
util::SecureString CurlHolder::urlEncode(std::string_view s) const {
3363
assert(handle);
3464
char* output = curl_easy_escape(handle, s.data(), static_cast<int>(s.length()));

‎include/cpr/curlholder.h‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -33,12 +33,12 @@ struct CurlHolder {
3333
std::array<char, CURL_ERROR_SIZE> error{};
3434

3535
CurlHolder();
36-
CurlHolder(const CurlHolder& other) = default;
37-
CurlHolder(CurlHolder&& old) noexcept = default;
36+
CurlHolder(const CurlHolder& other) = delete;
37+
CurlHolder(CurlHolder&& old) noexcept;
3838
~CurlHolder();
3939

40-
CurlHolder& operator=(CurlHolder&& old) noexcept = default;
41-
CurlHolder& operator=(const CurlHolder& other) = default;
40+
CurlHolder& operator=(const CurlHolder& other) = delete;
41+
CurlHolder& operator=(CurlHolder&& old) noexcept;
4242

4343
/**
4444
* Uses curl_easy_escape(...) for escaping the given string.

‎test/CMakeLists.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@ add_cpr_test(threadpool)
7272
add_cpr_test(testUtils)
7373
add_cpr_test(connection_pool)
7474
add_cpr_test(sse)
75+
add_cpr_test(curlholder)
7576

7677
if (ENABLE_SSL_TESTS)
7778
add_cpr_test(ssl)

‎test/curlholder_tests.cpp‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
#include <gtest/gtest.h>
2+
3+
#include <utility>
4+
5+
#include "cpr/curlholder.h"
6+
7+
// Check if there is a double free in curl holder after move.
8+
// To reproduce this, run with address sanitizers enabled.
9+
// https://github.com/libcpr/cpr/issues/1286
10+
TEST(CurlholderTests, MoveOperator) {
11+
cpr::CurlHolder a;
12+
cpr::CurlHolder b;
13+
14+
a = std::move(b);
15+
}
16+
17+
int main(int argc, char** argv) {
18+
::testing::InitGoogleTest(&argc, argv);
19+
return RUN_ALL_TESTS();
20+
}

0 commit comments

Comments
 (0)