Invalid rect encoding via UDP: xxxxx

kasmtech / KasmVNC

Modern VNC Server and client, web based and secure

GNU General Public License v2.0

3.06k stars 292 forks source link

Invalid rect encoding via UDP: xxxxx #190

Open DengJianShen opened 12 months ago

DengJianShen commented 12 months ago

docker run -it --network=host -e VNC_PW=password kasmweb/ubuntu-focal-desktop:1.14.0

I filled in the correct network.udp.public_ip in the kasmvnc.yaml file and WebRTC UDP Transit is enabled.

When I operate in kasmvnc, there is no rendering on the interface, and the devtool console displays "Invalid rect encoding via UDP: xxxxxx".

The reason for this error is line 20062 in the main.bundle.js file. parseInt((data[8] << 24) + (data[9] << 16) + (data[10] << 8) + data[11], 10)

I don't know if there is an error in main.bundle.js or if I am not using WebRTC UDP correctly.

Thank you.

mmcclaskey commented 11 months ago

Thank you for the feedback. I will be reviewing the WebRTC UDP feature here shortly in conjunction with dual monitor support. Just wanted to provide feedback that I saw your message and I will be looking at it, but it may be a bit before I respond with detail.

DengJianShen commented 11 months ago

Thank you for the feedback. I will be reviewing the WebRTC UDP feature here shortly in conjunction with dual monitor support. Just wanted to provide feedback that I saw your message and I will be looking at it, but it may be a bit before I respond with detail.

I think the problem should occur in the following code, but I am not sure.

this._ udpChannel.onmessage = function (e) {
    //Log.Info("got udp msg", e.data);
    var u8 = new Uint8Array(e.data); // Got an UDP packet. Do we need reassembly?

    var id = parseInt(u8[0] + (u8[1] << 8) + (u8[2] << 16) + (u8[3] << 24), 10);
    var i = parseInt(u8[4] + (u8[5] << 8) + (u8[6] << 16) + (u8[7] << 24), 10);
    var pieces = parseInt(u8[8] + (u8[9] << 8) + (u8[10] << 16) + (u8[11] << 24), 10);
    var hash = parseInt(u8[12] + (u8[13] << 8) + (u8[14] << 16) + (u8[15] << 24), 10); // TODO: check the hash. It's the low 32 bits of XXH64, seed 0

    var frame_id = parseInt(u8[16] + (u8[17] << 8) + (u8[18] << 16) + (u8[19] << 24), 10);

    if (me._transitConnectionState !== me.TransitConnectionStates.Udp) {
      me._display.clear();

      me._changeTransitConnectionState(me.TransitConnectionStates.Udp);
    }

    if (pieces == 1) {
      // Handle it immediately
      me._handleUdpRect(u8.slice(20), frame_id);
    } else {
      // Use buffer
      var now = Date.now();

      if (udpBuffer.has(id)) {
        var item = udpBuffer.get(id);
        item.recieved_pieces += 1;
        item.data[i] = u8.slice(20);
        item.total_bytes += item.data[i].length;

        if (item.total_pieces == item.recieved_pieces) {
          // Message is complete, combile data into a single array
          var finaldata = new Uint8Array(item.total_bytes);
          var z = 0;

          for (var x = 0; x < item.data.length; x++) {
            finaldata.set(item.data[x], z);
            z += item.data[x].length;
          }

          udpBuffer["delete"](id);

          me._handleUdpRect(finaldata, frame_id);
        }
      } else {
        var _item = {
          total_pieces: pieces,
          // number of pieces expected
          arrival: now,
          //time first piece was recieved
          recieved_pieces: 1,
          // current number of pieces in data
          total_bytes: 0,
          // total size of all data pieces combined
          data: new Array(pieces)
        };
        _item.data[i] = u8.slice(20);
        _item.total_bytes = _item.data[i].length;
        udpBuffer.set(id, _item);
      }
    }
}

{
      key: "_handleUdpRect",
      value: function _handleUdpRect(data, frame_id) {
        var frame = {
          x: (data[0] << 8) + data[1],
          y: (data[2] << 8) + data[3],
          width: (data[4] << 8) + data[5],
          height: (data[6] << 8) + data[7],
          encoding: parseInt((data[8] << 24) + (data[9] << 16) + (data[10] << 8) + data[11], 10)
        };
        ......
}

I would think so because I saw similarities in the way the following code and the above code handle data.

    /* New FramebufferUpdate */

    var hdr = this._sock.rQshiftBytes(12);

    this._FBU.x = (hdr[0] << 8) + hdr[1];
    this._FBU.y = (hdr[2] << 8) + hdr[3];
    this._FBU.width = (hdr[4] << 8) + hdr[5];
    this._FBU.height = (hdr[6] << 8) + hdr[7];
    this._FBU.encoding = parseInt((hdr[8] << 24) + (hdr[9] << 16) + (hdr[10] << 8) + hdr[11], 10);

So I think there is something wrong with this._ udpChannel.onmessage.

DengJianShen commented 11 months ago

Thank you for the feedback. I will be reviewing the WebRTC UDP feature here shortly in conjunction with dual monitor support. Just wanted to provide feedback that I saw your message and I will be looking at it, but it may be a bit before I respond with detail.

Hello, have you made any progress?

DengJianShen commented 10 months ago

@mmcclaskey I found that the reason may be that the value of this frame_id is incorrect, and this value is obtained through _udpChannel.onmessage.

mmcclaskey commented 10 months ago

We have identified the issue and should have it fixed in a developer preview in the next week or two.

john-deng commented 10 months ago

We have identified the issue and should have it fixed in a developer preview in the next week or two.

@mmcclaskey Great news! I also did some research on this issue, Matt, I was wondering if we can contribute more on this project, not only reporting bugs, but also writing code, fixing bugs.

I enabled the WebRTC UDP transit option and encountered some significant bugs. Below are my key findings from the source code:

issue with frame_id:

On the service side, the function udpsend assigns data to buf[16],

# kasmvnc/common/network/Udp.cxx

// Send one packet, split into N UDP-sized pieces
static uint8_t udpsend(WuClient *client, const uint8_t *data, unsigned len, uint32_t *id) {
   const uint32_t DATA_MAX = udpSize;

   uint8_t buf[1400 + sizeof(uint32_t) * 4];
   const uint32_t pieces = (len / DATA_MAX) + ((len % DATA_MAX) ? 1 : 0);

   uint32_t i;

   for (i = 0; i < pieces; i++) {
       const unsigned curlen = len > DATA_MAX ? DATA_MAX : len;
       const uint32_t hash = XXH64(data, curlen, 0);

       memcpy(buf, id, sizeof(uint32_t));
       memcpy(&buf[4], &i, sizeof(uint32_t));
       memcpy(&buf[8], &pieces, sizeof(uint32_t));
       memcpy(&buf[12], &hash, sizeof(uint32_t));

       memcpy(&buf[16], data, curlen);
       data += curlen;
       len -= curlen;

       if (WuHostSendBinary(host, client, buf, curlen + sizeof(uint32_t) * 4) < 0)
           return 1;
   }

   (*id)++;

   return 0;
}

However, on the client side, this is incorrectly assigned to frame_id. It seems to mistakenly capture the position of the rectangle instead. For example:

Here is the upd message that got in function _udpChannel.onmessage() in file kasmweb/core/rfb.js.

Address	+0	+1	+2	+3	+4	+5	+6	+7	+8	+9	+A	+B	+C	+D	+E	+F
7ffce3dee010	09	00	00	00	00	00	00	00	01	00	00	00	0c	6a	81	3c
7ffce3dee020	00	2c	01	09	00	30	00	30	00	00	00	07	b0	f6	08	52
7ffce3dee030	49	46	46	6e	04	00	00	57	45	42	50	56	50	38	20	62
7ffce3dee040	04	00	00	50	1b	00	9d	01	2a	30	00	30	00	3e	3d	1c

buf[16] -> x: 00 2c
buf[18] -> y: 01 09
buf[20] -> width: 00 30
bb buf[22] -> height: 00 30

Corresponding JavaScript snippet:

const frame_id = parseInt(u8[16] + (u8[17] << 8) + (u8[18] << 16) + (u8[19] << 24), 10);

Confusion with the UDP buffer's start index for the rectangle:
- The UDP buffer for the rectangle starts from index 20, which seems to truncate the width of the rectangle. This part is a bit confusing.
- Relevant JavaScript code:
```
me._handleUdpRect(u8.slice(20), frame_id); item.data[i] = u8.slice(20);
```

I have also attached the relevant sections of the code for a more detailed look. I would greatly appreciate your insights on these issues. Additionally, I am eager to know if there are any plans to address these in future updates.

In function _handleUdpRect, why pass frame.x + 1 as rect_cnt to this._display.flip?

_handleUdpRect(data, frame_id) {
        //...
        switch (frame.encoding) {
            case encodings.pseudoEncodingLastRect:
                this._display.flip(frame_id, frame.x + 1);  // why pass frame.x + 1 to flip here?

mmcclaskey commented 10 months ago

Sorry for the delay all. @john-deng , great reverse engineering, indeed you did find the issue. WebRTC UDP mode is currently a preview type feature and is not used by us internally. That being said, we did not catch this bug in release testing. The server side should be sending a frame number with each UDP message, this allows the client to group the rects into frames. To answer your question about why frame.x + 1. The LastRect message does not contain x, y, because it is not a rect. Rather than write code specifically for the LastRect, we merely reuse the x field to contain the Rect count for a specific frame. This information is used by display.js to know when it has recieved all rects for a given frame. We add one to the rect count because the client includes the last rect as a rect, while the server does not.

Here is a preview build of the fix, it is not yet merged into master. Please let me know your experience with this build.

john-deng commented 10 months ago

Hi Matt @mmcclaskey,

I'm excited to let you know that I've tested the preview build and am happy to report that the issue we discussed has been resolved. However, during my testing, I encountered several other issues in this build.

Here is one if the issues that I produced just now.

Regarding the frame_id and frame.x + 1, I now understand the reasoning. Thank you for your explanation.

As I mentioned previously, I'm eager to contribute further. Could you guide me on how I can access the source code for this preview? I'd like to delve deeper into these bugs to help accelerate the development of a stable release.

Looking forward to your guidance.

mmcclaskey commented 10 months ago

Our multi monitor branch has fixes to the rect queuing that may resolve the artifacts you are seeing. I'll get this UDP fix merged into master and after we get the multi-monitor branch merged in we can circle back and see where things stand.

clbr commented 10 months ago

While UDP has lower latency, the unreliability means some glitches are unavoidable. Can't tell from the image whether that is due to lost packets though.

john-deng commented 10 months ago

Our multi monitor branch has fixes to the rect queuing that may resolve the artifacts you are seeing. I'll get this UDP fix merged into master and after we get the multi-monitor branch merged in we can circle back and see where things stand.

@mmcclaskey Understood, and I appreciate the dedication and hard work that goes into maintaining such a robust open source project. Could you provide an estimated timeline for when these merged fixes will be available in the master branch?

john-deng commented 10 months ago

While UDP has lower latency, the unreliability means some glitches are unavoidable. Can't tell from the image whether that is due to lost packets though.

@clbr Thank you for the clarification. Despite UDP's limitations, we must find a way to mitigate these glitches to ensure a seamless user experience. Persistent issues like these could indeed frustrate our users. Let's explore possible solutions to address this reliability concern.