control: Limit memory use by control connections.

[mobin-2008]

Hi.

I working on this PR for resolving one of the last blockers for 0.17 release:

dinit/TODO

Lines 3 to 5 in 7d491be

	* Limit memory use by control connections. Currently clients have command responses queued without
	limit; it would be better to stop accepting new commands once a certain amount of response is
	buffered.

Signed-off-by: Mobin "Hojjat' Aydinfar <[email protected]>

[davmac314]

The idea is not to reject commands if the buffer is too full, but to stop receiving them until the buffer has had a chance to reduce. That would be done by disabling the read watch (IN_EVENTS) in the control_conn_watcher (and then enabling the read watch again once the buffer was below a threshold).

This solution as written doesn't actually address the problem that the TODO note is concerned with, which is that a malicious or badly-written client could keep stuffing the output buffer causing it to consume an excessive amount of memory. I.e. this solution requires that the buffer management is performed by the client itself, which doesn't help.

(Note that in practice a malicious client already needs to be root to be able to send commands via the control socket, so this is mostly a theoretical concern).

Just FYI there's also an error in your implementation that means it always returns DINIT_RP_IDLE. This line:

        if (sizeof(outbuf) < 1000) {

... is checking the size of the outbuf object itself, i.e. the size of the type list<vector<char>>, which is a compile-time constant (that happens to be less than 1000). It does not check the number of bytes in the buffer. A proper fix will keep track of the number of bytes currently in the buffer in a member variable. This would mean updating that variable whenever something is added to or removed from the buffer. (It would be possible to calculate the size instead of tracking it in a variable, but since that requires walking through each item in the top-level list it is not efficient).

[davmac314]

Btw. If you'd like to implementing as I've suggested please feel free to re-open this PR or create a new one - I'm just closing it for now because it's definitely not the right fix at the moment.

[mobin-2008]

I updated it to disable IN_EVENTS when outbuf vector have >= 1000 members (Is it efficient?) and enable it again when outbuf have < 1000 members. Is it fit our requirements?

[davmac314]

I updated it to disable IN_EVENTS when outbuf vector have >= 1000 members (Is it efficient?) and enable it again when outbuf have < 1000 members. Is it fit our requirements?

outbuf is a list of vectors. See the definition:

    // Buffer for outgoing packets. Each outgoing back is represented as a vector<char>.
    list<vector<char>> outbuf;

Checking its size with outbuf.size() just tells you how many vectors are in the list. It does not tell you how much data is in each of the vectors.

Also, as I mentioned earlier:

A proper fix will keep track of the number of bytes currently in the buffer in a member variable. This would mean updating that variable whenever something is added to or removed from the buffer. (It would be possible to calculate the size instead of tracking it in a variable, but since that requires walking through each item in the top-level list it is not efficient).

[davmac314]

This is on the right track.

Can you please also use a named constant (a constexpr int at the top of the file is fine) for the buffer watermark value rather than using a literal 1000. Also, 1000 is way too low. Maybe 16kb (16*1024) is reasonable.

[mobin-2008]

and It's done! I'm very excited for new 0.17 release and new episode of "Escape from System D" :)

[davmac314]

There are a few other changes to go in yet! But yes it is one step closer. 🚀