don't return -1 if the socket was closed

[moonbreon]

Summary

SSL_read() in netdata_ssl_read() returning 0 bytes is currently being treated as an error, and returning -1 instead. If the connection is closed on the other end, 0 bytes should be returned to the calling method, so the connection is properly closed on both sides.

Test Plan

Check that connector response methods now correctly handle a closed socket and don't treat 0 returns bytes as an error case.

Additional Information

For users: How does this change affect me?

[CLAassistant]

All committers have signed the CLA.

[tkatsoulas]

cc @MrZammler @ktsaou

[ktsaou]

According to man SSL_real():

<=0: The read operation was not successful, because either the connection was closed, an error occurred or action must be taken by the calling process. Call SSL_get_error(3) with the return value ret to find out the reason.

So, I am not sure this PR does the right thing. Returning zero and -1 should both close the socket on our end, it just changes the handling in the code about error conditions and logs.

Still, I see in the code that we never actually return zero (EOF). So, we should do something about it. But I am not sure a zero return from SSL_read means EOF.

@tkatsoulas and @MrZammler please setup a test to see what exactly is happening. If netdata is not cleaning up ssl sockets properly, it is a major problem that needs to be fixed.

@ilyam8 this is related to openssl and I know you are trying to trace an SSL related problem when netdata is compiled under muslc. Please check this. Does it improve your situation?

[moonbreon]

That's fair. I think the better fix here, at least for problem I'm seeing, would be to add an explicit check for SSL_ERROR_ZERO_RETURN and return bytes (which would be 0 and that would cause the upstream call to close the connection properly).

I can update the PR with those changes if you'd like, or I can hold off until further investigation is completed?

[ilyam8]

@ilyam8 this is related to openssl and I know you are trying to trace an SSL related problem when netdata is compiled under muslc. Please check this. Does it improve your situation?

I had the same thought and tried it - it doesn't help.

[ktsaou]

That's fair. I think the better fix here, at least for problem I'm seeing, would be to add an explicit check for SSL_ERROR_ZERO_RETURN and return bytes (which would be 0 and that would cause the upstream call to close the connection properly).

@moonbreon yes, this seems more reasonable. And I think it would be immediately mergeable.

[moonbreon]

@ktsaou - I added an explicit check for zero SSL_ERROR_ZERO_RETURN. I also closed the socket in the final else case because every time I've tested this I've gotten back an error that would require closing the socket. Apart from explicitly handling every other possible error case, I think this is the best approach. The big risk with not closing it in the final else case is that with such a high probability the socket has already been closed on the other side (and nothing else to read), you basically sit in a while(true) { netdata_log_error() } and the log file blows up very quickly.

[ktsaou]

@moonbreon make this little change and this is ready to be merged

[moonbreon]

@moonbreon make this little change and this is ready to be merged

I'm not seeing any change requests?

[ktsaou]

Sorry... I didn't finalize the review.
Just change the return code to zero.

[thiagoftsm]

PR is working as expected. Streaming, Cloud and Exporters had expected results when I connected a child and reboot parent and exporting.
@ktsaou the last commit addressed the issue you raised, when you have a time, please, take a look.
Thank you very much @moonbreon !

[ilyam8]

Do we need the same change for netdata_ssl_write()?

[ktsaou]

Do we need the same change for netdata_ssl_write()?

According to the write() docs:

Zero: It's technically possible for write() to return zero, indicating that zero bytes were written. However, for a
socket, this generally does not happen unless the size of the data to be written is zero.

So, I think is not needed there.

[ktsaou]

Merged! Thank you @moonbreon !