Unify monitor processing logic. #1895

Nexarian · 2021-05-25T04:12:13Z

There are two places where monitor descriptions are passed through the
RDP protocol:

TS_UD_CS_MONITOR
DISPLAYCONTROL_PDU_TYPE_MONITOR_LAYOUT

The processing logic for both of them is similar enough that they should
be unified.

Note that this is also the first step to making resizing work with the
Extension GFX channel.

matt335672 · 2021-05-25T09:36:16Z

libxrdp/xrdp_sec.c

-    int got_primary;
-    struct xrdp_client_info *client_info = (struct xrdp_client_info *)NULL;
+struct display_size_description*
+process_monitor_stream(struct stream *s, int full_parameters) {


Coding standard : Opening brace on separate line. Also, recommend using a name like xrdp_sec_process_monitor_stream so it's easy to find the function.

Fixed after merged with astyle.

matt335672 · 2021-05-25T09:37:42Z

libxrdp/xrdp_sec.c


-    client_info = &(self->rdp_layer->client_info);
+    int NumMonitor;


Coding standard : Use '_' in preference to camel-case for new variables

matt335672 · 2021-05-25T09:49:04Z

libxrdp/xrdp_sec.c

    }
    /* make sure virtual desktop size is ok */
-    if (client_info->width > 0x7FFE || client_info->width < 0xC8 ||


I appreciate that these values aren't given a name in MS-RDBBCGR, but is it worth adding defines for them for the scope of this file to improve readability and add a reference (e.g.):-

/* Desktop limits [MS-RDPBCGR] 2.2.1.3.6 */ #define MIN_VIRTUAL_DESKTOP_WIDTH 200 (etc)

Updated by adding these to ms-rdpbcgr.h

Works great.

matt335672 · 2021-05-25T09:52:56Z

libxrdp/xrdp_sec.c

+
+    struct display_size_description *description = process_monitor_stream(s, 0);
+
+    client_info->monitorCount = description->monitorCount;


Needs test for NULL return here if an invalid PDU is received.

Good catch. Fixed.

matt335672 · 2021-05-25T09:55:49Z

libxrdp/xrdp_sec.c

    {
-        LOG(LOG_LEVEL_ERROR,
+        LOG(LOG_LEVEL_INFO,


I don't understand the severity change of this message. An error seems appropriate here. Am I missing something?

I think this was leftover from my debugging while developing the change. Reverted.

matt335672 · 2021-05-25T09:57:15Z

xrdp/xrdp_mm.c

@@ -1042,27 +1042,110 @@ dynamic_monitor_data_first(intptr_t id, int chan_id, char *data, int bytes,
    return 0;
 }

+/******************************************************************************/
+static int
+process_dynamic_monitor_description(struct xrdp_wm *wm, struct display_size_description *description) {


Coding standard again, also a few lines below.

Fixed with merge to astyle.

matt335672 · 2021-05-25T09:59:09Z

xrdp/xrdp_mm.c

+        LOG(LOG_LEVEL_ERROR, "dynamic_monitor_data: MonitorLayoutSize is %d. Per spec it must be 40.", MonitorLayoutSize);
+        return 1;
+    }
+    struct display_size_description *description = process_monitor_stream(s, 1);


This can return NULL, which will break process_dynamic_monitor_description(). Suggest adding a guard here, or in that function.

Added null check to process_dynamic_monitor_description.

matt335672 · 2021-05-25T10:07:23Z

Hi @Nexarian

Thanks for this.

I've added a few review comments for you above. If there's anything you disagree with please come back at me.

As regards the coding standard, and particularly given @aquesnel's #1879, can I suggest you run the files you've modified through astyle? There's some instructions on the wiki.

Personally, all I've done in my dev environment is define ARTISTIC_STYLE_PROJECT_OPTIONS=astyle_config.as and then I run astyle on individual files. There's a bit of a trick to it in that if you don't wildcard an argument it grumbles like this:-

$ astyle os_calls.h
Recursive option with no wildcard
Did you intend quote the filename
Artistic Style has terminated

Fix by wildcarding your arg:-

$ astyle os_calls.h\*
------------------------------------------------------------
Directory  /home/mjb/xrdp/common/os_calls.h*
------------------------------------------------------------
Formatted  os_calls.h

Hope that's useful.

Nexarian · 2021-05-26T05:02:25Z

Lets wait until #1879 to merge this then so that my change has no risk of breaking the style change.

aquesnel · 2021-05-28T04:29:14Z

libxrdp/xrdp_sec.c

-            return 1;
+            goto exit_error;


I'm very much against using goto in new code, since it is prone to misuse. Even though it's usage here seems reasonable, I'd still prefer replacing it with a structured programing construct.

One option for removing the goto's is to allocate the description struct in the xrdp_sec_process_mcs_data_monitors function, since you already unconditionally free the description struct there currently.

This way of exiting a method is actually what a lot of C programmers at Microsoft use (I used to work there), and other than exiting nested loops this is the only legitimate use of goto I'm aware of.

Still, I hear you. I've refactored it so that process_monitor_stream accepts a display_size_description structure that needs to be pre-allocated by the calling functions, and since there are only two places where it's used/allocated/destroyed unconditionally, this shouldn't be a problem.

aquesnel · 2021-05-28T04:33:10Z

libxrdp/xrdp_sec.c

-        if (!s_check_rem_and_log(s, 20, "Parsing [MS-RDPBCGR] TS_UD_CS_MONITOR.TS_MONITOR_DEF"))
+        if (!s_check_rem_and_log(s, full_parameters == 0 ? 20 : 40, "Parsing monitor definitions."))


I find it very useful to have the spec and struct refence in the error message when there is a parsing failure. Can you please add the spec and struct reference in the error message that matches the struct that is being parsed.

Those values aren't directly specified in the Microsoft specification, they're derived. However, I've made an attempt to clarify where they come from and make the error messages clearer.

aquesnel · 2021-05-28T04:38:18Z

libxrdp/xrdp_sec.c

+            LOG(LOG_LEVEL_INFO, "    Index: %d, Flags 0x%8.8x Left %d Top %d "
+                "Width %d Height %d PhysicalWidth %d PhysicalHeight %d "
+                "Orientation %d DesktopScaleFactor %d DeviceScaleFactor %d",
+                monitor_index, monitor_layout->flags, monitor_layout->left,
+                monitor_layout->top, monitor_layout->width, monitor_layout->height,
+                monitor_layout->physical_width, monitor_layout->physical_height,
+                monitor_layout->orientation, monitor_layout->desktop_scale_factor,
+                monitor_layout->device_scale_factor);
        }
        else
        {
-            x1 = MIN(x1, client_info->minfo[index].left);
-            y1 = MIN(y1, client_info->minfo[index].top);
-            x2 = MAX(x2, client_info->minfo[index].right);
-            y2 = MAX(y2, client_info->minfo[index].bottom);
+            LOG_DEVEL(LOG_LEVEL_TRACE, "Received [MS-RDPBCGR] "
+                "TS_UD_CS_MONITOR.TS_MONITOR_DEF %d "
+                "left %d, top %d, right %d, bottom %d, flags 0x%8.8x",


why are these two log messages at different levels and using different formats?
can you please make them consistent, and include the spec and struct reference in the messages.

aquesnel · 2021-05-28T04:40:21Z

libxrdp/xrdp_sec.c

-    LOG_DEVEL(LOG_LEVEL_TRACE, "Received [MS-RDPBCGR] TS_UD_CS_MONITOR "
-              "flags 0x%8.8x, monitorCount %d", flags, monitorCount);


This trace log message seems to have been removed. Can you please add it back in the xrdp_sec_process_mcs_data_monitors function?

The flow of the logic has changed with this PR; making it difficult to put this somewhere elegant, but I've done my best to add it back

aquesnel · 2021-05-28T04:53:29Z

xrdp/xrdp_mm.c

+    error = libxrdp_reset(wm->session, description->session_width, description->session_height, wm->screen->bpp);
+    if (error != 0)
+    {
+        LOG(LOG_LEVEL_INFO, "dynamic_monitor_data: libxrdp_reset failed %d", error);


this seem like it warrants an error message (as well as all of the other error cases in this function). The rest of the code use the error log level for these situations.

I think I fixed this?

Nexarian · 2021-06-03T04:00:50Z

Alright, I've addressed all of the comments and then verified that the update PR works correctly and doesn't break dynamic resizing.

Take another look @metalefty @aquesnel @matt335672

aquesnel

I've done a partial code review and left a few comments. I'll complete the review when I have more time later.

aquesnel · 2021-06-03T12:51:12Z

libxrdp/xrdp_sec.c

    {
+        LOG_DEVEL(LOG_LEVEL_DEBUG, "\tprocess_monitor_stream: description was null. Valid pointer to allocated description expected.");


If we get here it should indicate a bug in the code. I think this should be logged as an error.
Also, @matt335672 suggested that we log these error conditions using LOG instead of LOG_DEVEL so that we see this error message in big reports from users.

Yes - I appreciate that a 'null pointer' error may not mean much to a user, but if it's not easy or possible to find a user-focussed answer it's better for them to have something to report than nothing at all.

Also, I agree this should be LOG_LEVEL_ERROR

Another also - please split the string into multiple string literals "..." "..." and reformat to 80 chars (or a little bit over) max line width as per the coding standard. There are quite a few other instances of this further below too. This bit of shell magic picks out the culprits:-

grep -n '' libxrdp/xrdp_sec.c | sed -ne '2307,2505p' | grep -E ':.{80}'

I appreciate that other bits of this file are also breaking the standard here, but for new code I think we should try to avoid this where possible.

aquesnel · 2021-06-03T12:58:04Z

libxrdp/xrdp_sec.c

    {
        LOG(LOG_LEVEL_ERROR,
-            "[MS-RDPBCGR] Protocol error: TS_UD_CS_MONITOR flags MUST be zero, "
-            "received: 0x%8.8x", flags);
+            "\tprocess_monitor_stream: [MS-RDPBCGR] Protocol error: TS_UD_CS_MONITOR monitorCount "


FYI You can remove the function name orchid from the log message since they are added automatically when using a debug build.

But this isn't a debug log (DEVEL) -- If it's not a debug build, does the orchid still appear?

Good point - the orchid won't appear in a non-debug build.

aquesnel · 2021-06-03T13:02:11Z

libxrdp/xrdp_sec.c

+    else
+    {
+        monitor_struct_stream_check_bytes = 40;
+        monitor_struct_stream_check_message = "\tprocess_monitor_stream: Parsing monitor definitions from 2.2.2.2.1 DISPLAYCONTROL_MONITOR_LAYOUT.";


Since the RDP spec has many extension specs, can you please include the spec name abbreviation in the message. Eg. [MS-RDPBCGR]

aquesnel

I've finished reviewing the code. Please let me know if you disagree with my suggestions.

aquesnel · 2021-06-07T01:04:59Z

libxrdp/xrdp_sec.c

        {
-            if (client_info->minfo[index].left == x1 &&
-                    client_info->minfo[index].top == y1)
+            monitor_layout = description->minfo + monitor_index;


I'm curious as to why pointer arithmetic is used here instead of array indexing?

I'm happy to do it either way, I liked the way pointer arithmetic worked better here, but if you'd prefer indexing I'll change it.

aquesnel · 2021-06-07T01:25:12Z

common/xrdp_client_info.h

+    struct monitor_info minfo[CLIENT_MONITOR_DATA_MAXIMUM_MONITORS]; /* client monitor data */
+    struct monitor_info minfo_wm[CLIENT_MONITOR_DATA_MAXIMUM_MONITORS]; /* client monitor data, non-negative values */


Can you clarify what the difference between what these two fields represent?
it seems like minfo is the client side representation of the monitor layout, and minfo_wm is the server side representation of the same monitor layout. Is that correct?

Pardon me for jumping in, but since I'm here and I've used these values I can say that that's essentially correct The _wm values are normalised for a top-left co-ordinate of (0,0), which is useful for some bits of the software, namely the VNC multimon stuff and the login window.

Exactly. Does this need further documentation in the code?

Personally I'm happy with the existing comment

aquesnel · 2021-06-07T01:27:17Z

libxrdp/xrdp_sec.c

-    if (flags != 0)
+    int num_monitor;
+    struct monitor_info *monitor_layout;
+    struct xrdp_rect rect = {8192, 8192, -8192, -8192};


Where do these initial rectangle bounds come from?
it looks like these initial value get overwritten on line 2481, am I missing something that makes these initial values important?

Also, the variable name rect isn't very descriptive, and I want to make sure I understand what's it's calculating correctly: rect's field values are in coordinate space, and rect represents the minimum sized bounding box which encloses all monitors. Is this correct?

Updated rect to all_monitors_encompassing_bounds -- You're right the initial values are from an older version of the code, so I've removed them.

aquesnel · 2021-06-07T01:30:33Z

libxrdp/xrdp_sec.c

+    const char *monitor_struct_stream_check_message;
+
+    in_uint32_le(s, num_monitor);
+    LOG(LOG_LEVEL_DEBUG, "  num_monitor %d", num_monitor);


Since this log message is for sys admins can you please add more context and expand or avoid abbreviations.

aquesnel · 2021-06-07T01:34:50Z

libxrdp/xrdp_sec.c

-            "[MS-RDPBCGR] Protocol error: TS_UD_CS_MONITOR flags MUST be zero, "
-            "received: 0x%8.8x", flags);
+            "\tprocess_monitor_stream: [MS-RDPBCGR] Protocol error: TS_UD_CS_MONITOR monitorCount "
+            "MUST be less than 16, received: %d", num_monitor);


Can you please use string formatting with the constant CLIENT_MONITOR_DATA_MAXIMUM_MONITORS instead of hard coding the value in the error message so that the error message doesn't get out-of-sync withthe constant value.

Good catch. Updated.

aquesnel · 2021-06-07T02:17:44Z

libxrdp/xrdp_sec.c

+                monitor_layout->top,
+                monitor_layout->right,
+                monitor_layout->bottom,
+                monitor_layout->is_primary);


monitor_layout->is_primary isn't assigned anywhere in the new version, but was assigned in the old version. Is this intentional?

You're right. Fixed.

aquesnel · 2021-06-07T02:31:28Z

libxrdp/xrdp_sec.c

-    if ((x2 > x1) && (y2 > y1))
+    if ((rect.right > rect.left) && (rect.bottom > rect.top))
    {
-        client_info->width = (x2 - x1) + 1;
-        client_info->height = (y2 - y1) + 1;
+        description->session_width = rect.right - rect.left;
+        description->session_height = rect.bottom - rect.top;
    }


why do we only set the session's width and height when rect is well-formed? it seems like if rect is not well-formed then xrdp should log and return an error.

Also, why was the +1 from (x2 - x1) + 1 removed?

Good point -- I added the error.

The +1 was removed because it wasn't present in the similar code that came from the resizing code, but it probably should be there as it's actually more correct.

aquesnel · 2021-06-07T02:37:51Z

libxrdp/xrdp_sec.c

+    if (description->session_width > CLIENT_MONITOR_DATA_MAXIMUM_VIRTUAL_DESKTOP_WIDTH || description->session_width < CLIENT_MONITOR_DATA_MINIMUM_VIRTUAL_DESKTOP_WIDTH ||
+            description->session_height > CLIENT_MONITOR_DATA_MAXIMUM_VIRTUAL_DESKTOP_HEIGHT || description->session_height < CLIENT_MONITOR_DATA_MINIMUM_VIRTUAL_DESKTOP_HEIGHT)


The description->session_width and description->session_height are not initialized by this function, and only conditionally set if the rect variable is well-formed. This means that this function could be logging values in the error message that were not sent by the client. Can you please fix this so that the error message always shows what was received from the client.

I think I addressed this? Let me know.

aquesnel · 2021-06-07T02:52:01Z

common/xrdp_client_info.h

 struct monitor_info
 {
+    /* From 2.2.1.3.6.1 Monitor Definition (TS_MONITOR_DEF) */
    int left;
    int top;
    int right;
    int bottom;
+    int flags;
+
+    /* From 2.2.2.2.1 DISPLAYCONTROL_MONITOR_LAYOUT */
+    int width;
+    int height;
+    int physical_width;
+    int physical_height;
+    int orientation;
+    int desktop_scale_factor;
+    int device_scale_factor;
+
+    /* Derived setting */
    int is_primary;
 };


The width and height fields are redundant with the right and bottom fields. To avoid having out-of-sync fields, I think we should only keep the right and bottom fields, and calculate width and height when needed.

Also, all of the DISPLAYCONTROL_MONITOR_LAYOUT are optional, but there is no way to determine of the values in those fields are valid or not. Can we make them always valid, and just put in some sensible default values?

I don't see where in https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpedisp/ea2de591-9203-42cd-9908-be7a55237d1c the values in the structure are optional...

I was trying to keep the data in the monitor info structure to be representative of the data sent from the client, but I agree data consistency is more important. Updated.

Sorry my comment was not clear. You are correct that all of the fields in the spec are mandatory.

What I meant by optional fields is that with the changes to the xrdp monitor_info structure, now the physical size and scaling fields are are not populated when parsing a TS_MONITOR_DEF.

I suggest that when the code parses a TS_MONITOR_DEF then all of the monitor_info field are populated with reasonable defaults, so that we don't have any undefined data in those fields under some conditions.

aquesnel · 2021-06-07T03:03:22Z

xrdp/xrdp_mm.c

+        /* 2.2.2.2 DISPLAYCONTROL_MONITOR_LAYOUT_PDU */
+        LOG(LOG_LEVEL_ERROR, "\tdynamic_monitor_data: monitor_layout_size is %d. Per spec it must be 40.", monitor_layout_size);


Per spec it must be 40.

Which spec? when I find this message in the log file, where will I find where in the spec this value is defined?
I recommend moving the code comment into the error message, so that the error message has the full context of the problem.

matt335672

Hi @Nexarian - thanks again for all the work you're putting into this. Please come back to me if you disagree with anything I've said below.

matt335672 · 2021-06-07T07:52:18Z

libxrdp/xrdp_sec.c

    {
+        LOG_DEVEL(LOG_LEVEL_DEBUG, "\tprocess_monitor_stream: description was null. Valid pointer to allocated description expected.");


Yes - I appreciate that a 'null pointer' error may not mean much to a user, but if it's not easy or possible to find a user-focussed answer it's better for them to have something to report than nothing at all.

matt335672 · 2021-06-07T08:03:30Z

common/xrdp_client_info.h

+    struct monitor_info minfo[CLIENT_MONITOR_DATA_MAXIMUM_MONITORS]; /* client monitor data */
+    struct monitor_info minfo_wm[CLIENT_MONITOR_DATA_MAXIMUM_MONITORS]; /* client monitor data, non-negative values */


Pardon me for jumping in, but since I'm here and I've used these values I can say that that's essentially correct The _wm values are normalised for a top-left co-ordinate of (0,0), which is useful for some bits of the software, namely the VNC multimon stuff and the login window.

matt335672 · 2021-06-07T08:14:04Z

common/xrdp_client_info.h

+    int session_width;
+    int session_height;
+};
+


There's some duplication here - the new struct display_size_description contains fields which are already present in this file further down.

I know. I didn't want to change xrdp_client_info since that will incur a revision of the header file and also affect xorgxrdp. I want to make the change to unify these two, but not in this PR... I want to do it for a PR that's focused on unifying these two data structures and leave the ones that change xorgxrdp more focused and linked. Make sense?

Yes that's fine- you can raise a PR now and mark it as draft if you like to keep the book-keeping simple.

I decided to actually just do it now. It's not as terrible as I thought.

matt335672 · 2021-06-07T08:24:09Z

xrdp/xrdp_mm.c

+
+/******************************************************************************/
+int
+process_monitor_stream(struct stream *s, struct display_size_description *description, int full_parameters);


See comments for xrdp_sec.c - delete this function declaration.

matt335672 · 2021-06-07T08:43:04Z

libxrdp/xrdp_sec.c

-static int
-xrdp_sec_process_mcs_data_monitors(struct xrdp_sec *self, struct stream *s)
+int
+process_monitor_stream(struct stream *s, struct display_size_description *description, int full_parameters)


To add to @aquesnel's comment - there's another more significant issue here. This function is defined and used here, but also used in xrdp_mm.c. At the moment there isn't a common declaration for this function in an include file, and so the compiler isn't able to check that the function signature is the same between xrdp_mm.c and this file. So if you added an extra parameter to this function and forgot to update xrdp_mm.c we'd end up with a difficult-to-debug situation.

The function is defined in libxrdp, and as far as I can tell, the naming convention here is to prefix global functions from this library with libxrdp_ and declare them in libxrdp/libxrdpinc.h. I don't think this is written down anywhere, and I've only just worked it out myself, but if you look at the calls that xrdp makes to libxrdp, you'll see what I mean.

So I think this function should be called libxrdp_process_monitor_stream()

Also, we're trying for new stuff to add proper function documentation headers using Doxygen-style comments. At some stage we can then start generating development docs from the code using Doxygen like FreeRDP.

My suggestion here is:-

Rename this function as libxrdp_process_monitor_stream()

Add a declaration for the function to libxrdp/libxrdpinc.h

Add function documentation to the declaration using a Doxygen-style header. Examples are in common/log.h or common/string_calls.h. Both of these are fairly new.

There's the small matter of struct display_size_description which isn't specified in libxrdp/libxrdpinc.h. One solution to this would be to move the whole struct to libxrdp/libxrdpinc.h and call it struct xrdp_display_size_description. Personally I don't think this is necessary. Since you're just using a pointer to the structure in your function prototype you can use an incomplete type declaration for the struct in libxrdp/libxrdpinc.h like this:-

/* Defined in xrdp_client_info.h */ struct display_size_description; <stuff snipped> /** * Doxygen function comment * @param s . . . * @param description . . . * @param full_parameters . . . * @return . . . */ int libxrdp_process_monitor_stream(struct stream *s, struct display_size_description *description, int full_parameters);

Let me know if that doesn't makes sense.

Makes sense! Updated.

Nexarian · 2021-06-08T05:31:48Z

Addressed comments, give it another look!

Nexarian · 2022-03-21T00:12:55Z

@aquesnel Thanks for the approve! Please take a look at neutrinolabs/xorgxrdp#212 as well!

Nexarian · 2022-03-21T00:27:32Z

@matt335672 @metalefty I've updated to include most of the comments above. What could be discussed is:

I took the liberty of redoing xrdp_client_info to both be more compliant with the spec and to prevent duplication for monitor processing. This is an important win, but I separated it out into a separate commit for clarity.
This could probably use more unit tests to make absolutely sure I'm not regressing anything, but given that multi-mon is already in flux, that could also be done later and we can take this an important change that will help inform other important changes going forward such as support set monitor info and scale xorgxrdp#172
I'm not sure what to do with the "This value MUST be ignored if..." -- That maybe LOG a warning and do nothing else? Don't populate the structure with the loaded value after first checking for the range? Right now, I simply load the value without checking anything... What is our standard here?

Please take another look! In concert with neutrinolabs/xorgxrdp#212, this works on my Ubuntu VM and I verified resizing still works.

Related to neutrinolabs/xrdp#1895

matt335672 · 2022-03-22T10:34:34Z

As regards 'MUST be ignored' values, there aren't many of them in [MS-RDPEDISP]. My suggestion is:-

PhysicalWidth/PhysicalHeight - these aren't currently used. Set to zero, and add a comment to xrdp.h that they may be zero. When/if we use them for scaling we'll have to cope with that. An alternative is to assume a particular DPI to set them, but I think that's dangerous in the long run.
Orientation - assume ORIENTATION_LANDSCAPE
DeviceScaleFactor/DesktopScaleFactor - assume 100%

That's the ignored values. Out-of-range values can just be set to the corresponding min/max value as appropriate.

How does that sound? It's only a suggestion.

matt335672

Looking great - thanks @Nexarian

From my perspective, we can merge this once we've decided on the 'must ignore' actions.

matt335672 · 2022-03-22T10:37:43Z

libxrdp/libxrdp.c

+            }
+
+            in_uint32_le(s, monitor_layout->physical_width);
+            in_uint32_le(s, monitor_layout->physical_height);


Note to implement whatever we decide to do for ignored values.

matt335672 · 2022-03-22T10:38:00Z

libxrdp/libxrdp.c

+                        ORIENTATION_LANDSCAPE_FLIPPED,
+                        ORIENTATION_PORTRAIT_FLIPPED,
+                        monitor_layout->orientation);
+            }


matt335672 · 2022-03-22T10:38:08Z

libxrdp/libxrdp.c

+            }
+
+            in_uint32_le(s, monitor_layout->desktop_scale_factor);
+            in_uint32_le(s, monitor_layout->device_scale_factor);


Nexarian · 2022-03-23T06:19:35Z

I like the idea of adding defaults in for "must ignore if" parameters. I'm working on it, I've also decided to add more unit tests. Should be finished in a day or so more.

metalefty · 2022-03-23T08:20:38Z

libxrdp/libxrdp.c

+            /*
+             * 2.2.2.2.1 DISPLAYCONTROL_MONITOR_LAYOUT
+             */
+            LOG_DEVEL(LOG_LEVEL_TRACE, "libxrdp_process_monitor_stream:"


I think the level of this log should be promoted. This is informative. What about DEVEL-INFO?

metalefty · 2022-03-23T08:23:00Z

I'm fine with other parts 'cause Matt and Aaquesnel have already reviewed well.

There are two places where monitor descriptions are passed through the RDP protocol: - TS_UD_CS_MONITOR ([MS-RDPBCGR] 2.2.1.3.6 Client Monitor Data) - DISPLAYCONTROL_PDU_TYPE_MONITOR_LAYOUT ([MS-RDPEDISP] 2.2.2.2) The processing logic for both of them is similar enough that they should be unified. Also update to define the constants for the maximum and minimum desktop width/height for monitors and total area. Also a large number of clarifications for the constants and protocol requirements. Note that this is also the first step to making resizing work with the extension GFX channel as well as an important foundational step to enable HiDPI compatibility. Also some misc logging updates.

Related to neutrinolabs/xrdp#1895

- Eliminate duplicaiton for display_size_description - monitorCount needs to be uint32_t - width/height -> session_width/session_height - Update CLIENT_INFO_CURRENT_VERSION - Also some misc unit test updates. - Minor log updates.

matt335672

I'm OK with this now - thanks @Nexarian

Related to neutrinolabs/xrdp#1895

Nexarian · 2022-03-28T21:08:45Z

@matt335672 I've updated neutrinolabs/xorgxrdp#212. Let's get these checked in, and then we can move on to my state machine changes here: #2175

matt335672 · 2022-03-29T10:00:53Z


		client_info = &(self->rdp_layer->client_info);
		int NumMonitor;


		struct display_size_description *description = process_monitor_stream(s, 0);

		client_info->monitorCount = description->monitorCount;

		if (!s_check_rem_and_log(s, 20, "Parsing [MS-RDPBCGR] TS_UD_CS_MONITOR.TS_MONITOR_DEF"))
		if (!s_check_rem_and_log(s, full_parameters == 0 ? 20 : 40, "Parsing monitor definitions."))

		LOG_DEVEL(LOG_LEVEL_TRACE, "Received [MS-RDPBCGR] TS_UD_CS_MONITOR "
		"flags 0x%8.8x, monitorCount %d", flags, monitorCount);

		{
		LOG_DEVEL(LOG_LEVEL_DEBUG, "\tprocess_monitor_stream: description was null. Valid pointer to allocated description expected.");

		struct monitor_info minfo[CLIENT_MONITOR_DATA_MAXIMUM_MONITORS]; /* client monitor data */
		struct monitor_info minfo_wm[CLIENT_MONITOR_DATA_MAXIMUM_MONITORS]; /* client monitor data, non-negative values */

		if (description->session_width > CLIENT_MONITOR_DATA_MAXIMUM_VIRTUAL_DESKTOP_WIDTH \|\| description->session_width < CLIENT_MONITOR_DATA_MINIMUM_VIRTUAL_DESKTOP_WIDTH \|\|
		description->session_height > CLIENT_MONITOR_DATA_MAXIMUM_VIRTUAL_DESKTOP_HEIGHT \|\| description->session_height < CLIENT_MONITOR_DATA_MINIMUM_VIRTUAL_DESKTOP_HEIGHT)

		/* 2.2.2.2 DISPLAYCONTROL_MONITOR_LAYOUT_PDU */
		LOG(LOG_LEVEL_ERROR, "\tdynamic_monitor_data: monitor_layout_size is %d. Per spec it must be 40.", monitor_layout_size);

Unify monitor processing logic. #1895

Unify monitor processing logic. #1895

Conversation

Nexarian commented May 25, 2021

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

matt335672 commented May 25, 2021

Nexarian commented May 26, 2021

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

Nexarian commented Jun 3, 2021 • edited Loading

aquesnel 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

aquesnel 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

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

aquesnel Jun 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matt335672 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

Nexarian commented Jun 8, 2021

Nexarian commented Jun 3, 2021 •

edited

Loading

aquesnel Jun 7, 2021 •

edited

Loading