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.

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

wsd: fix assert fail in DocumentBroker::getJailRoot() #11025

Merged
merged 1 commit into from
Jan 27, 2025

Conversation

vmiklos
Copy link
Contributor

@vmiklos vmiklos commented Jan 27, 2025

This can happen if the websocket incoming traffic has a load command,
followed by a removesession command, so asserting a non-empty jail ID
seems to be going too far.

Backtrace:

#9 0x555e36ada9b8 in DocumentBroker::getJailRoot[abi:cxx11]() const wsd/DocumentBroker.cpp:3646:5
#10 0x555e36b49ae3 in DocumentBroker::uploadPresetsToWopiHost(Authorization const&) wsd/DocumentBroker.cpp:4094:41
#11 0x555e36b444c9 in DocumentBroker::removeSession(std::shared_ptr<ClientSession> const&) wsd/DocumentBroker.cpp:3772:9
#12 0x555e369bb709 in ClientSession::_handleInput(char const*, int) ClientSession.cpp:1088:24
#13 0x555e3703957b in Session::handleMessage(std::vector<char, std::allocator<char>> const&) common/Session.cpp

Signed-off-by: Miklos Vajna [email protected]
Change-Id: Ib9cfd4856838dfa6ba304888770898964e922260

This can happen if the websocket incoming traffic has a load command,
followed by a removesession command, so asserting a non-empty jail ID
seems to be going too far.

Backtrace:

    #9 0x555e36ada9b8 in DocumentBroker::getJailRoot[abi:cxx11]() const wsd/DocumentBroker.cpp:3646:5
    #10 0x555e36b49ae3 in DocumentBroker::uploadPresetsToWopiHost(Authorization const&) wsd/DocumentBroker.cpp:4094:41
    #11 0x555e36b444c9 in DocumentBroker::removeSession(std::shared_ptr<ClientSession> const&) wsd/DocumentBroker.cpp:3772:9
    #12 0x555e369bb709 in ClientSession::_handleInput(char const*, int) ClientSession.cpp:1088:24
    #13 0x555e3703957b in Session::handleMessage(std::vector<char, std::allocator<char>> const&) common/Session.cpp

Signed-off-by: Miklos Vajna <[email protected]>
Change-Id: Ib9cfd4856838dfa6ba304888770898964e922260
@vmiklos vmiklos requested a review from caolanm January 27, 2025 14:49
@vmiklos
Copy link
Contributor Author

vmiklos commented Jan 27, 2025

@caolanm could you please review this? Thanks.

Found by a fuzzer, we assumed that a doc is always loaded (downloaded) before the session removal message arrives, but the added sample shows this is not necessarily the case. I assume it's OK to downgrade this assert to a warning, at least the caller in the backtrace just iterates the files of the return dir, so it copes with the empty result. The assert was there for a long time, but the new ~autotext code now uses it more, I guess that's how this was found just now.

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.

I imagine this is just a matter of the fuzzer finally hitting on this, rather than a specific recent change that allows hitting this path

@caolanm caolanm merged commit c402f76 into master Jan 27, 2025
14 checks passed
@caolanm caolanm deleted the private/vmiklos/master branch January 27, 2025 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants