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

visit unregistered websocket handler will crash civetweb #901

Closed
quhezheng opened this issue Aug 14, 2020 · 8 comments
Closed

visit unregistered websocket handler will crash civetweb #901

quhezheng opened this issue Aug 14, 2020 · 8 comments

Comments

@quhezheng
Copy link

quhezheng commented Aug 14, 2020

version 1.11

Register WS handler by CivetServer::AddWebsocketHandler() is OK, visit the WB handler is OK.
But if visit the WRONG WS address, civetweb will crash, call stack:

handle_websocket_request.isra
handle_request
process_new_connection
worker_thread
start_thread
clone

@bel2125
Copy link
Member

bel2125 commented Aug 14, 2020

But if visit the WRONG WS address, civetweb will crash

What do you mean by "visit the wrong websocket address"? You try to open a websocket connection to an ws://host/mywebsock URL, where there is no handler for "/mywebsock" registered? There never was an handler for this URL or there was a handler but you unregistered it?

@quhezheng
Copy link
Author

But if visit the WRONG WS address, civetweb will crash

What do you mean by "visit the wrong websocket address"? You try to open a websocket connection to an ws://host/mywebsock URL, where there is no handler for "/mywebsock" registered? There never was an handler for this URL or there was a handler but you unregistered it?

Tried to open a websocket connection to an ws://host/mywebsock URL, but there is no handler for "/mywebsock" registered.

bel2125 added a commit that referenced this issue Aug 14, 2020
@bel2125
Copy link
Member

bel2125 commented Aug 14, 2020

Just checked with the current HEAD version as well as with V1.11, but did not see any crash.
I used the same websocket.xhtml test file as in the test/ directory, but changed the websocket URL to something non-existing.
Could you provide some steps to reproduce this issue?

@quhezheng
Copy link
Author

quhezheng commented Aug 15, 2020

I played the trick during the compiling process by adding 'MG_LEGACY_INTERFACE'
I editded include/civetweb.h by adding following to line 25

#ifndef MG_LEGACY_INTERFACE
#define MG_LEGACY_INTERFACE
#endif

make lib WITH_CPP=1 WITH_WEBSOCKET=1 WITH_ZLIB=1

@bel2125
Copy link
Member

bel2125 commented Aug 15, 2020

I added the MG_LEGACY_INTERFACE, fixed some compile issues (old LEGAGY_INTERFACEs are untested), and successfully tested it with embedded_cpp:

make lib WITH_CPP=1 WITH_WEBSOCKET=1 WITH_ZLIB=1
cd examples/embedded_cpp
make WITH_CPP=1 WITH_WEBSOCKET=1 WITH_ZLIB=1
cp ../../test/websocket_wrong_url.xhtml .
./embedded_cpp
--> open browser: http://localhost:8081/websocket_wrong_url.xhtml

No crash.
I cannot help as long as I cannot reproduce the problem.

Probably you could turn off optimization, add debug information (use a debug build) to get line numbers.
The "*.isra" extension in your call stack tells my, you are using at least -O2 (see here). If you replace -O2 (-Os, -O3) by "-Og -g -ggdb", you should get line numbers in the calls stack, and if you get a core file even values of variables at the crash.

@quhezheng
Copy link
Author

quhezheng commented Aug 17, 2020

I added the MG_LEGACY_INTERFACE, fixed some compile issues (old LEGAGY_INTERFACEs are untested), and successfully tested it with embedded_cpp:

make lib WITH_CPP=1 WITH_WEBSOCKET=1 WITH_ZLIB=1
cd examples/embedded_cpp
make WITH_CPP=1 WITH_WEBSOCKET=1 WITH_ZLIB=1
cp ../../test/websocket_wrong_url.xhtml .
./embedded_cpp
--> open browser: http://localhost:8081/websocket_wrong_url.xhtml

No crash.
I cannot help as long as I cannot reproduce the problem.

Probably you could turn off optimization, add debug information (use a debug build) to get line numbers.
The "*.isra" extension in your call stack tells my, you are using at least -O2 (see here). If you replace -O2 (-Os, -O3) by "-Og -g -ggdb", you should get line numbers in the calls stack, and if you get a core file even values of variables at the crash.

I walked around the crash by bypassing the block

#if defined(MG_LEGACY_INTERFACE)

changed to an undefined macro as below :
#if defined(__MG_LEGACY_INTERFACE__)

The crash is no longer anymore

@bel2125
Copy link
Member

bel2125 commented Aug 17, 2020

Interesting you ran into this if branch, while I did not in my test, that should do the same thing.

Anyway, this part of the code is declared "legacy" since August 19, 2015 - so this code should have been removed already, since the tests for this have already been removed. I will remove some of the code that has been declared legacy for 3+ years - this corresponds to the solution you have already found.

@bel2125
Copy link
Member

bel2125 commented Sep 21, 2020

The legacy code that might have caused this issue has been removed.

@bel2125 bel2125 closed this as completed Sep 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants