sociomantic-tsunami / swarm

Asynchronous client/node framework library
Boost Software License 1.0
14 stars 26 forks source link

Rethink out-of-memory handling in IStorageChannels #338

Closed gavin-norman-sociomantic closed 5 years ago

gavin-norman-sociomantic commented 6 years ago

IStorageChannels currently has some kind of weird support for catching out-of-memory errors, and setting a flag to disallow creation of more channels (see here). OutOfMemoryException is deprecated in ocean, and is an alias to an Error in D2. Catching Errors is not recommended.

We need to rethink this.

gavin-norman-sociomantic commented 5 years ago

The more robust approach, when creating a new channel, would be to check whether there's memory space available for the channel before attempting to create it. I'm not sure how feasible this is, though.

gavin-norman-sociomantic commented 5 years ago

Ok I've done some local tests with a DMQ node, allocating and filling large channels.

gavin-norman-sociomantic commented 5 years ago

Here's a rethought:

gavin-norman-sociomantic commented 5 years ago

Of course, channel creation isn't the only way we can get into an OOM situation in a node. In the DHT, for example, every record that's added causes a memory allocation that could fail due to OOM.

gavin-norman-sociomantic commented 5 years ago

Instead, creating channels is only possible via a unix socket command (i.e. by the node administrators).

This would be a slight hassle, as the command would need to be run on every node. But as this would only ever be done in careful coordination with an app reading or writing a new channel, I think the risk is minimal.

gavin-norman-sociomantic commented 5 years ago

So what I'm proposing is a 2-step fix here:

  1. Remove the handling of OutOfMemoryException. If OOM occurs in IStorageChannels, the thrown Error will cause the program to crash (just like an OOM in any other code location will).
  2. Remove the ability to create channels ad hoc. Replace this with a unix socket command to create a new channel.

Step 1 fixes the basic problem that this issue describes (OutOfMemoryException being deprecated). Step 2 fixes a node vulnerability that whereby a malicious or buggy client could crash nodes by relentlessly creating new channels.

gavin-norman-sociomantic commented 5 years ago

Remove the ability to create channels ad hoc.

All requests (both legacy and neo) would be adapted to return the Error status code, if the specified channel does not exist.

gavin-norman-sociomantic commented 5 years ago

The only problem with this suggestion, I think, is that it removes the capability for channels to be created "programmatically" (i.e. channel names generated at runtime by an application). We've never needed this capability, but we'd be in trouble if we ever did need it, after having removed it.

gavin-norman-sociomantic commented 5 years ago

Another problem came up in verbal discussion: adding new nodes to a DHT becomes hassle. (You'd need to add all extant channels.)

The conclusion of our discussion was to just remove the OOM exception handling and allow the node to crash in this situation. The burden of avoiding OOM is then on the node maintainers (which is the status quo anyway).