sustrik / libdill

Structured concurrency in C
MIT License
1.68k stars 156 forks source link

[Question]: mrecv doesn't obey len? #172

Open intc opened 5 years ago

intc commented 5 years ago
$ git describe
2.13-7-g1a3f938

Played today with tutorial/basics/step6.c.

--- step6.c 2018-11-04 10:25:38.048573782 +0800
+++ step6a.c    2018-11-04 10:32:22.720551398 +0800
@@ -71,8 +71,9 @@
     int64_t deadline = now() + 60000;
     rc = msend(s, "What's your name?", 17, deadline);
     if(rc != 0) goto cleanup;
-    char inbuf[256];
+    char inbuf[8];
     ssize_t sz = mrecv(s, inbuf, sizeof(inbuf), deadline);
+    printf("sz: %lu\n", sz);

Compiled with gcc -ldill step6a.c -o step6a.

While running ./step6a and connecting with telnet:

What's your name?
0123456789
Hello, 01234567Hello, !

./step6a output:

active: 1      succeeded: 0      failed: 0    
sz: 10
active: 0      succeeded: 1      failed: 0

mrecv returns 10 in this case. Later in code we have inbuf[sz] = 0; which will now write outside the buffer range.

sustrik commented 5 years ago

That works as expected, although the documentation is unclear. In case the message is larger than the buffer the call succeeds but the message is truncated.

Now there's a different question whether it's a good idea to do it that way. Partial failures always suck.

intc commented 5 years ago

Ok. Yes. By reading man mrecv one could assume that sz would be set to "-1" and errno to EMSGSIZE: The data won't fit into the supplied buffer..

sustrik commented 5 years ago

Fair enough. I'll fix that.

intc commented 5 years ago

Thank you!

intc commented 5 years ago

Just for curiosity, would this be an acceptable way to implement EMSGSIZE for mrecv?

index c493bba..fea5901 100644
--- a/msock.c
+++ b/msock.c
@@ -42,7 +42,12 @@ ssize_t dill_mrecv(int s, void *buf, size_t len, int64_t deadline) {
     struct dill_msock_vfs *m = dill_hquery(s, dill_msock_type);
     if(dill_slow(!m)) return -1;
     struct dill_iolist iol = {buf, len, NULL, 0};
-    return m->mrecvl(m, &iol, &iol, deadline);
+    ssize_t sz;
+    sz = m->mrecvl(m, &iol, &iol, deadline);
+    if(sz>len){
+        errno = EMSGSIZE; return -1;
+    }
+    return sz;
 }
sustrik commented 5 years ago

You also have to close the socket when it happens.

intc commented 5 years ago

Altered step6.c:

coroutine void dialogue(int s, int ch) {
    int op = CONN_ESTABLISHED;
    int rc = chsend(ch, &op, sizeof(op), -1);
    assert(rc == 0);
    int64_t deadline = now() + 60000;
    rc = msend(s, "What's your name?", 17, deadline);
    if(rc != 0) goto cleanup;
    char inbuf[256];
    char outbuf[256];
    ssize_t sz = mrecv(s, inbuf, sizeof(inbuf)-1, deadline);
    if(sz < 0) {
        if(errno == EMSGSIZE) {
            rc = snprintf(outbuf, sizeof(outbuf),
                    "Error: Maximum length of %lu characters exceeded. Closing.\n",
                    sizeof(inbuf)-1);
            rc = msend(s, outbuf, rc, deadline);
        }
        goto cleanup;
    }
    inbuf[sz] = 0;
    rc = snprintf(outbuf, sizeof(outbuf), "Hello, %s!", inbuf);
    rc = msend(s, outbuf, rc, deadline);
    if(rc != 0) goto cleanup;
cleanup:
    op = errno == 0 ? CONN_SUCCEEDED : CONN_FAILED;
    rc = chsend(ch, &op, sizeof(op), -1);
    assert(rc == 0 || errno == ECANCELED);
    rc = hclose(s);
    assert(rc == 0);
}

Isn't it enough that we close the socket at "cleanup"?

sustrik commented 5 years ago

What I've meant was that the protocol itself should close the socket. Otherwise the socket would be in an undefined state.

intc commented 5 years ago

Reverted msock.c back to original.

dill_suffix_mrecvl seems to have this while loop:

164         while(first->iol_len == 0) {
165             if(!it.iol_next) {self->inerr = 1; errno = EMSGSIZE; return -1;}
166             it = *it.iol_next;
167         }

So EMSGSIZE and return value -1 should be set here if iol_len is exhausted.

While stepping libdill/tutorial/basics/step6 with gdb it looked like line 172 it.iol_len--; (suffix.c) would have been optimized out by the compiler (at least gdb didn't report this line while stepping). So decided to try small change (suffix.c):

168         /* Move one character to the user's iolist. */
169         if(it.iol_base) {
170             ((uint8_t*)it.iol_base)[0] = self->buf[0];
171             it.iol_base = ((uint8_t*)it.iol_base) + 1;
172             //it.iol_len--;
173             first->iol_len--; /* <- HERE */
174         }

Compiled step6.c against this and now the execution enters the while loop when the buffer has exhausted. Testing here with char inbuf[6];:

What's your name?
12345
Hello, 12345!
Connection closed by foreign host.
What's your name?
123456
Error: Maximum length of 5 characters exceeded. Closing.

Connection closed by foreign host.

And server "log":

active: 1      succeeded: 0      failed: 0    
active: 0      succeeded: 1      failed: 0    
active: 1      succeeded: 1      failed: 0    
active: 0      succeeded: 1      failed: 1  

I fail to see how the socket would be left to undefined state in this case.

Note: I'm a total noob with gdb and my c skills are outdated. The motivation to dig into these things is to learn and get a general idea of the internals of libdill so could perhaps use it in some projects.

sustrik commented 5 years ago

Hm, now I realized there's a logic behind the current behaviour.

Message-based API (mrecv) is also used for unreliable protocols (e.g. UDP), which means that failure to receive a full message doesn't disrupt the entire flow of messages. When you get a half of a message you just shrug on move on to another message.

Currently, reliable message-based transpotys mimic the behaviour: If the buffer is too small they just read the rest of the message, drop the data and move on.

I am not 100% sure about this design but that's the way it is.

intc commented 5 years ago

Ok.

I think there are two strategies on how the input buffering was dealt with (via a temporary buffer or directly to the buffer provided by the user).