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

Sanitization #11026

Merged
merged 9 commits into from
Jan 28, 2025
Merged

Sanitization #11026

merged 9 commits into from
Jan 28, 2025

Conversation

Ashod
Copy link
Contributor

@Ashod Ashod commented Jan 27, 2025 •
edited
Loading

  • wsd: resolve race on ChildSpawnTimeoutMs
  • wsd: use unordered_map for default config
  • wsd: avoid overflowing cleanupWait
  • wsd: signed-to-unsigned conversion
  • wsd: avoid copying strings
  • wsd: avoid overflowing unsigned index
  • wsd: sign-to-unsigned conversion
  • wsd: unsigned char to char conversion
  • wsd: signed-to-unsigned char conversion

Ashod added 9 commits January 28, 2025 07:11
@Ashod
wsd: resolve race on ChildSpawnTimeoutMs
8d26380 
The race is visible by code inspection, but
thread-sanitizer confirms as well.

WARNING: ThreadSanitizer: data race (pid=1740319)
  Write of size 4 at 0x562466da9104 by thread T2:
    #0 COOLWSD::createForKit() /home/ash/prj/lo/online/wsd/COOLWSD.cpp:2690:25 (coolwsd+0x238dc0) (BuildId: 32a3cacb8538b8135e5abf92bcea6da3f0c0b7b0)
    #1 COOLWSD::checkAndRestoreForKit() /home/ash/prj/lo/online/wsd/COOLWSD.cpp:2504:52 (coolwsd+0x237a7e) (BuildId: 32a3cacb8538b8135e5abf92bcea6da3f0c0b7b0)
    #2 PrisonPoll::wakeupHook() /home/ash/prj/lo/online/wsd/COOLWSD.cpp:2666:10 (coolwsd+0x23ce5f) (BuildId: 32a3cacb8538b8135e5abf92bcea6da3f0c0b7b0)
    #3 SocketPoll::poll(long) /home/ash/prj/lo/online/net/Socket.cpp:585:13 (coolwsd+0x52d192) (BuildId: 32a3cacb8538b8135e5abf92bcea6da3f0c0b7b0)
    #4 SocketPoll::poll(std::chrono::duration<long, std::ratio<1l, 1000000l>>) /home/ash/prj/lo/online/./net/Socket.hpp:790:61 (coolwsd+0x255400) (BuildId: 32a3cacb8538b8135e5abf92bcea6da3f0
c0b7b0)
    #5 SocketPoll::pollingThread() /home/ash/prj/lo/online/./net/Socket.hpp:954:13 (coolwsd+0x255400)
    #6 SocketPoll::pollingThreadEntry() /home/ash/prj/lo/online/net/Socket.cpp:454:9 (coolwsd+0x52b599) (BuildId: 32a3cacb8538b8135e5abf92bcea6da3f0c0b7b0)
    #7 void std::__invoke_impl<void, void (SocketPoll::*)(), SocketPoll*>(std::__invoke_memfun_deref, void (SocketPoll::*&&)(), SocketPoll*&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../.
./../include/c++/12/bits/invoke.h:74:14 (coolwsd+0x54e3eb) (BuildId: 32a3cacb8538b8135e5abf92bcea6da3f0c0b7b0)
    #8 std::__invoke_result<void (SocketPoll::*)(), SocketPoll*>::type std::__invoke<void (SocketPoll::*)(), SocketPoll*>(void (SocketPoll::*&&)(), SocketPoll*&&) /usr/bin/../lib/gcc/x86_64-
linux-gnu/12/../../../../include/c++/12/bits/invoke.h:96:14 (coolwsd+0x54e3eb)
    #9 void std::thread::_Invoker<std::tuple<void (SocketPoll::*)(), SocketPoll*>>::_M_invoke<0ul, 1ul>(std::_Index_tuple<0ul, 1ul>) /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/std_thread.h:252:13 (coolwsd+0x54e3eb)
    #10 std::thread::_Invoker<std::tuple<void (SocketPoll::*)(), SocketPoll*>>::operator()() /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/std_thread.h:259:11 (coolwsd+0x54e3eb)
    #11 std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (SocketPoll::*)(), SocketPoll*>>>::_M_run() /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/std_thread.h:210:13 (coolwsd+0x54e3eb)
    #12 <null> <null> (libstdc++.so.6+0xd44a2) (BuildId: 0c47cec75226c7736517d5acb61e373d541a5023)

  Previous read of size 4 at 0x562466da9104 by main thread (mutexes: write M0):
    #0 std::chrono::duration<long, std::ratio<1l, 1000l>>::duration<int, void>(int const&) /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/chrono.h:506:27 (coolwsd+0x23ea85) (BuildId: 32a3cacb8538b8135e5abf92bcea6da3f0c0b7b0)
    #1 COOLWSD::innerMain() /home/ash/prj/lo/online/wsd/COOLWSD.cpp:3660:34 (coolwsd+0x23ea85)
    #2 COOLWSD::main(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>> const&) /home/ash/prj/lo/online/wsd/COOLWSD.cpp:4099:23 (coolwsd+0x2437b3) (BuildId: 32a3cacb8538b8135e5abf92bcea6da3f0c0b7b0)
    #3 Poco::Util::Application::run() <null> (libPocoUtil.so.80+0x44876) (BuildId: a79feb8c0ba4d991758859f6797571382de4a434)
    #4 __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16 (libc.so.6+0x27249) (BuildId: c047672cae7964324658491e7dee26748ae5d2f8)

  Location is global 'ChildSpawnTimeoutMs' of size 4 at 0x562466da9104 (coolwsd+0x63a104)

  Mutex M0 (0x562468195f58) created at:
    #0 pthread_mutex_lock <null> (coolwsd+0x105f7a) (BuildId: 32a3cacb8538b8135e5abf92bcea6da3f0c0b7b0)
    #1 __gthread_mutex_lock(pthread_mutex_t*) /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/x86_64-linux-gnu/c++/12/bits/gthr-default.h:749:12 (coolwsd+0x23ea1e) (BuildId: 32a3cacb8538b8135e5abf92bcea6da3f0c0b7b0)
    #2 std::mutex::lock() /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/std_mutex.h:100:17 (coolwsd+0x23ea1e)
    #3 std::unique_lock<std::mutex>::lock() /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/unique_lock.h:139:17 (coolwsd+0x23ea1e)
    #4 std::unique_lock<std::mutex>::unique_lock(std::mutex&) /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/unique_lock.h:69:2 (coolwsd+0x23ea1e)
    #5 COOLWSD::innerMain() /home/ash/prj/lo/online/wsd/COOLWSD.cpp:3648:38 (coolwsd+0x23ea1e)
    #6 COOLWSD::main(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>> const&) /home/ash/prj/lo/online/wsd/COOLWSD.cpp:4099:23 (coolwsd+0x2437b3) (BuildId: 32a3cacb8538b8135e5abf92bcea6da3f0c0b7b0)
    #7 Poco::Util::Application::run() <null> (libPocoUtil.so.80+0x44876) (BuildId: a79feb8c0ba4d991758859f6797571382de4a434)
    #8 __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16 (libc.so.6+0x27249) (BuildId: c047672cae7964324658491e7dee26748ae5d2f8)

  Thread T2 'prisoner_poll' (tid=1763890, running) created by main thread at:
    #0 pthread_create <null> (coolwsd+0x10447b) (BuildId: 32a3cacb8538b8135e5abf92bcea6da3f0c0b7b0)
    #1 std::thread::_M_start_thread(std::unique_ptr<std::thread::_State, std::default_delete<std::thread::_State>>, void (*)()) <null> (libstdc++.so.6+0xd4578) (BuildId: 0c47cec75226c7736517d5acb61e373d541a5023)
    #2 COOLWSDServer::startPrisoners() /home/ash/prj/lo/online/wsd/COOLWSD.cpp:3236:23 (coolwsd+0x2511f4) (BuildId: 32a3cacb8538b8135e5abf92bcea6da3f0c0b7b0)
    #3 COOLWSD::innerMain() /home/ash/prj/lo/online/wsd/COOLWSD.cpp:3640:13 (coolwsd+0x23df3c) (BuildId: 32a3cacb8538b8135e5abf92bcea6da3f0c0b7b0)
    #4 COOLWSD::main(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>> const&) /home/ash/prj/lo/online/wsd/COOLWSD.cpp:4099:23 (coolwsd+0x2437b3) (BuildId: 32a3cacb8538b8135e5abf92bcea6da3f0c0b7b0)
    #5 Poco::Util::Application::run() <null> (libPocoUtil.so.80+0x44876) (BuildId: a79feb8c0ba4d991758859f6797571382de4a434)
    #6 __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16 (libc.so.6+0x27249) (BuildId: c047672cae7964324658491e7dee26748ae5d2f8)

SUMMARY: ThreadSanitizer: data race /home/ash/prj/lo/online/wsd/COOLWSD.cpp:2690:25 in COOLWSD::createForKit()

Change-Id: Ifee84342f02ff36141ab26bdf61b5773d48e5ccc
Signed-off-by: Ashod Nakashian <[email protected]>
@Ashod
wsd: use unordered_map for default config
9c0813d 
This cleans up ConfigUtil a little and
replaces map with unordered_map.

The primary reason for the latter change
is because memory-sanitizer reports
use-of-uninitialized-value in the map
constructor.

Change-Id: I8af0924acb50f4f42ae6672d272bc92498d5a266
Signed-off-by: Ashod Nakashian <[email protected]>
@Ashod
wsd: avoid overflowing cleanupWait
f20afe3 
UB-Sanitizer reports:

wsd/Admin.cpp:721:13: runtime error: implicit conversion from type 'int'
of value -564212 (32-bit, signed) to type 'unsigned int' changed the
value to 4294403084 (32-bit, unsigned)
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior
wsd/Admin.cpp:721:13

Change-Id: I8820bf9e07d341b74e6d115cdb79046453152b5d
Signed-off-by: Ashod Nakashian <[email protected]>
@Ashod
wsd: signed-to-unsigned conversion
2e6ca6c 
Change-Id: Ia5296143908bb680eec3e7743d6f08f358d9f856
Signed-off-by: Ashod Nakashian <[email protected]>
@Ashod
wsd: avoid copying strings
8270d24 
Change-Id: I2a9345e2623dbc054550501f07b15b2bdaf4fba3
Signed-off-by: Ashod Nakashian <[email protected]>
@Ashod
wsd: avoid overflowing unsigned index
370dde3 
Since we unconditionally decrement i,
we will always overflow when we hit 0.

kit/ForKit.cpp:409:13: runtime error: unsigned integer overflow: 0 - 1
cannot be represented in type 'size_type' (aka 'unsigned long')
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior
kit/ForKit.cpp:409:13

Also simplifies the loop body, somewhat.

Change-Id: Ibfed0a5808ba2ad3dbefee456f5572a92d48a390
Signed-off-by: Ashod Nakashian <[email protected]>
@Ashod
wsd: sign-to-unsigned conversion
41333e4 
net/Socket.hpp:1315:22: runtime error: implicit conversion from type
'ssize_t' (aka 'long') of value -1 (64-bit, signed) to type 'unsigned
long' changed the value to 18446744073709551615 (64
-bit, unsigned)
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior
net/Socket.hpp:1315:22

Change-Id: I671ffa9be66c4cc5af4baa392b23e0e6667396be
Signed-off-by: Ashod Nakashian <[email protected]>
@Ashod
wsd: unsigned char to char conversion
c1501ea 
Change-Id: I0eb18d86c32d554dca0e3e7bd528eb3b48f45485
Signed-off-by: Ashod Nakashian <[email protected]>
@Ashod
wsd: signed-to-unsigned char conversion
31470c3 
../common/Protocol.hpp:314:34: runtime error: implicit conversion from
type 'char' of value -68 (8-bit, signed) to type 'const uint8_t' (aka
'const unsigned char') changed the value to 188 (
8-bit, unsigned)
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior
../common/Protocol.hpp:314:34
../common/Protocol.hpp:318:27: runtime error: implicit conversion from
type 'uint8_t' (aka 'unsigned char') of value 188 (8-bit, unsigned) to
type 'char' changed the value to -68 (8-bit, sig
ned)
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior
../common/Protocol.hpp:318:27

Change-Id: Id5677dcd8ce0940e214b4cd4169498b70f52d1f7
Signed-off-by: Ashod Nakashian <[email protected]>
@Ashod Ashod changed the title private/ash/misc Sanitization Jan 28, 2025
@Ashod Ashod requested a review from caolanm January 28, 2025 12:13
caolanm
caolanm approved these changes Jan 28, 2025
View reviewed changes
Copy link
Contributor

@caolanm caolanm left a comment

Choose a reason for hiding this comment

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

looks good to me

@caolanm caolanm merged commit fd3c7ac into master Jan 28, 2025
14 checks passed
@caolanm caolanm deleted the private/ash/misc branch January 28, 2025 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caolanm caolanm caolanm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Collabora Online
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Ashod @caolanm