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

SetInformation and UnsupportedMinorFunctionRequest #312

Merged
merged 4 commits into from
Nov 29, 2023

Conversation

ibeckermayer
Copy link
Contributor

SetInformation is straightforward.

UnsupportedMinorFunctionRequest because I realized that the server is liable to send us back an occasional NotifyChangeDirectory or None which we want to handle. This allows us to handle these cases gracefully (but without writing all the decode/encode logic), and kicks the handling up the stack to the RdpdrBackend implementer (Teleport in our case). Teleport simply mimics what FreeRDP currently does with these.

Isaiah Becker-Mayer added 2 commits November 28, 2023 12:27
- Extend ReadCursor with methods to read and try reading i64 values in both little and big endian formats.
- Update RdpdrPdu to handle ClientDriveSetInformationResponse, reflecting the addition of ServerDriveSetInformationRequest handling in the RDPDR (Remote Desktop Protocol: Device Redirection) protocol.
- Implement decoding and encoding of various FileInformationClass types such as FileEndOfFileInformation, FileDispositionInformation, FileRenameInformation, and FileAllocationInformation.
- Refactor the existing code to improve the handling of different FileInformationClassLevels and FileSystemInformationClassLevels.
… functions in ServerDriveIoRequest

- Add a 'discard' method to ReadCursor in ironrdp-pdu, which advances the cursor by the remaining length of the buffer. This method aids in efficiently skipping over unused data.
- Modify ServerDriveIoRequest in ironrdp-rdpdr to handle unsupported minor functions. Instead of returning an error for an unsupported minor function, the new approach discards the remaining data and returns a special 'UnsupportedMinorFunctionRequest' type. This change allows for more flexible handling of unsupported functions by upstream logic
@ibeckermayer ibeckermayer requested a review from CBenoit November 28, 2023 21:43
Copy link

github-actions bot commented Nov 28, 2023

Coverage Report 🤖 ⚙️

Past:
Total lines: 26493
Covered lines: 16382 (61.84%)

New:
Total lines: 26686
Covered lines: 16377 (61.37%)

Diff: -0.47%

[this comment will be updated automatically]

@CBenoit
Copy link
Member

CBenoit commented Nov 28, 2023

SetInformation is straightforward.

👍

UnsupportedMinorFunctionRequest because I realized that the server is liable to send us back an occasional NotifyChangeDirectory or None which we want to handle. This allows us to handle these cases gracefully (but without writing all the decode/encode logic), and kicks the handling up the stack to the RdpdrBackend implementer (Teleport in our case). Teleport simply mimics what FreeRDP currently does with these.

I expressed my concern about the new variant in the comment above, but I’m okay with this if this is really simpler for your use case.
I just want to make sure of one thing. FreeRDP is calling Discard on the IRP handle. The purpose of this is just to free the memory.
So it seems to me that we don’t need this step in Rust, and it would in fact map to us skipping this line:

self.backend.handle_drive_io_request(req)?;

But I don’t know how it’s actually handled in your backend. Maybe this callback is really required for other kind of manual clean up. (In this case I would suggest documenting a bit more the trait, just to make sure we don’t accidentally break your assumption later.)

EDIT: Upon reading further, I understand that indeed you likely need this callback for closing the underlying resource.
I suggest adding a new method to the trait dedicated to this. For instance: fn handle_drive_io_failure(req: DeviceIoRequest). Just another avenue.

- Remove the 'discard' method from ReadCursor in ironrdp-pdu.
- Refactor MinorFunction in DeviceIoRequest from an enum to a struct with constants, simplifying its representation and usage.
- Add handling for ServerDriveNotifyChangeDirectoryRequest in ServerDriveIoRequest, supporting the specific case when MinorFunction is set to IRP_MN_NOTIFY_CHANGE_DIRECTORY.
- Update decoding logic in DeviceIoRequest to directly convert the minor function value from u32 to MinorFunction without conditional checks, simplifying the code.
- Remove handling of unsupported minor functions, as the new approach directly supports all minor function values.
@ibeckermayer
Copy link
Contributor Author

@CBenoit it wasn't too much effort to actually just handle the decoding per usual, see now with 5b0a29e

@ibeckermayer ibeckermayer requested a review from CBenoit November 28, 2023 23:28
Copy link
Member

@CBenoit CBenoit left a comment

Choose a reason for hiding this comment

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

Nice. This looks good to me. Approving and enabling auto-merge.
For some reason CI is failing with an error at the fuzzing step, but I don’t see how your code could be the reason for that. 🤔

@CBenoit CBenoit enabled auto-merge (squash) November 28, 2023 23:37
@CBenoit
Copy link
Member

CBenoit commented Nov 28, 2023

On, in fact there are errors related to your changes.
image
Try to run cargo xtask ci locally and see if you get the errors on your side too. If not, I wonder if this is a caching problem. I can try to delete the cache if everything is fine on your side.

@ibeckermayer
Copy link
Contributor Author

ibeckermayer commented Nov 29, 2023

@CBenoit it's passing for me locally

@CBenoit
Copy link
Member

CBenoit commented Nov 29, 2023

it's passing for me locally

I tried to clear the cache and run again, unfortunately the problem is not gone.
image

Also, I have the same output when I run locally:

$ cargo clean
$ cargo xtask ci

image

Strange 🤷

EDIT: see comment below

Update the Cargo.lock files to reflect the changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants