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.

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

Remove xrdp_sec_in_mcs_data() function #3263

Merged

Conversation

matt335672
Copy link
Member

Back in #1742, @aquesnel added these comments to the header of xrdp_sec_in_mcs_data():-

/* TODO: why does this method parse the strust from back to front (resetting after each field) instead of from front to back like the rest of the parsing code? /
/
TODO: why does this method exist when the same struct is parsed in xrdp_sec_process_mcs_data_CS_CORE and there does not seem to be any dependencies preventing a call to that function. /
/
TODO: this is a brittle function that assumes field offsets in the stream instead of parsing the variable length fields of [ITU T.124] ConferenceCreateRequest */

I've finally gotten round to looking into this, and, like @aquesnel, I've reached the conclusion this function is not necessary (and is, in fact, confusing). The function parses data within the TS_UD_CS_CORE struct which could just as easily be parsed when xrdp_sec_process_mcs_data_CS_CORE() is called earlier.

This ASCII art show the call sequence of these functions:-

xrdp_sec_incoming()
    |
    +-xrdp_mcs_incoming()
    |   |
    |   +-xrdp_sec_process_mcs_data()
    |       |
    |       +-xrdp_sec_process_mcs_data_CS_CORE()
    . . .
    |
    +-xrdp_sec_in_mcs_data()

Currently the contents of the MSC Connect Initial PDU are stored within the client_mcs_data member of the xrdp_sec struct. This stream is parsed once by xrdp_sec_process_mcs_data() and then separately by xrdp_sec_in_mcs_data() (as the diagram above shows). There is no reason not to perform the parse in a single pass through the stream.

This commit folds the functionality in xrdp_sec_in_mcs_data() into xrdp_sec_process_mcs_data_CS_CORE() and removes xrdp_sec_in_mcs_data()

Looking back on the history of these functions, xrdp_sec_in_mcs_data() is the older, and was added in August 2005 by 5e69f15. xrdp_sec_process_mcs_data_CS_CORE() was originally called xrdp_rdp_parse_client_mcs_data_CS_CORE() and was added in Feb 2014 by 1d9c773 to a different file. The two functions have never been combined.

THe function xrdp_sec_in_mcs_data() parses data within the
TS_UD_CS_CORE struct which could just as easily be parsed
when xrdp_sec_process_mcs_data_CS_CORE() is called.

Currently the contents of the MSC Connect Initial PDU are stored within
the client_mcs_data member of the xrdp_sec struct. This stream is parsed
once by xrdp_sec_process_mcs_data() and then separately by
xrdp_sec_in_mcs_data(). There is no reason not to perform the parse in
a single pass through the stream.

This commit folds the functionality in xrdp_sec_in_mcs_data() into
xrdp_sec_process_mcs_data_CS_CORE() and removes xrdp_sec_in_mcs_data()
@matt335672 matt335672 merged commit 493e01e into neutrinolabs:devel Oct 14, 2024
14 checks passed
@matt335672 matt335672 deleted the remove_xrdp_sec_in_mcs_data branch October 14, 2024 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants