Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Websockets: a bug in curl_ws_recv #10438

Closed
mikeduglas opened this issue Feb 7, 2023 · 14 comments
Closed

Websockets: a bug in curl_ws_recv #10438

mikeduglas opened this issue Feb 7, 2023 · 14 comments
Assignees

Comments

@mikeduglas
Copy link
Contributor

Description

Current curl_ws_recv implementation (ws.c) works properly if a message (frame) size is less than 16K and buffer size is equal or greater than a message size, so curl_ws_recv reads entire message in one call. In other cases it returns incomplete data or hangs.

To make it working, wsp->frame.bytesleft needs to be initialized not with oleft ("bytes yet to come (for this frame)"), but with datalen + oleft ("total payload size"):

    if(!wsp->frame.bytesleft) {
      size_t headlen;
      curl_off_t oleft;
      /* detect new frame */
      result = ws_decode(data, (unsigned char *)wsp->stillb, wsp->stillblen,
                         &headlen, &datalen, &oleft, &recvflags);
      if(result == CURLE_AGAIN)
        /* a packet fragment only */
        break;
      else if(result)
        return result;
      wsp->stillb += headlen;
      wsp->stillblen -= headlen;
      wsp->frame.offset = 0;
      // wsp->frame.bytesleft = oleft;  <-- this is wrong, next line is a fix
      wsp->frame.bytesleft = datalen + oleft; /* total payload size */
      wsp->frame.flags = recvflags;
    }

I tested modified code, it works as expected.

I expected the following

curl/libcurl version

curl 7.87.1-DEV (i386-pc-win32) libcurl/7.87.1-DEV Schannel WinIDN

operating system

Windows 11

@dfandrich
Copy link
Contributor

That would explain my confused comment in #10347. That failing assert may have been pointing to this.

@bagder
Copy link
Member

bagder commented Feb 8, 2023

I don't think that is correct. The frame struct contains metadata about this frame, not about pending data in general as we provide a pointer to that struct with curl_ws_meta(). frame.bytesleft should therefor not have a larger value than number of bytes left that belongs to this frame.

@dfandrich
Copy link
Contributor

Clearly, I haven't puzzled my way through the websockets code sufficiently yet...

@bagder
Copy link
Member

bagder commented Feb 8, 2023

BTW, this really shows we should improve the test suite for WebSocket. It's too "bare bones" right now.

@mikeduglas
Copy link
Contributor Author

@bagder What is a frame?
Suppose the server sent 20K message as a single frame, with FIN bit set. libcurl receives first 16K, processes it, then receives final 4K, processes it. But both 16K and 4K fragments are the same frame from my point of view. So it is naturally that bytesLeft has the value of 20K (not 4K which it is in current code).

@bagder
Copy link
Member

bagder commented Feb 8, 2023

I mean frame as in a WebSocket frame as defined in RFC 6455.

both 16K and 4K fragments are the same frame from my point of view

If they are indeed a single 20K WebSocket frame, then the two data pieces are indeed just two parts of the same frame. The libcurl API can provide the frame to the application in two parts though.

it is naturally that bytesLeft has the value of 20K (not 4K which it is in current code).

Bytesleft should then be (20K - size of the current fragment), yes. Number of bytes left of this frame after this piece has been delivered. If this is 16K and the entire frame is 20K, then there is 4K left.

@mikeduglas
Copy link
Contributor Author

Suppose I pass 1K buffer to curl_ws_recv. If Bytesleft is initally 4K, then I will get 4K data in 4 curl_ws_recv calls, bytesLeft will decreased to 0 (4-1-1-1-1) and game over. If I call curl_ws_recv 5th time for some reason then new 4K fragment will be read, accidently decoded and bytesLeft will have random value.

@bagder
Copy link
Member

bagder commented Feb 8, 2023

Sorry, I don't follow. I understand that there is an issue here, I just think that the fix needs to be different than what you proposed above.

If I call curl_ws_recv 5th time for some reason then new 4K fragment will be read, accidently decoded and bytesLeft will have random value.

Why? When curl_ws_recv is called with bytesleft == 0, then does it not reach this code block?

curl/lib/ws.c

Lines 431 to 446 in ead2b2d

if(!wsp->frame.bytesleft) {
size_t headlen;
curl_off_t oleft;
/* detect new frame */
result = ws_decode(data, (unsigned char *)wsp->stillb, wsp->stillblen,
&headlen, &datalen, &oleft, &recvflags);
if(result == CURLE_AGAIN)
/* a packet fragment only */
break;
else if(result)
return result;
wsp->stillb += headlen;
wsp->stillblen -= headlen;
wsp->frame.offset = 0;
wsp->frame.bytesleft = oleft;
wsp->frame.flags = recvflags;

@mikeduglas
Copy link
Contributor Author

mikeduglas commented Feb 8, 2023

Why? When curl_ws_recv is called with bytesleft == 0, then does it not reach this code block?

Oops sorry you're right, in the case of bytesleft == 0 ws_decode block is not called, datalen is assigned to 0 and we get an endless loop.

I just think that the fix needs to be different than what you proposed above.

I believe that my bad English does not allow me to convince you. :-)

@mikeduglas
Copy link
Contributor Author

then the two data pieces are indeed just two parts of the same frame.

Hence wsp->frame.bytesleft should contain bytes left in the frame to be delivered to a caller. Initially (right after ws_decode call) it is 20K (full frame size), 1K of these 20K are memcopied to the 1K user's buffer, at the same time bytesleft is reduced by 1K, so the user gets back 1K in *nread and 19K in meta.bytesleft after 1st curl_ws_recv call. Next call gets a user 1K in *nread and 18K in bytesleft and so on.

@mikeduglas
Copy link
Contributor Author

badger: Why? When curl_ws_recv is called with bytesleft == 0, then does it not reach this code block?

mikeduglas: Oops sorry you're right

Actually you aren't right, the condition is "!wsp->frame.bytesleft" so when bytesleft == 0 then the condition is true and the block is reached. But it makes little difference, in any case when bytesleft is initially set to 4K, the code fails.

By the way, in struct websocket (ws.h) there is a member oleft:

  curl_off_t oleft; /* outstanding number of payload bytes left from the
                       server */

which is never used. In my example that 4K should be assigned to this wsp->oleft.

@bagder
Copy link
Member

bagder commented Feb 8, 2023

The fix is bigger than what has been discussed here. I am working on it.

@bagder bagder self-assigned this Feb 8, 2023
@bagder bagder linked a pull request Feb 9, 2023 that will close this issue
bagder added a commit that referenced this issue Feb 9, 2023
 + remove 'oleft' from the struct
 + deal with "overflow data" in a separate dynbuf

Fixes #10438
Closes #10447
@bagder
Copy link
Member

bagder commented Feb 9, 2023

#10447 has a proposed fix, and also a new test case (2305) to verify this kind of scenario.

@mikeduglas
Copy link
Contributor Author

I have tested this fix (on my test suite), it works great, thank you!

bagder added a commit that referenced this issue Feb 9, 2023
 + remove 'oleft' from the struct
 + deal with "overflow data" in a separate dynbuf

Fixes #10438
Closes #10447
bagder added a commit that referenced this issue Feb 9, 2023
 + remove 'oleft' from the struct
 + deal with "overflow data" in a separate dynbuf

Reported-by: Mike Duglas
Fixes #10438
Closes #10447
bagder added a commit that referenced this issue Feb 10, 2023
 + remove 'oleft' from the struct
 + deal with "overflow data" in a separate dynbuf

Reported-by: Mike Duglas
Fixes #10438
Closes #10447
bch pushed a commit to bch/curl that referenced this issue Jul 19, 2023
 + remove 'oleft' from the struct
 + deal with "overflow data" in a separate dynbuf

Reported-by: Mike Duglas
Fixes curl#10438
Closes curl#10447
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

3 participants