Hi All,
There is a race condition in both the tcp and tls code (in functions tcplistener and tlslistener) when accepting new connections. If new connections arrive in very short succession, and the second (or any further) accept() call returns before the previously created thread has actually started off, these threads operate on the wrong socket. (the pointer handed to the thread points to the local variable s in [tcp|tls]listener() which gets its new value before the thread could copy its value)
Proposed change: copy the variable to the heap and use this as argument to the new thread; also new thread is responsible for the memory if spawn is successful.
Additional side-fix: tcpreadtimeout() does a select() on a write-fd with the intention to wait for readable bytes. This was fixed in 1.6.3 for tls code, but not tcp.
Patch is attached.
Best regards, Fabian
Fabian Mauchle fabian.mauchle@switch.ch wrote Thu, 15 Sep 2016 11:12:56 +0200:
Hi All,
There is a race condition in both the tcp and tls code (in functions tcplistener and tlslistener) when accepting new connections. If new connections arrive in very short succession, and the second (or any further) accept() call returns before the previously created thread has actually started off, these threads operate on the wrong socket. (the pointer handed to the thread points to the local variable s in [tcp|tls]listener() which gets its new value before the thread could copy its value)
This was identified some time ago (thanks Ralf) and https://project.nordu.net/browse/RADSECPROXY-64 was created to track the fix. Branch 'stompless' contains my proposed fix which haven't made it to master (or the maintenance) branch yet. Thanks for rediscovering it and providing a patch!
Proposed change: copy the variable to the heap and use this as argument to the new thread; also new thread is responsible for the memory if spawn is successful.
The argument is an integer. Is there anything wrong with just passing it as is? (Yes, I'm asking for review of branch 'stompless'. ;))
Additional side-fix: tcpreadtimeout() does a select() on a write-fd with the intention to wait for readable bytes. This was fixed in 1.6.3 for tls code, but not tcp.
Good catch! Committed to master and pushed. Thanks!
Also, thanks for CC to radsecproxy-bugs@nordu.net, creating an issue in the bug tracker. Very useful.
On 19/09/16 10:11, Linus Nordberg wrote:
Proposed change: copy the variable to the heap and use this as argument to the new thread; also new thread is responsible for the memory if spawn is successful.
The argument is an integer. Is there anything wrong with just passing it as is? (Yes, I'm asking for review of branch 'stompless'. ;))
I would be concerned about portability here (thought issues might be very rare or even non-existent in the real world). Casting between pointer and int is implementation-defined and might not work on all platforms. [1] gives some explanation and reference to C99 standard.
BR Fabian
[1] http://stackoverflow.com/questions/8618637/what-does-it-mean-to-convert-int-...
Fabian Mauchle fabian.mauchle@switch.ch wrote Mon, 19 Sep 2016 13:34:15 +0200:
On 19/09/16 10:11, Linus Nordberg wrote:
Proposed change: copy the variable to the heap and use this as argument to the new thread; also new thread is responsible for the memory if spawn is successful.
The argument is an integer. Is there anything wrong with just passing it as is? (Yes, I'm asking for review of branch 'stompless'. ;))
I would be concerned about portability here (thought issues might be very rare or even non-existent in the real world). Casting between pointer and int is implementation-defined and might not work on all platforms. [1] gives some explanation and reference to C99 standard.
I agree that it's not wise. I will update my branch and merge it shortly.
Thanks, Linus
-
Fabian Mauchle -
Linus Nordberg