drdynvc: Improve dynamic channel support

The dynamic channel handler in xrdp_channel.c is updated to allow
the procs `data_first` pointer to be NULL. If this is done, the
channel handler performs all the dechunking necessary for the channel,
and only complete data PDUs are passed to procs 'data' callback.

This facility is applied to the dynamic channels supported by xrdp_mm.c.
The incoming callbacks for these channels now provide complete support
for the specification in [MS-RDPEDYC]. The existing channels were
incomplete in these respects:
1) The "Microsoft::Windows::RDS::Graphics" channel handler did not
   support incoming PDUs between 1591 and 1600 bytes. The specification
   calls for these to be sent as a single DATA_FIRST PDU.
2) The "Microsoft::Windows::RDS::DisplayControl" channel handler did
   not support incoming PDUs over 1590 bytes.

common/xrdp_constants.h

@@ -87,6 +87,9 @@
 #define XRDP_MAX_BITMAP_CACHE_IDX 2000
 #define XRDP_BITMAP_CACHE_ENTRIES 2048
 
+/* Max number of dynamic channels we support */
+#define DRDYNVC_CHANNEL_COUNT 256
+
 /*
  * Constants come from ITU-T Recommendations
  */

libxrdp/libxrdp.h

@@ -143,6 +143,7 @@ struct xrdp_drdynvc
     int status; /* see XRDP_DRDYNVC_STATUS_* */
     int flags;
     int pad0;
+    struct dyn_dechunker *dc; // Use to dechunk fragments
     int (*open_response)(struct xrdp_process *id, int chan_id,
                          int creation_status);
     int (*close_response)(struct xrdp_process *id, int chan_id);
@@ -161,7 +162,7 @@ struct xrdp_channel
     int drdynvc_channel_id;
     int drdynvc_state;
     struct vc_dechunker *drdynvc_dc;
-    struct xrdp_drdynvc drdynvcs[256];
+    struct xrdp_drdynvc drdynvcs[DRDYNVC_CHANNEL_COUNT];
 };
 
 /* rdp */

libxrdp/libxrdpinc.h

@@ -91,6 +91,9 @@ struct xrdp_drdynvc_procs
     int (*open_response)(struct xrdp_process *id, int chan_id,
                          int creation_status);
     int (*close_response)(struct xrdp_process *id, int chan_id);
+    // Only set data_first if you want to be responsible for
+    // defragging your own PDUs. Otherwise the channel
+    // process will do it for you.
     int (*data_first)(struct xrdp_process *id, int chan_id, struct stream *s,
                       int total_bytes);
     int (*data)(struct xrdp_process *id, int chan_id, struct stream *s);

libxrdp/xrdp_channel.c

@@ -74,11 +74,16 @@ xrdp_channel_create(struct xrdp_sec *owner, struct xrdp_mcs *mcs_layer)
 void
 xrdp_channel_delete(struct xrdp_channel *self)
 {
+    int i;
     if (self == 0)
     {
         return;
     }
     vc_dechunker_free(self->drdynvc_dc);
+    for (i = 0 ; i < DRDYNVC_CHANNEL_COUNT ; ++i)
+    {
+        dyn_dechunker_free(self->drdynvcs[i].dc);
+    }
     g_memset(self, 0, sizeof(struct xrdp_channel));
     g_free(self);
 }
@@ -338,10 +343,10 @@ drdynvc_process_open_channel_response(struct xrdp_channel *self,
     in_uint32_le(s, creation_status); /* CreationStatus */
     LOG_DEVEL(LOG_LEVEL_TRACE, "Received [MS-RDPEDYC] DYNVC_CREATE_RSP "
               "ChannelId %d, CreationStatus %d", chan_id, creation_status);
-    if (chan_id > 255)
+    if (chan_id >= DRDYNVC_CHANNEL_COUNT)
     {
-        LOG(LOG_LEVEL_ERROR, "Received [MS-RDPEDYC] DYNVC_CREATE_RSP for an "
+        LOG(LOG_LEVEL_ERROR, "Received [MS-RDPEDYC] DYNVC_CREATE_RSP "
-            "invalid channel id. Max allowed 255, received %d", chan_id);
+            "for an invalid channel id %d", chan_id);
         return 1;
     }
 
@@ -354,6 +359,8 @@ drdynvc_process_open_channel_response(struct xrdp_channel *self,
     else
     {
         drdynvc->status = XRDP_DRDYNVC_STATUS_CLOSED;
+        dyn_dechunker_free(drdynvc->dc);
+        drdynvc->dc = NULL;
     }
     LOG_DEVEL(LOG_LEVEL_DEBUG,
               "Dynamic Virtual Channel %s (%d) updated: status = %s",
@@ -392,15 +399,28 @@ drdynvc_process_close_channel_response(struct xrdp_channel *self,
     LOG_DEVEL(LOG_LEVEL_TRACE, "Received [MS-RDPEDYC] DYNVC_CLOSE "
               "ChannelId %d", chan_id);
     session = self->sec_layer->rdp_layer->session;
-    if (chan_id > 255)
+    if (chan_id >= DRDYNVC_CHANNEL_COUNT)
     {
         LOG(LOG_LEVEL_ERROR, "Received message for an invalid "
-            "channel id. channel id %d", chan_id);
+            "channel id %d", chan_id);
         return 1;
     }
 
     drdynvc = self->drdynvcs + chan_id;
+
+    if (dyn_dechunker_pending(drdynvc->dc))
+    {
+        // The last PDU wasn't completed
+        LOG(LOG_LEVEL_WARNING,
+            "Dynamic Virtual Channel %s (%d) closing with outstanding data",
+            XRDP_DRDYNVC_CHANNEL_ID_TO_NAME(self, chan_id),
+            chan_id);
+    }
+
     drdynvc->status = XRDP_DRDYNVC_STATUS_CLOSED;
+    dyn_dechunker_free(drdynvc->dc);
+    drdynvc->dc = NULL;
+
     LOG_DEVEL(LOG_LEVEL_DEBUG,
               "Dynamic Virtual Channel %s (%d) updated: status = %s",
               XRDP_DRDYNVC_CHANNEL_ID_TO_NAME(self, chan_id),
@@ -426,75 +446,113 @@ static int
 drdynvc_process_data_first(struct xrdp_channel *self,
                            int cmd, struct stream *s)
 {
-    struct xrdp_session *session;
     uint32_t chan_id;
-    int len;
+    int rv = 0;
-    int bytes;
-    int total_bytes;
-    struct xrdp_drdynvc *drdynvc;
-
     if (drdynvc_get_chan_id(s, cmd, &chan_id) != 0) /* ChannelId */
     {
         LOG(LOG_LEVEL_ERROR,
-            "Parsing [MS-RDPEDYC] DYNVC_DATA_FIRST failed");
+            "Parsing [MS-RDPEDYC] DYNVC_DATA_FIRST channel ID failed");
-        return 1;
+        rv = 1;
-    }
-    len = (cmd >> 2) & 0x03;
-    if (len == 0)
-    {
-        if (!s_check_rem_and_log(s, 1, "Parsing [MS-RDPEDYC] DYNVC_DATA_FIRST"))
-        {
-            return 1;
-        }
-        in_uint8(s, total_bytes); /* Length */
-    }
-    else if (len == 1)
-    {
-        if (!s_check_rem_and_log(s, 2, "Parsing [MS-RDPEDYC] DYNVC_DATA_FIRST"))
-        {
-            return 1;
-        }
-        in_uint16_le(s, total_bytes); /* Length */
     }
     else
     {
-        if (!s_check_rem_and_log(s, 4, "Parsing [MS-RDPEDYC] DYNVC_DATA_FIRST"))
+        int len = (cmd >> 2) & 0x03;
+        int total_bytes;
+        switch (len)
         {
-            return 1;
+            case 0:
+                // Technically this can't happen as DATA_FIRST is only used for
+                // PDUs over 1590 bytes ([MS-RDPEDYC] 2.2.3)
+                if (!s_check_rem(s, 1))
+                {
+                    goto short_pdu;
+                }
+                in_uint8(s, total_bytes); /* Length */
+                break;
+
+            case 1:
+                if (!s_check_rem(s, 2))
+                {
+                    goto short_pdu;
+                }
+                in_uint16_le(s, total_bytes); /* Length */
+                break;
+
+            case 2:
+                if (!s_check_rem(s, 4))
+                {
+                    goto short_pdu;
                 }
                 in_uint32_le(s, total_bytes); /* Length */
+                break;
+
+            default:
+                LOG(LOG_LEVEL_ERROR,
+                    "[MS-RDPEDYC] DYNVC_DATA_FIRST has bad Len field");
+                return 1;
         }
-    bytes = (int) (s->end - s->p);
+
         LOG_DEVEL(LOG_LEVEL_TRACE, "Received [MS-RDPEDYC] DYNVC_DATA_FIRST "
                   "ChannelId %d, Length %d, Data (omitted from the log)",
                   chan_id, total_bytes);
 
-    // See [MS-RDPBCGR] 2.2.3
+        if (chan_id >= DRDYNVC_CHANNEL_COUNT)
-    if (total_bytes < 1590 || bytes > total_bytes)
         {
-        LOG(LOG_LEVEL_ERROR,
+            LOG(LOG_LEVEL_ERROR, "Received [MS-RDPEDYC] DYNVC_DATA_FIRST "
-            "Badly formed DYNVC_DATA_FIRST PDU received on dynamic channel %d",
+                "for an invalid channel id %d", chan_id);
-            chan_id);
+            rv = 1;
-        return 1;
         }
+        else
+        {
+            struct xrdp_drdynvc *drdynvc = self->drdynvcs + chan_id;
 
-    session = self->sec_layer->rdp_layer->session;
-    if (chan_id > 255)
-    {
-        LOG(LOG_LEVEL_ERROR, "Received [MS-RDPEDYC] DYNVC_DATA_FIRST for an "
-            "invalid channel id. Max allowed 255, received %d", chan_id);
-        return 1;
-    }
-    drdynvc = self->drdynvcs + chan_id;
             if (drdynvc->data_first != NULL)
             {
-        return drdynvc->data_first(session->id, chan_id, s, total_bytes);
+                // Caller has requested to defragment PDUs themself
+                struct xrdp_session *session =
+                        self->sec_layer->rdp_layer->session;
+                rv = drdynvc->data_first(session->id, chan_id, s, total_bytes);
             }
-    LOG_DEVEL(LOG_LEVEL_WARNING, "Dynamic Virtual Channel %s (%d): "
+            else
-              "callback 'data_first' is NULL",
+            {
-              XRDP_DRDYNVC_CHANNEL_ID_TO_NAME(self, chan_id),
+                // Get the dechunker working on the stream
-              chan_id);
+                enum dyn_dechunker_status status;
-    return 0;
+                status = dyn_dechunker_process_first_chunk(drdynvc->dc,
+                         s, total_bytes);
+                switch (status)
+                {
+                    case E_DYN_INLINE_CHUNK:
+                        // Pass through as data PDU
+                        if (drdynvc->data != NULL)
+                        {
+                            struct xrdp_session *session =
+                                    self->sec_layer->rdp_layer->session;
+                            rv = drdynvc->data(session->id, chan_id, s);
+                        }
+                        break;
+
+                    case E_DYN_IN_PROGRESS:
+                        break;
+
+                    case E_DYN_ERROR:
+                        rv = 1;
+                        break;
+
+                    default:
+                        LOG(LOG_LEVEL_ERROR,
+                            "Dechunker returned unknown error %d",
+                            (int)status);
+                        rv = 1;
+                }
+            }
+        }
+    }
+
+    return rv;
+
+short_pdu:
+    LOG(LOG_LEVEL_ERROR, "[MS-RDPEDYC] DYNVC_DATA_FIRST is too short");
+    return 1;
 }
 
 /*****************************************************************************/
@@ -505,37 +563,86 @@ static int
 drdynvc_process_data(struct xrdp_channel *self,
                      int cmd, struct stream *s)
 {
-    struct xrdp_session *session;
+    int rv = 0;
     uint32_t chan_id;
-    int bytes;
-    struct xrdp_drdynvc *drdynvc;
 
     if (drdynvc_get_chan_id(s, cmd, &chan_id) != 0) /* ChannelId */
     {
-        LOG(LOG_LEVEL_ERROR, "drdynvc_process_data: drdynvc_get_chan_id failed");
+        LOG(LOG_LEVEL_ERROR,
-        return 1;
+            "Parsing [MS-RDPEDYC] DYNVC_DATA channel ID failed");
+        rv = 1;
     }
-    bytes = (int) (s->end - s->p);
+    else
+    {
         LOG_DEVEL(LOG_LEVEL_TRACE, "Received [MS-RDPEDYC] DYNVC_DATA "
-              "ChannelId %d, (re-assembled) Length %d, Data (omitted from the log)",
+                  "ChannelId %d, Length %d, Data (omitted from the log)",
-              chan_id, bytes);
+                  chan_id, s_rem(s));
-    session = self->sec_layer->rdp_layer->session;
+        if (chan_id >= DRDYNVC_CHANNEL_COUNT)
-    if (chan_id > 255)
         {
             LOG(LOG_LEVEL_ERROR, "Received DYNVC_DATA PDU for an invalid "
-            "channel id. channel id %d", chan_id);
+                "channel id %d", chan_id);
-        return 1;
+            rv = 1;
         }
-    drdynvc = self->drdynvcs + chan_id;
+        else
+        {
+            struct stream *ls = NULL; // Set if the application to be called
+            int free_ls = 0;    // Set if we need to clear ls when we're done
+            struct xrdp_drdynvc *drdynvc = self->drdynvcs + chan_id;
+            if (drdynvc->data_first != NULL)
+            {
+                // Caller is processing all PDUs directly
+                ls = s;
+            }
+            else
+            {
+                // Pass the PDU to the dechunker
+                enum dyn_dechunker_status dechunker_status =
+                    dyn_dechunker_process_data_chunk(drdynvc->dc, s);
+                switch (dechunker_status)
+                {
+                    case E_DYN_INLINE_CHUNK:
+                        ls = s;
+                        break;
+
+                    case E_DYN_IN_PROGRESS:
+                        break;
+
+                    case E_DYN_READY:
+                        ls = dyn_dechunker_get_stream(drdynvc->dc);
+                        // We now own the stream, so must delete it
+                        free_ls = 1;
+                        break;
+
+                    case E_DYN_ERROR:
+                        // Error has been logged
+                        rv = 1;
+                        break;
+
+                    default:
+                        LOG(LOG_LEVEL_ERROR,
+                            "Dechunker returned unknown error %d",
+                            (int)dechunker_status);
+                        rv = 1;
+                }
+            }
+
+            if (ls != NULL)
+            {
                 if (drdynvc->data != NULL)
                 {
-        return drdynvc->data(session->id, chan_id, s);
+                    struct xrdp_session *session =
+                            self->sec_layer->rdp_layer->session;
+                    rv = drdynvc->data(session->id, chan_id, ls);
                 }
-    LOG_DEVEL(LOG_LEVEL_WARNING, "Dynamic Virtual Channel %s (%d): "
+
-              "callback 'data' is NULL",
+                if (free_ls)
-              XRDP_DRDYNVC_CHANNEL_ID_TO_NAME(self, chan_id),
+                {
-              chan_id);
+                    free_stream(ls);
-    return 0;
+                }
+            }
+        }
+    }
+    return rv;
 }
 
 /*****************************************************************************/
@@ -819,6 +926,7 @@ xrdp_channel_drdynvc_open(struct xrdp_channel *self, const char *name,
     int total_data_len;
     int static_flags;
     char *cmd_ptr;
+    struct dyn_dechunker *dc = NULL;
 
     make_stream(s);
     init_stream(s, 8192);
@@ -826,8 +934,18 @@ xrdp_channel_drdynvc_open(struct xrdp_channel *self, const char *name,
     {
         LOG(LOG_LEVEL_ERROR,
             "xrdp_channel_drdynvc_open: xrdp_channel_init failed");
-        free_stream(s);
+        goto cleanup;
-        return 1;
+    }
+
+    // If there's no 'data_first' proc, we need a dechunker for the
+    // channel
+    if (procs->data_first == NULL)
+    {
+        if ((dc = dyn_dechunker_init(name)) == NULL)
+        {
+            // Error logged
+            goto cleanup;
+        }
     }
     cmd_ptr = s->p;
     out_uint8(s, 0); /* set later */
@@ -835,14 +953,12 @@ xrdp_channel_drdynvc_open(struct xrdp_channel *self, const char *name,
     while (self->drdynvcs[ChId].status != XRDP_DRDYNVC_STATUS_CLOSED)
     {
         ChId++;
-        if (ChId > 255)
+        if (ChId >= DRDYNVC_CHANNEL_COUNT)
         {
             LOG(LOG_LEVEL_ERROR,
                 "Attempting to create a new channel when the maximum "
-                "number of channels have already been created. "
+                "number of channels have already been created.");
-                "XRDP only supports 255 open channels.");
+            goto cleanup;
-            free_stream(s);
-            return 1;
         }
     }
     cbChId = drdynvc_insert_uint_124(s, ChId); /* ChannelId */
@@ -864,17 +980,30 @@ xrdp_channel_drdynvc_open(struct xrdp_channel *self, const char *name,
     {
         LOG(LOG_LEVEL_ERROR,
             "Sending [MS-RDPEDYC] DYNVC_CREATE_REQ failed");
-        free_stream(s);
+        goto cleanup;
-        return 1;
     }
-    free_stream(s);
+
+    if (procs->data == NULL)
+    {
+        // Not expected. Log this once.
+        LOG(LOG_LEVEL_WARNING, "Dynamic Virtual Channel %s (%d): "
+            "callback 'data' is NULL", name, ChId);
+    }
+
     *chan_id = ChId;
     self->drdynvcs[ChId].open_response = procs->open_response;
     self->drdynvcs[ChId].close_response = procs->close_response;
     self->drdynvcs[ChId].data_first = procs->data_first;
     self->drdynvcs[ChId].data = procs->data;
     self->drdynvcs[ChId].status = XRDP_DRDYNVC_STATUS_OPEN_SENT;
+    self->drdynvcs[ChId].dc = dc;
+    free_stream(s);
     return 0;
+
+cleanup:
+    free_stream(s);
+    dyn_dechunker_free(dc);
+    return 1;
 }
 
 /*****************************************************************************/
@@ -892,9 +1021,9 @@ xrdp_channel_drdynvc_close(struct xrdp_channel *self, int chan_id)
     int static_flags;
     char *cmd_ptr;
 
-    if ((chan_id < 0) || (chan_id > 255))
+    if ((chan_id < 0) || (chan_id >= DRDYNVC_CHANNEL_COUNT))
     {
-        LOG(LOG_LEVEL_ERROR, "Attempting to close an invalid channel id. "
+        LOG(LOG_LEVEL_ERROR, "Attempting to close an invalid "
             "channel id %d", chan_id);
         return 1;
     }
@@ -962,10 +1091,10 @@ xrdp_channel_drdynvc_data_first(struct xrdp_channel *self, int chan_id,
     int static_flags;
     char *cmd_ptr;
 
-    if ((chan_id < 0) || (chan_id > 255))
+    if ((chan_id < 0) || (chan_id >= DRDYNVC_CHANNEL_COUNT))
     {
         LOG(LOG_LEVEL_ERROR, "Attempting to send data to an invalid "
-            "channel id. channel id %d", chan_id);
+            "channel id %d", chan_id);
         return 1;
     }
     if (self->drdynvcs[chan_id].status != XRDP_DRDYNVC_STATUS_OPEN)
@@ -1034,10 +1163,10 @@ xrdp_channel_drdynvc_data(struct xrdp_channel *self, int chan_id,
     int static_flags;
     char *cmd_ptr;
 
-    if ((chan_id < 0) || (chan_id > 255))
+    if ((chan_id < 0) || (chan_id >= DRDYNVC_CHANNEL_COUNT))
     {
         LOG(LOG_LEVEL_ERROR, "Attempting to send data to an invalid "
-            "channel id. channel id %d", chan_id);
+            "channel id %d", chan_id);
         return 1;
     }
     if (self->drdynvcs[chan_id].status != XRDP_DRDYNVC_STATUS_OPEN)

xrdp/xrdp_egfx.c

@@ -942,37 +942,11 @@ xrdp_egfx_close_response(struct xrdp_process *id, int chan_id)
     return 0;
 }
 
-/******************************************************************************/
-/* from client */
-static int
-xrdp_egfx_data_first(struct xrdp_process *id, int chan_id,
-                     struct stream *s, int total_bytes)
-{
-    struct xrdp_egfx *egfx;
-    int bytes = s_rem(s);
-
-    LOG(LOG_LEVEL_TRACE, "xrdp_egfx_data_first: bytes %d"
-        " total_bytes %d", bytes, total_bytes);
-    egfx = id->wm->mm->egfx;
-    if (egfx->s != NULL)
-    {
-        LOG(LOG_LEVEL_ERROR, "DYNVC_DATA_FIRST PDU received while"
-            " another stream is active on channel %d", chan_id);
-        return 1;
-    }
-    make_stream(egfx->s);
-    // Caller has checked total_bytes is >= 0  and bytes is < total_bytes
-    init_stream(egfx->s, total_bytes);
-    out_uint8a(egfx->s, s->p, bytes);
-    return 0;
-}
-
 /******************************************************************************/
 /* from client */
 static int
 xrdp_egfx_data(struct xrdp_process *id, int chan_id, struct stream *s)
 {
-    int error;
     struct xrdp_wm *wm;
     struct xrdp_mm *mm;
     struct xrdp_egfx *egfx;
@@ -1002,30 +976,8 @@ xrdp_egfx_data(struct xrdp_process *id, int chan_id, struct stream *s)
         return 0;
     }
 
-    if (egfx->s == NULL)
-    {
     return xrdp_egfx_process(egfx, s);
 }
-    int bytes = s_rem(s);
-
-    if (!s_check_rem_out(egfx->s, bytes))
-    {
-        LOG(LOG_LEVEL_ERROR, "DYNVC_DATA PDU data overflow on channel %d",
-            chan_id);
-        return 1;
-    }
-    out_uint8a(egfx->s, s->p, bytes);
-    if (!s_check_rem_out(egfx->s, 1))
-    {
-        s_mark_end(egfx->s);
-        egfx->s->p = egfx->s->data;
-        error = xrdp_egfx_process(egfx, egfx->s);
-        free_stream(egfx->s);
-        egfx->s = NULL;
-        return error;
-    }
-    return 0;
-}
 
 /******************************************************************************/
 int
@@ -1045,7 +997,7 @@ xrdp_egfx_create(struct xrdp_mm *mm, struct xrdp_egfx **egfx)
     }
     procs.open_response = xrdp_egfx_open_response;
     procs.close_response = xrdp_egfx_close_response;
-    procs.data_first = xrdp_egfx_data_first;
+    procs.data_first = NULL; // Defragging handled elsewere
     procs.data = xrdp_egfx_data;
     process = mm->wm->pro_layer;
     error = libxrdp_drdynvc_open(process->session,

xrdp/xrdp_egfx.h

@@ -127,7 +127,6 @@ struct xrdp_egfx
     int channel_id;
     int surface_id;
     int frame_id;
-    struct stream *s;
     void *user;
     struct xrdp_egfx_bulk *bulk;
     int (*caps_advertise)(void *user, int num_caps, int *version, int *flags);

xrdp/xrdp_mm.c

@@ -1044,15 +1044,6 @@ dynamic_monitor_close_response(struct xrdp_process *id, int chan_id)
     return 0;
 }
 
-/******************************************************************************/
-static int
-dynamic_monitor_data_first(struct xrdp_process *id, int chan_id,
-                           struct stream *s, int total_bytes)
-{
-    LOG_DEVEL(LOG_LEVEL_TRACE, "dynamic_monitor_data_first:");
-    return 0;
-}
-
 /******************************************************************************/
 int
 advance_resize_state_machine(struct xrdp_mm *mm,
@@ -1967,7 +1958,7 @@ dynamic_monitor_initialize(struct xrdp_mm *self)
     g_memset(&d_procs, 0, sizeof(d_procs));
     d_procs.open_response = dynamic_monitor_open_response;
     d_procs.close_response = dynamic_monitor_close_response;
-    d_procs.data_first = dynamic_monitor_data_first;
+    d_procs.data_first = NULL; // Defragging handled elsewhere
     d_procs.data = dynamic_monitor_data;
     flags = 0;
     error = libxrdp_drdynvc_open(self->wm->session,