search

3cky / mbusd

Open-source Modbus TCP to Modbus RTU (RS-232/485) gateway.
BSD 3-Clause "New" or "Revised" License
565 stars 216 forks source link

Do not segfault when closing last connection #83

Closed vicencb closed 2 years ago

vicencb commented 2 years ago

When there is a single item in the queue, queue_next_elem returns that same and only item, so, in the function conn_close, nextconn is conn. Afterwards, conn is deleted, which is also nextconn, so, conn_close returns a pointer to a freed connection. This patch sets the return value of conn_close to NULL when there are no more connections.

3cky commented 2 years ago

Hello @vicencb!

I see your point and did some mbusd stress testing with concurrent client connections opening/closing, but still can't get a segfault.

Could you provide some steps to reproduce the problem in practice?

vicencb commented 2 years ago

Hello @3cky, dereferencing a pointer to freed memory is undefined behavior. In my case this behavior showed up as a segmentation fault, your case is different probably because of different compiler or other involved parts.

You can still see the issue by compiling the program with -fsanitize=address which is supported by both gcc and clang or else you can run mbusd with valgrind.

In order to test it, there is no need to use concurrent connections. Just do one TCP connection and then disconnect it.

dgoo2308 commented 2 years ago

@3cky the logic is sound on this request!

3cky commented 2 years ago

Valgrind did the trick. I was able to reproduce the use-after-free behavior and I confirm that this PR fixes it.

LGTM, will merge. Thanks!