Implement read_ahead option for inet_drv.c to not overconsume kTLS data
#8690
Conversation
|
Thanks a lot for the fix! (I still hope ssl application can be changed to use the native socket module instead of the gen_tcp_socket, but it is unrelated to this issue.) |
There was a problem hiding this comment.
Since I work inside the OTP team I can commit that file.
That file has to be on the branch for the final merge to work, but we are not allowed to take a preloaded .beam file from an external contributor, for security reasons.
External contributors cannot have a preloaded file in a PR branch, and then someone in the OTP team has to build it on top of the branch, internally, cannot use the "Testing" label and has to add the branch manually to our integration branch.
But on the other hand, a security aware external entity might not like to take a preloaded file for granted from single, probably, OTP developer... Maybe I should put it in the last commit so it would be easy to back out. Although I prefer to have it in the commit that changes it's source code, which facilitate bisecting.
|
Hi @RaimoNiskanen . I tested this out (many times) with our code and the bugfix seems to remove the race condition as I am not able to trigger it anymore. Thanks for the fix! |
|
@miriampena: Great to hear! Unfortunately it is not quite done yet; the fix passes a NULL pointer to memmove in some odd case, so it causes segfaults in our daily runs. That, and |
|
SHA 405c4dd looks better in our daily builds; no more core dumps. Please re-verify! I am now working on |
|
My changes in I think I will release this as-is, an undocumented option used by SSL's kTLS distribution, and implement the I hope that will be sufficient...? I also wait for confirmation that my new version 405c4dd fixing the memmove NULL pointer problem still works as expected. Edit I cleaned up the branch to f7a4fff, with the same content. |
f7a4fff
to
b3c4eaf
Compare
|
New update; corrected where in SSL the |
|
Thanks @RaimoNiskanen . Will test this soon :) |
|
Unfortunately I can trigger the race condition with this code. |
|
Interesting... I will investigate! |
This reverts commit 45ebcd3671a7d51aba21847f9a22a8b985185a99.
b3c4eaf
to
fbafeda
Compare
|
Unfortunately our test cases does not trigger the race, and the obvious bug that inet_drv.c reads encrypted data ahead of decryption should be fixed. At least from what I can see. May there be some other bug lurking under the fixed one... I am thinking about merging this next week, and take it from there. |
|
I have merged this PR (commit fbafeda) forward to 'maint' and 'master', and it will be released in OTP-26.2.5.3 hopefully later this week. The read ahead problem for kTLS in SSL should be fixed, but there is probably some other bug that we can handle in a new Issue/PR... |
|
Continuation, full implementation, in PR #8785 |
|
Thanks a lot! Ill try to create a test that trigger the bug I am seeing. |
|
Hi, I pulled the latest code from maintenance and all seems to work fine and the bug is no longer seen. Thanks a million! ❤️ |
|
@miriampena: Oh, so the bug is fixed on OTP 26.2.5.3 and 'maint', but I see some other bug or testcase problem in our daily runs. Bummer!... Anyway; good for you! One bug less. |
Closes #8561.
This PR adds a boolean option
read_aheadto gen_tcp that affects if inet_drv.c is greedy when reading packet data.The default is the backwards compatible
{read_ahead,true}but for a connection that will switch over to kTLS it should be{read_ahead,false}or else inet_drv.c will probably, when switching over to kTLS in greediness have received some data that was for the OS TCP stack to decrypt, so the connection's TLS packet framing derails.I changed SSL code to use the option, but I guess it is used in too many places for the SSL distribution. @IngelaAndin will have to educate me on that.
This PR lacks handling the option in the
socketbackend. Documentation is also missing, for now.