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.

Sign up for GitHub

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

Jump to bottom

Record codec GUID to identify unknown codec #2401

Merged
merged 7 commits into from
Nov 4, 2022
Merged

Record codec GUID to identify unknown codec #2401

merged 7 commits into from
Nov 4, 2022

Conversation

metalefty
Copy link
Member

Related to #2400, I was always wondering what is the unknown codec which has codec id 5 that appears in the log very often.

[2022-10-28T22:04:34.814+0900] [WARN ] xrdp_caps_process_codecs: unknown codec id 5

I added codec GUID to the log in order to identify unknown codecs. It turned out the one is Image RemoteFX.

[2022-10-29T02:34:55.707+0900] [INFO ] xrdp_caps_process_codecs: NSCodec(CA8D1BB9-000F-154F-589FAE2D1A87E2D6), codec id [1], properties len [3]
[2022-10-29T02:34:55.708+0900] [WARN ] xrdp_caps_process_codecs: unknown(2744CCD4-9D8A-4E74-803C0ECBEEA19C54), codec id [5], properties len [49]
[2022-10-29T02:34:55.710+0900] [INFO ] xrdp_caps_process_codecs: RemoteFX(76772F12-BD72-4463-AFB3B73C9C6F7886), codec id [3], properties len [49]

@matt335672 I'd appreciate it if you review the g_guid_to_string function. It is the only void function in string_calls. Should it return some value?

@metalefty metalefty requested a review from matt335672 October 28, 2022 18:00
matt335672
matt335672 reviewed Oct 31, 2022
View reviewed changes
Copy link
Member

@matt335672 matt335672 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @metalefty. Looks like a worthwhile addition. There's some duplication with an existing function which I've mentioned in my comments.

common/string_calls.c Outdated Show resolved Hide resolved
common/string_calls.c Outdated Show resolved Hide resolved
common/string_calls.c Outdated Show resolved Hide resolved
libxrdp/xrdp_caps.c
@@ -521,6 +522,7 @@ xrdp_caps_process_codecs(struct xrdp_rdp *self, struct stream *s, int len)
int i1;
char *codec_guid;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May need const struct guid *codec_guid instead if we use the function in guid.c. Also the cast will need to be changed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will do this later in this PR. Can you have a look again at other points?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good to me now.

I've read the link you've provided, and I don't think there's any difference between Microsoft's interpretation of a GUID and RFC4122. In other words, I think you can just call get rid of the existing guid_to_str() and replace it with your new function completely. The existing uses of the function should be fine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it fine to add hyphens for other use cases of guid_to_str()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing uses of the function should be fine.

Ah, you've already mentioned that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matt335672 I've replaced your guid_to_str() with my ms_guid_to_str(). Can you have a look again?

tests/common/test_string_calls.c Show resolved Hide resolved
@matt335672
Copy link
Member

Hi @metalefty.

I've done some more thinking about this. There may be some mileage in updating guid.c to generate proper UUIDs as specified by RFC4122 section 4.4.

I can look at that as a subsequent PR after this one is merged.

@metalefty
Copy link
Member Author

Moved some functions and tests to guid.{c,h}.

Microsoft's GUID is a little bit different from the common UUID.
See: https://devblogs.microsoft.com/oldnewthing/20220928-00/?p=107221

So I created a separate function ms_guid_to_str() to convert MS GUID to string.

@metalefty metalefty force-pushed the codec_guid branch 2 times, most recently from ccd94fc to c32f6f4 Compare November 1, 2022 10:19
@metalefty
Copy link
Member Author

Sorry for the noise of frequent force-pushes. I forgot to remove some unused things in string_calls.

I've done some more thinking about this. There may be some mileage in updating guid.c to generate proper UUIDs as specified by RFC4122 section 4.4.

Indeed. Current GUID generation is actually not compliant with the spec.

@matt335672
Copy link
Member

One comment added above. Also you're right - I need to fix the GUID generation.

metalefty
metalefty commented Nov 1, 2022
View reviewed changes
libxrdp/xrdp_caps.c
@@ -521,6 +522,10 @@ xrdp_caps_process_codecs(struct xrdp_rdp *self, struct stream *s, int len)
int i1;
char *codec_guid;
char *next_guid;
struct guid guid;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure char *next_guid can be replaced with struct guid so just kept.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks fine to me

matt335672
matt335672 approved these changes Nov 3, 2022
View reviewed changes
Copy link
Member

@matt335672 matt335672 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine, with the noted change to the function header comment

common/guid.h Show resolved Hide resolved
libxrdp/xrdp_caps.c
@@ -521,6 +522,10 @@ xrdp_caps_process_codecs(struct xrdp_rdp *self, struct stream *s, int len)
int i1;
char *codec_guid;
char *next_guid;
struct guid guid;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks fine to me

@metalefty metalefty merged commit cc43061 into neutrinolabs:devel Nov 4, 2022
@metalefty metalefty deleted the codec_guid branch November 4, 2022 07:13
@matt335672 matt335672 mentioned this pull request Nov 10, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matt335672 matt335672 matt335672 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@metalefty @matt335672