WebSocketWorker does not handle Throwable

[Marcono1234]

Describe the bug
The WebSocketWorker does not catch thrown (non-Exception) Throwables; instead they cause the worker thread to die. Even though in general catching Throwable should be avoided it seems appropriate here, otherwise an adversary could exploit a vulnerability in the application, killing one worker thread after the other until no more workers are remaining, causing a denial of service.

Even though java.lang.Error is described as:

An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch.

Some of the subclasses are thrown for non-serious problems, e.g. StackOverflowError could occur when malicious deeply nested data is deserialized.

To Reproduce
Steps to reproduce the behavior:

1. Modify the onMessage(WebSocket, String) method of the example ChatServer to throw a java.lang.Error (simulating an error in the application code)
2. Start the server
3. Run the ExampleClient multiple times
   ❌ At some point all worker threads have died

Expected behavior
WebSocketWorker should catch Throwable and log it as fatal error, but it should not terminate due to it.

Alternatively shut down the complete server so whoever started it will notice that an error occurred (and can restart the server).

Environment

• Version used: 1.5.3-SNAPSHOT (5f8d597)

Additional context
It appears in previous versions the following catch block for RuntimeExceptions would have caused a complete server shutdown, see #207:

Java-WebSocket/src/main/java/org/java_websocket/server/WebSocketServer.java

Lines 1082 to 1084 in 5f8d597

	} catch (RuntimeException e) {
	handleFatal(ws, e);
	}

However, in current versions that catch block cannot be reached (except for bugs caused by this library) since the called method doDecode catches and logs any thrown Exception.

[marci4]

Hello @Marcono1234,
could you describe why this is an issue for you?

Best regards,
Marcel

[Marcono1234]

The issue is that the worker threads just die (without being replaced). So eventually once all worker threads have died the web socket server will be unable to process any subsequent connections since no worker thread is left for processing the messages.

I think large HTTP server frameworks usually catch Throwable in their worker threads, since as described above there can be situations where a java.lang.Error occurs without it being a "serious problem", or in some cases it can even be triggered by an attacker. In such cases this can represent a denial of service vulnerability since the attacker can kill one worker thread after the other.

However, if you decide that the worker threads should not only log Throwable, then at least the complete server should terminate. This way when it is for example started in a container, the container could be restarted automatically once it notices that the server has terminated.

[PhilipRoman]

Yeah, I just tried and it can indeed bring down the entire server. The most common way would probably be AssertionError. Not sure how realistic it is to recover from other errors though.

[marci4]

So we call onError in this case and shut down everything?
Or do we need another callback?

[Marcono1234]

Or do we need another callback?

If so, then when the callback returns successfully (without throwing an exception), it would be good if the worker thread would continue running, or would be replaced. Otherwise such a callback would not add much value since still eventually all worker threads would have died.

It might also be good to verify whether a worker which has encountered a Throwable can actually continue, or whether there are any buffers or other shared state which would be in a corrupted state, and prevent the server or that worker thread from continuing.

[ange-black69]

Ok I confirm, it's the same so no solution without forking ?

[marci4]

Feel free to open a PR

[Serpion-ua]

@marci4 Hello I've created pull request for that issue #1223 could you please check it and comment? Thanks!