Add detailed logging to libxrdp #1742

aquesnel · 2020-12-05T22:47:09Z

Follow up pull request from #1738 to add logs with more detailed information to the libxrdp code.

This code conflicts with #1738 and should therefore wait to to be merge until after #1738

TODO

incorporate feedback changes made in Unify logging in libxrdp #1738
update code formatting with astyle

metalefty · 2020-12-17T08:25:23Z

Removed v0.9.15 milestone based on discussion on gitter chat.
https://gitter.im/neutrinolabs/xrdp?at=5fd83fc091e8cb3e8c00b463

aquesnel · 2020-12-28T15:09:44Z

I've rebased this change to devel, and it is now ready to be reviewed.
@matt335672 I'd like your feedback for this pull request.

matt335672 · 2020-12-30T11:49:58Z

Hi @aquesnel - I've started to review this, but I'm sure you appreciate it may take me a while! What you've done looks very thorough. Thanks for structuring it the way you have - it should help a lot.

aquesnel · 2020-12-30T14:40:22Z

@matt335672 thank you for taking the time to review this big pull request.

That's a good point about this taking a lot of time just to review. I think for the next pull request like this I'll split the request by file instead of by directory so that it can be reviewed and merged incrementally.

matt335672

Three things to say up-front:-

Absolutely the first thing to say; this is an amazing piece of work. The extra commenting you've added against the spec is a significant contribution in itself, in addition to all the work you've put in to the logging.
Second thing to say is, sorry it's taken me a while. I've tried to be thorough while balancing by xrdp time against the other issues we've had come in recently.
Thirdly, I haven't come across anything I'd regard as a show-stopper here. There are a couple of minor functional changes but they all look OK. If this gets merged as-is, it will be a major step forward in any case.

Virtually all of my comments are along the lines of a query as to whether something should be a LOG() or LOG_DEVEL(). The reasoning I've adapted is that if an error is returned which will drop the connection, it should be logged. I suspect that in a lot of these cases a static analysis will show that this happens anyway. I've not done that analysis myself - you may well have done. Please treat these comments as queries only.

I've got one other general comment; there's a lot of code along the lines of ;

if (!s_check_rem(s, <some val>))
        {
            LOG_DEVEL(LOG_LEVEL_ERROR, "Not enough bytes in the stream "
                      "len %d, remaining %d", <some_val>, s_rem(s));
            ...
        }

It might be worth considering factoring that out into a macro like s_check_rem_and_log()

#define s_check_rem_and_log(s, n) \
    ( ((s)->p + (n) <= (s)->end) ? 1 : log_insufficient_in_stream(s, n) )

where log_insufficient_in_stream() is a function call or another lightweight macro wrapper which returns 0.

Please challenge me on anything you think I'm being unreasonable about - and thanks again for a great piece of work!

libxrdp/xrdp_caps.c

libxrdp/xrdp_channel.c

libxrdp/xrdp_iso.c

libxrdp/xrdp_orders_rail.c

aquesnel · 2021-01-11T06:56:42Z

@matt335672 thank for taking the time to look at the changes.

Somehow I had missed this comment last week. I'll take a deeper look and start to implement the feedback tomorrow.

aquesnel · 2021-01-23T22:04:40Z

I've implemented s_check_rem_and_log and added a message prefix to say which field is currently being parsed that is causing the error.

…ng s_check_rem_and_log()

aquesnel · 2021-01-23T22:43:56Z

@matt335672 I've finished implement the feedback you suggested. Can you please take another look and let me know if there is any additional feedback before this can be merged?
Thanks

matt335672

Few extra minor comments

libxrdp/xrdp_mcs.c

matt335672 · 2021-01-25T09:44:13Z

libxrdp/xrdp_mcs.c

-        LOG_DEVEL(LOG_LEVEL_ERROR, "MCS Protocol error: length too big. "
-                  "max length %d, len %d", 16 * 1024, len);
+        LOG(LOG_LEVEL_ERROR,
+            "Parsing [ITU-T T.125] Connect-Initial userData: length too big. "


Suggest 'invalid' rather than 'too big', as <=0 could be an error value too.

Could still be <= 0. Suggest 'invalid' rather than 'too big'.

I don't think this diff got updated when I pushed commit a736ac1 where I changed the message to say "invalid length"

https://github.com/neutrinolabs/xrdp/pull/1742/files?file-filters%5B%5D=.c#diff-03b1c835c3224786c0616fdd06554b40997d7f3f5d4cdf0cc233b3a653738e3bR488

libxrdp/xrdp_rdp.c

matt335672 · 2021-01-25T09:54:00Z

libxrdp/xrdp_sec.c

@@ -920,38 +1039,35 @@ xrdp_sec_process_logon_info(struct xrdp_sec *self, struct stream *s)
              self->rdp_layer->client_info.directory);
    LOG(LOG_LEVEL_DEBUG, "Client supplied domain: %s", self->rdp_layer->client_info.domain);
    LOG(LOG_LEVEL_DEBUG, "Client supplied username: %s", self->rdp_layer->client_info.username);
-    LOG(LOG_LEVEL_DEBUG, "Client supplied password: ommitted from the log");
+    LOG(LOG_LEVEL_DEBUG, "Client supplied password: <ommitted from log>");


ommitted -> omitted

I meant it's a spelling mistake. Should be 'omitted' rather than 'ommitted'

I don't think this diff got updated when I pushed commit a736ac1 where I changed the message to say "omitted"

https://github.com/neutrinolabs/xrdp/pull/1742/files?file-filters%5B%5D=.c#diff-ed1ef0e469b471dffd615bef5434f9a24a9e2f9045817c90f7202864a33b9312R1042

matt335672 · 2021-01-25T10:07:32Z

@aquesnel - this is really great work!

I've just reviewed your additional changes. I'm out-of-time today, but tomorrow I'll go through our previous conversation and resolve things.

libxrdp/xrdp_mcs.c

aquesnel · 2021-01-30T15:23:59Z

@matt335672 I've implemented the feedback you gave me, and resolved conflicts with devel. I've give you a chance to look at the minimal changes that you recommended before I rebase or squash these changes.

matt335672 · 2021-01-30T16:47:38Z

Thanks for that - looks good. I've made a few comments in reply. I should be able to approve this in the next few days, then it's over to @metalefty

metalefty · 2021-02-01T02:20:56Z

LGTM.

@matt335672 Feel free to merge this at your convenience.

aquesnel · 2021-02-04T01:56:19Z

@matt335672 I've implemented the suggestions you made. Please let me know if I missed anything.

Do you want me to squash the changes before you merge them?

matt335672 · 2021-02-04T10:14:36Z

That's fine - I've done a squash and merge. It results in a slightly cleaner git log.

Thanks again for this - it should make debugging quite a bit easier!

aquesnel mentioned this pull request Dec 5, 2020

Unify logging in libxrdp #1738

Merged

metalefty modified the milestone: v0.9.15 Dec 16, 2020

aquesnel mentioned this pull request Dec 24, 2020

Do not spit on the console 2nd stage (inspired by Debian) #1762

Closed

aquesnel added 25 commits December 24, 2020 19:04

Adding s_rem(s) for getting the remaining bytes in a stream

945ca0b

Adding logs to libxrdp/*

b09cc1d

Adding s_rem_out() macro

6bbdc20

Adding additional constants and to string macros

0a3b875

Adding documentation for MCS constants

08a0294

Adding logs to libxrdp/xrdp_caps.c

e761d68

Adding logs to libxrdp/libxrdp.c

bd0a130

Adding logs to libxrdp/xrdp_channel.c

459ccb1

Adding logs to libxrdp/xrdp_iso.c

5aee6e9

Adding logs to libxrdp/xrdp_mcs.c

78ed757

Adding logs to libxrdp/xrdp_mppc_enc.c

4258013

Adding logs to libxrdp/xrdp_rdp.c

ee41b40

Adding logs to libxrdp/xrdp_sec.c

b95ecbc

Adding logs to libxrdp/xrdp_iso.c

7042c34

Adding logs to libxrdp/xrdp_sec.c

d610caa

Adding logs to libxrdp/xrdp_rdp.c

2d4b3af

Adding logs to libxrdp/xrdp_mcs.c

8a7ee38

Adding logs to libxrdp/xrdp_iso.c

b060a45

Adding logs to libxrdp/xrdp_fastpath.c

b40b7de

Adding logs to libxrdp/xrdp_channel.c

b8249bb

Adding logs to libxrdp/*

4f0c758

Fixing unused variables warnings

dac4222

Adding logs to libxrdp/libxrdp.c

2036190

Fixing formatting with astyle

a181a5e

Adding logs to libxrdp/xrdp_orders.c

db19510

Fixing formatting with astyle

5530c91

aquesnel force-pushed the logging_add_libxrdp branch from 93c0f72 to 5530c91 Compare December 28, 2020 06:26

matt335672 mentioned this pull request Jan 6, 2021

XRDP is logging some normal conditions as ERROR messages #1775

Closed

matt335672 reviewed Jan 8, 2021

View reviewed changes

matt335672 mentioned this pull request Jan 22, 2021

Added libxrdp_get_channel_count() #1789

Merged

aquesnel added 4 commits January 23, 2021 22:27

Updating all error messages to use LOG instead of LOG_DEVEL, and addi…

0c677aa

…ng s_check_rem_and_log()

Fixing 15bpp pointer error checking

b39d2f4

Combined the 512 and 2048 bit certificate sending code paths

b217d3e

Fixing uninitialized valirables

f57b0a7

aquesnel mentioned this pull request Jan 25, 2021

Guacamole and XRDP - Fastpath and other errors. #1785

Closed

matt335672 reviewed Jan 25, 2021

View reviewed changes

matt335672 reviewed Jan 26, 2021

View reviewed changes

libxrdp/xrdp_mcs.c Show resolved Hide resolved

aquesnel added 2 commits January 30, 2021 15:07

Fixing spelling mistakes and other feedback

a736ac1

Merge branch 'devel' into logging_add_libxrdp

5d0415b

Implementing PR feedback

3d60652

matt335672 merged commit 0ec471b into neutrinolabs:devel Feb 4, 2021

aquesnel deleted the logging_add_libxrdp branch February 7, 2021 20:22

matt335672 mentioned this pull request Sep 30, 2024

Remove xrdp_sec_in_mcs_data() function #3263

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add detailed logging to libxrdp #1742

Add detailed logging to libxrdp #1742

aquesnel commented Dec 5, 2020 •

edited

Loading

metalefty commented Dec 17, 2020

aquesnel commented Dec 28, 2020

matt335672 commented Dec 30, 2020

aquesnel commented Dec 30, 2020

matt335672 left a comment

aquesnel commented Jan 11, 2021

aquesnel commented Jan 23, 2021

aquesnel commented Jan 23, 2021

matt335672 left a comment

matt335672 Jan 25, 2021

aquesnel Jan 30, 2021

matt335672 Jan 30, 2021

aquesnel Feb 4, 2021

matt335672 Jan 25, 2021

aquesnel Jan 30, 2021

matt335672 Jan 30, 2021

aquesnel Feb 4, 2021

matt335672 commented Jan 25, 2021

aquesnel commented Jan 30, 2021

matt335672 commented Jan 30, 2021

metalefty commented Feb 1, 2021

aquesnel commented Feb 4, 2021

matt335672 commented Feb 4, 2021

Add detailed logging to libxrdp #1742

Add detailed logging to libxrdp #1742

Conversation

aquesnel commented Dec 5, 2020 • edited Loading

TODO

metalefty commented Dec 17, 2020

aquesnel commented Dec 28, 2020

matt335672 commented Dec 30, 2020

aquesnel commented Dec 30, 2020

matt335672 left a comment

Choose a reason for hiding this comment

aquesnel commented Jan 11, 2021

aquesnel commented Jan 23, 2021

aquesnel commented Jan 23, 2021

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

matt335672 commented Jan 25, 2021

aquesnel commented Jan 30, 2021

matt335672 commented Jan 30, 2021

metalefty commented Feb 1, 2021

aquesnel commented Feb 4, 2021

matt335672 commented Feb 4, 2021

aquesnel commented Dec 5, 2020 •

edited

Loading