Stack buffer overflow

[zottce]

There is a critical bug with CongestionHandler.
You can easily get STATUS_STACK_BUFFER_OVERRUN error on Windows with like 3000 virtual connections. Also this can be reproduced on Linux servers. Today I have bad feedback because my application causes crashes on real working servers.

This test is able to show what I'm talking about

    #[test]
    fn billion_virtual_connections() {
        let mut connections = Vec::with_capacity(3000);
        
        for i in 0..connections.capacity() {
            connections.push(create_virtual_connection());
        }
    }

Error message I got from cargo:

memory allocation of 1572840 bytes failederror: test failed, to rerun pass '--lib'

Caused by:
  process didn't exit successfully: `D:\sources\rust\laminar\target\debug\deps\laminar-c1edf1cab4d7715a.exe` (exit code: 0xc0000409, STATUS_STACK_BUFFER_OVERRUN)

[kstrafe]

Works just fine on linux. I increased it to 30k and it just consumes a lot of memory. Are you running out of memory? Is the OOM killer invoked? On my machine it consume somewhere around 5-6 GB of RAM.

[zottce]

Yeah, I see it's really consumes so much memory. I don't think it's an OOM killer. I'm stuck with 32 system because of a main application (my app is a dynamic library).
Is it fine to just disable this handler?

[kstrafe]

If you're on a 32-bit system then you could address at most 4 GB, maybe more with extended addressing, can you check if it's OOM? Not sure what you mean by "disable this handler".

[zottce]

I know about this 4 GB limitation and it is a reason of crashes. I need to find workaround to use this cool UDP library.
I mean can I replace current implementation with empty functions like this? Will it affect something internal?

pub struct CongestionHandler {}

impl CongestionHandler {
    pub fn new(config: &Config) -> CongestionHandler {
        CongestionHandler {}
    }

    pub fn process_incoming(&mut self, _incoming_seq: u16) {}

    pub fn process_outgoing(&mut self, _seq: u16, _time: Instant) {}
}

[kstrafe]

Where are you getting the idea from that it's the congestion handler? Can you check the kernel log for OOM messages?

[zottce]

I got a crash log with backtrace (there is an external library that gets it when the app crashes) and it looks like this:

std::alloc::rust_oom
alloc::alloc::handle_alloc_error
< ... some internal memory allocations ..  >
SequenceBuffer::with_capacity
CongestionHandler::new

And what about the kernel log I'll talk to the server administrator.

Also I don't get why CongestionHandler should be used. It literally does nothing but drains memory.

[kstrafe]

That indeed looks like OOM. You could try editing the capacity of the internal sequencebuffer to be smaller. Then each sequence buffer can grow as demand requires it. This will incur some more allocations during run-time though. Currently we do

congestion_data: SequenceBuffer::with_capacity(<u16>::max_value()),

Maybe that data structure needs to be changed to a HashSet instead since 65536 elements of type CongestionData which contains Instant and SequenceNumber, on unix platforms this is 64 + 16 bits so 10 bytes in total, meaning each congestion handler allocates ~655 kB which is quite a lot.

So we can try to make the capacity lower, or replace the functionality by a HashMap or HashSet.

[zottce]

It's about ~766 kB for each connection, because SequenceBuffer does two allocations. These allocations exist even the connection isn't active, so ... It is not so hard to drain all memory in server (especially for 32 bit systems), make a small program using laminar and try to connect to server and done the server is dead.

So, the questions is can I remove this CongestionHandler while you're fixing this? Because as I said there is no usage of the handler. Also there is no algorithm to extend internal buffers in SequenceBuffer implementation.

[kstrafe]

I'm not fixing this since I don't need it. You can try it yourself. Try removing it and check if all tests succeed. It would be appreciated if you could make a pull request for any fix you find.

[moien007]

Please mention 32bit in the title.