HPCC-31474 Refactor dafilesrv handling to cope with SSL/non-blocking #18437

jakesmith · 2024-03-21T13:12:57Z

Type of change:

This change is a bug fix (non-breaking change which fixes an issue).
This change is a new feature (non-breaking change which adds functionality).
This change improves the code (refactor or other change that does not change the functionality)
This change fixes warnings (the fix does not alter the functionality or the generated code)
This change is a breaking change (fix or feature that will cause existing behavior to change).
This change alters the query API (existing queries will have to be recompiled)

Checklist:

Smoketest:

Send notifications about my Pull Request position in Smoketest queue.
Test my draft Pull Request.

Testing:

mckellyln · 2024-03-28T13:49:28Z

system/jlib/jsocket.hpp

+extern jlib_decl IJSOCK_Exception* createJSocketException(int jsockErr, const char *_msg, const char *file, unsigned line);
+extern jlib_decl void throwJSockException(int jsockErr, const char *_msg, const char *file, unsigned line);
+#define THROWJSOCKEXCEPTION(exc) throwJSockException(exc, nullptr, __FILE__, __LINE__)
+#define THROWJSOCKEXCEPTION_X(exc, msg) throwJSockException(exc, msg, __FILE__, __LINE__)


THROWJSOCKETEXCEPTION_MSG() ?

agreed, will change.

mckellyln · 2024-04-01T18:19:43Z

system/security/securesocket/securesocket.cpp

-{
-    readTimeout(buf, min_size, max_size, size_read, timeoutms, false);
+                unsigned remainingMs = timer.remainingMs(timeoutMs);
+                rc = wait_read(remainingMs);


If we get a SSL_WANT_WRITE error then should we call wait_write() here instead of wait_read() ?

I guess so, I haven't seen SSL_WANT_WRITE occur in testing, but.. will change to handle either.

mckellyln · 2024-04-01T18:33:03Z

system/security/securesocket/securesocket.cpp

-            throw createJSocketException(JSOCKERR_broken_pipe, errmsg);
-    }
-    return numwritten;
+        if (err != SSL_ERROR_WANT_WRITE || !nonBlocking)


SSL_write() nonblocking can return SSL_WANT_READ in which case we can call wait_read() to see when to continue ...

agreed, will change to handle either.

mckellyln · 2024-04-01T18:37:20Z

system/jlib/jsocket.hpp

@@ -296,7 +296,7 @@ class jlib_decl ISocket : extends IInterface
                           unsigned timeout) = 0;
    virtual void   read(void* buf, size32_t size) = 0;
    virtual size32_t write(void const* buf, size32_t size) = 0; // returns amount written normally same as in size (see set_nonblock)
-    virtual size32_t writetms(void const* buf, size32_t size, unsigned timeoutms=WAIT_FOREVER) = 0;
+    virtual size32_t writetms(void const* buf, size32_t minSize, size32_t size, unsigned timeoutms=WAIT_FOREVER) = 0;


Why add a minSize arg to writetms() ? Is it to match readtms() ?

Yes, writetms was not previously used, but to make consistent added so that writetms can write a minimum of minSize before returning.
NB: not used in practice by secure socket (see comment re. SSL_MODE_ENABLE_PARTIAL_WRITE)

mckellyln · 2024-04-01T18:46:49Z

system/jlib/jsocket.cpp

+    size32_t sizeRead = 0;
+    if (0 == max_size)
+    {
+        _sizeRead = 0;


Before this change wait_read() was called which means a read(buf, 0, 0, ...) could throw socket closed or some other exception. Not sure if we ever call readtms() with a max_size of 0 but this would possibly act differently if we did.

Doubt readtms is ever called with max_size=0, but seems wrong to call wait_read if it were

mckellyln · 2024-04-02T14:06:06Z

fs/dafsserver/dafsserver.cpp

+                    if (0 == msg.length()) // 1st time
+                        msgWritePtr = (byte *)msg.reserveTruncate(sizeof(size32_t));
+                    size32_t szRead;
+                    sock->read(msgWritePtr, 1, sizeof(size32_t)-left, szRead);


why not just make the min_size sizeof(size32_t) here ?

because don't want to block, which readtms would do (loop internally), waiting for that min_size.
We have been notified there's something on the wire, but we don't know how much, could be < sizeof(size32_t)

This could probably be further improved by utilizing a very small (or 0) timeout, particular with the SSL version in mind.
In could have been notified due to low level SSL packet data, then wait inside readtms for min_size(=1).

That should be fine in any case, but it would be better not to block here in the select handler at all.
A timeout could allow it to bail out and potentially let other sockets be processed.
However, it would need a bit more refactoring of readtms and the calling code, to ensure that any bytes that had been read, were accounted for.

Yes, can we make it use readtms() with a small timeout here and below.

mckellyln · 2024-04-02T14:15:28Z

fs/dafsserver/dafsserver.cpp

-                    toread = left;
-                    msg.clear();
+                    size32_t szRead;
+                    sock->read(msgWritePtr, 1, left, szRead);


can we make min_size here left ? Either we get as much as we know we need or we time out ?

We don't want to block the select handler, so that other sockets are serviced.
This reads as much as it can then returns, to be notified to read more/rest when notified again.

mckellyln · 2024-04-02T14:26:11Z

fs/dafsserver/dafsserver.cpp

            }
-            return false;
+            return true;


do we use the return value anywhere ?

mckellyln · 2024-04-02T14:29:13Z

system/security/securesocket/securesocket.cpp

-}
-
-inline unsigned socketTimeRemaining(bool useSeconds, unsigned start, unsigned timeout)
+void CSecureSocket::readtms(void* buf, size32_t min_size, size32_t max_size, size32_t &_sizeRead, unsigned timeoutMs)


I might be inclined to make this always be nonblocking inside here so we can ensure we never block ?

why? If client code using a socket wants it to be blocking, this code should work (and block) but that's okay afaics (depends on the use case).

mckellyln

Looks really good, but I have a few comments to discuss.

jakesmith · 2024-04-02T18:12:17Z

@mckellyln - please see replies + new commit

mckellyln · 2024-04-03T17:51:01Z

system/security/securesocket/securesocket.cpp

@@ -244,7 +246,11 @@ class CSecureSocket : implements ISecureSocket, public CInterface
    //
    virtual bool set_nonblock(bool on) // returns old state
    {
-        throw MakeStringException(-1, "CSecureSocket::set_nonblock: not implemented");
+        bool prevState = m_socket->set_nonblock(on);
+        int flags = fcntl(SSL_get_fd(m_ssl), F_GETFL, 0);


minor comment - why set without checking first, and why get the flags and confirm after setting ? for example. if already nonblocking and want to set nonblocking true then just return ?
And if we want to change it, we set this in CSocket::set_nonblock() so why check it after setting it ?

true, I'll change.

jakesmith · 2024-04-04T13:34:30Z

@mckellyln - please see modified secure set_nonblock impl.

mckellyln

Approved.
Great to have added this needed functionality.

jakesmith · 2024-04-05T15:16:49Z

@ghalliday - will leave it as separate commits for now.

ghalliday

@jakesmith looks good. A few minor comments/questions.

ghalliday · 2024-04-10T11:01:28Z

system/security/securesocket/securesocket.cpp

+        }
+        else if (0 == rc)
+        {
+            if (0 == min_size) // mirror behviour of jsocket impl.


trivial: "behaviour"

system/security/securesocket/securesocket.cpp

ghalliday · 2024-04-10T11:25:33Z

system/jlib/jsocket.cpp

+            if (rc == 0)
+            {
+                state = ss_shutdown;
+                if (min_size==0) 


is if (sizeRead >= min_size) better?

I think in this case, yes, that may be better/more correct.
This condition is suppressing the graceful close in the specific case when min_size is 0 at the moment.

This kept the previous semantics.
I think it would be safe to change and do so if sizeRead >= min_size here and in securesocket

I'll change it.

ghalliday · 2024-04-10T11:28:18Z

system/jlib/jsocket.cpp

-                    goto EintrRetry;
+                    if (nonblocking && (err == JSE_WOULDBLOCK || err == EAGAIN)) // if EGAIN or EWOULDBLOCK - no more data to read
+                    {
+                        if (0 == min_size) // if min_read is 0, then whatever we have read so far is good enough (even if 0)


if (sizeRead >= min_size)?

Should this also be tested if blocking (or any error, and rc == 0)?

if (sizeRead >= min_size)?

as with the similar code in secure socket (and unlike the same test when rc==0), I think this is okay as it is, because if min_size > 0, and sizeRead > 0, then it would have already checked sizeRead >= min_size in the (rc > 0) block.

Should this also be tested if blocking (or any error, and rc == 0)?

I don't think so. Any error condition which are all conditions where rc < 0, should fire an error I think.
When nonBlocked is on, this code is checking an exception that rule where the error may be indicate blocked.

ok. Comment could be improved then to avoid confusion - since this will never have read anything so far.

ok. Comment could be improved then to avoid confusion - since this will never have read anything so far.

changed comments, and squashed.
@ghalliday - please review/merge.

jakesmith · 2024-04-10T16:15:50Z

@ghalliday - please see responses and last commit.

ghalliday

One comment on a comment, otherwise looks good. Please fix comment and squash.

ghalliday · 2024-04-11T08:43:03Z

system/jlib/jsocket.cpp

-                    goto EintrRetry;
+                    if (nonblocking && (err == JSE_WOULDBLOCK || err == EAGAIN)) // if EGAIN or EWOULDBLOCK - no more data to read
+                    {
+                        if (0 == min_size) // if min_read is 0, then whatever we have read so far is good enough (even if 0)


ok. Comment could be improved then to avoid confusion - since this will never have read anything so far.

Cleanup and refactor the jsocket and securesocket readtms/writetms and derivatives to cope with nonblocking sockets. Symantically they remain the same. e.g. readtms will not return until it has written at least min_size. Duplicate/very similar code in dedupped/consolidated into readtms and writetms 1) in non-blocking mode, readtms would fire an exception if socket not ready (EWOULDBLOCK, EAGAIN). 2) jsocket writetms would temporarily place a non-blocking socket into nonblocking mode. Now writetms takes a minSize (akin to readtms minSize). It will return if at least this much is written (but normal usage is with min=max). NB: previous writetms in both implementations were not called anywhere. 3) in non-blocking mode securesocket readtms and writetms would previously fail if blocked Signed-off-by: Jake Smith <[email protected]>

GordonSmith · 2024-04-11T18:01:18Z

system/security/securesocket/securesocket.cpp

+    if (-1 == flags) // unknown
+        nonBlocking = false;
+    else
+        nonBlocking = 0 != (flags & O_NONBLOCK);


@jakesmith this change is breaking the windows build...

jakesmith requested a review from mckellyln March 21, 2024 15:38

mckellyln reviewed Mar 28, 2024

View reviewed changes

mckellyln reviewed Apr 1, 2024

View reviewed changes

mckellyln reviewed Apr 2, 2024

View reviewed changes

jakesmith changed the title ~~Hpcc 31474 dafs refactor ssl read~~ HPCC-31474 Refactor dafilesrv handling to cope with SSL/non-blocking Apr 2, 2024

jakesmith marked this pull request as ready for review April 2, 2024 18:11

jakesmith requested a review from mckellyln April 2, 2024 18:11

mckellyln reviewed Apr 3, 2024

View reviewed changes

jakesmith requested a review from mckellyln April 4, 2024 13:34

mckellyln approved these changes Apr 4, 2024

View reviewed changes

jakesmith requested a review from ghalliday April 5, 2024 15:16

ghalliday reviewed Apr 10, 2024

View reviewed changes

jakesmith requested a review from ghalliday April 10, 2024 16:15

ghalliday approved these changes Apr 11, 2024

View reviewed changes

jakesmith force-pushed the HPCC-31474-dafs-refactor-ssl-read branch from 0e6f993 to 3a5e979 Compare April 11, 2024 14:58

ghalliday merged commit a782d9a into hpcc-systems:candidate-9.6.x Apr 11, 2024
48 of 50 checks passed

GordonSmith reviewed Apr 11, 2024

View reviewed changes

HPCC-31474 Refactor dafilesrv handling to cope with SSL/non-blocking #18437

HPCC-31474 Refactor dafilesrv handling to cope with SSL/non-blocking #18437

Conversation

jakesmith commented Mar 21, 2024 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mckellyln left a comment

Choose a reason for hiding this comment

jakesmith commented Apr 2, 2024

mckellyln Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakesmith commented Apr 4, 2024

mckellyln left a comment

Choose a reason for hiding this comment

jakesmith commented Apr 5, 2024

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakesmith commented Apr 10, 2024

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakesmith commented Mar 21, 2024 •

edited

Loading

mckellyln Apr 3, 2024 •

edited

Loading