Windows Desktop Directory Sharing by ibeckermayer · Pull Request #13630 · gravitational/teleport

ibeckermayer · 2022-06-17T18:47:36Z

No description provided.

github-actions · 2022-07-06T16:02:53Z

@ibeckermayer - this PR is large and will require admin approval to merge. Consider breaking it up into a series smaller changes.

github-actions · 2022-07-07T14:50:43Z

@ibeckermayer - this PR is large and will require admin approval to merge. Consider breaking it up into a series smaller changes.

github-actions · 2022-07-07T14:54:15Z

@ibeckermayer - this PR is large and will require admin approval to merge. Consider breaking it up into a series smaller changes.

github-actions · 2022-07-08T23:13:49Z

@ibeckermayer - this PR is large and will require admin approval to merge. Consider breaking it up into a series smaller changes.

github-actions · 2022-07-08T23:33:49Z

@ibeckermayer - this PR is large and will require admin approval to merge. Consider breaking it up into a series smaller changes.

github-actions · 2022-07-11T13:15:58Z

@ibeckermayer - this PR is large and will require admin approval to merge. Consider breaking it up into a series smaller changes.

github-actions · 2022-07-26T21:26:03Z

@ibeckermayer - this PR is large and will require admin approval to merge. Consider breaking it up into a series smaller changes.

github-actions · 2022-07-26T21:36:33Z

@ibeckermayer - this PR is large and will require admin approval to merge. Consider breaking it up into a series smaller changes.

github-actions · 2022-07-26T21:36:48Z

@ibeckermayer - this PR is large and will require admin approval to merge. Consider breaking it up into a series smaller changes.

github-actions · 2022-07-27T17:13:58Z

@ibeckermayer - this PR is large and will require admin approval to merge. Consider breaking it up into a series smaller changes.

github-actions · 2022-07-28T18:50:37Z

@ibeckermayer - this PR is large and will require admin approval to merge. Consider breaking it up into a series smaller changes.

github-actions · 2022-07-29T15:15:38Z

@ibeckermayer - this PR is large and will require admin approval to merge. Consider breaking it up into a series smaller changes.

LKozlowski · 2022-08-03T08:40:47Z

+    // In other words, all pointer data that needs to persist after this function returns MUST
+    // be copied into Rust-owned memory.
+
+    let res = SharedDirectoryInfoResponse::from(res);


At first, the SharedDirectoryInfoResponse was created in place where it was used, inside the call to handle_tdp_sd_info_response, but now it has been moved into top of the function. There's nothing wrong with defining it as a standalone variable, but I'm just wondering why you've moved it here? There are couple of old functions in which we follow the old pattern, so maybe we should unify them?

The purpose of these changes are to always ensure that we adhere to the following memory safety protocol, all the way up here at the "C" boundary:

teleport/lib/srv/desktop/rdp/rdpclient/src/lib.rs

Lines 838 to 842 in 9f10dca

// # Safety

//

// This function MUST NOT hang on to any of the pointers passed in to it after it returns.

// In other words, all pointer data that needs to persist after this function returns MUST

// be copied into Rust-owned memory.

(Which we do in SharedDirectoryInfoResponse::from)

This way, we don't need to worry about any tricky memory bugs further down the call stack. It's all taken care of right here at the boundary.

I'm not sure which old functions you're referring to, can you point them out?

Thanks for the response! I should've included the other function I had in my mind in my previous response. Like here: I did it the other way:

teleport/lib/srv/desktop/rdp/rdpclient/src/lib.rs

Lines 984 to 1004 in 9f10dca

#[no_mangle]

pub unsafe extern "C" fn handle_tdp_sd_read_response(

client_ptr: *mut Client,

res: CGOSharedDirectoryReadResponse,

) -> CGOErrCode {

let client = match Client::from_ptr(client_ptr) {

Ok(client) => client,

Err(cgo_error) => {

return cgo_error;

}

};

let mut rdp_client = client.rdp_client.lock().unwrap();

match rdp_client.handle_tdp_sd_read_response(SharedDirectoryReadResponse::from(res)) {

Ok(()) => CGOErrCode::ErrCodeSuccess,

Err(e) => {

error!("failed to handle Shared Directory Read Response: {:?}", e);

CGOErrCode::ErrCodeFailure

}

}

}

Ah I see, good eye. I must have not had those functions merged yet whenever I went back and revised all of these functions to use this pattern. The code and memory all works fine, but I'll add the safety comments and rearrange the order in an upcoming PR just for the sake of consistency.

Co-authored-by: Łukasz Kozłowski <admin@lkozlowski.com>

github-actions · 2022-08-04T21:51:00Z

@ibeckermayer See the table below for backport results.

Branch	Result
branch/v10	Failed
branch/v9	Failed

* `IRP_MJ_CREATE` (#12665) * `IRP_MJ_QUERY_INFORMATION` (#12717) * `IRP_MJ_CLOSE` (#12729) * Refactor rdpdr client (#12750) * Adding logic for `FILE_SUPERSEDE` (#12829) * Improve `process_irp_create` (#12830) * adds return statements that got lost in a merge * `IRP_MJ_DIRECTORY_CONTROL` (#12870) * `FileFullDirectoryInformation` (#12908) * Improve `ClientDriveQueryDirectoryResponse.encode()` (#12912) * `IRP_MJ_QUERY_VOLUME_INFORMATION` (#13071) * Fix Shared Directory Request handling when feature is disabled (#13439) * IRP_MJ_READ, IRP_MJ_WRITE, and IRP_MJ_SET_INFORMATION (#13995) * Adds constants for sizing calculations (#14051) Co-authored-by: Łukasz Kozłowski <lukasz.kozlowski@goteleport.com> Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com>

…ring) (#15770) * Windows Desktop Directory Sharing (#13630) * `IRP_MJ_CREATE` (#12665) * `IRP_MJ_QUERY_INFORMATION` (#12717) * `IRP_MJ_CLOSE` (#12729) * Refactor rdpdr client (#12750) * Adding logic for `FILE_SUPERSEDE` (#12829) * Improve `process_irp_create` (#12830) * adds return statements that got lost in a merge * `IRP_MJ_DIRECTORY_CONTROL` (#12870) * `FileFullDirectoryInformation` (#12908) * Improve `ClientDriveQueryDirectoryResponse.encode()` (#12912) * `IRP_MJ_QUERY_VOLUME_INFORMATION` (#13071) * Fix Shared Directory Request handling when feature is disabled (#13439) * IRP_MJ_READ, IRP_MJ_WRITE, and IRP_MJ_SET_INFORMATION (#13995) * Adds constants for sizing calculations (#14051) Co-authored-by: Łukasz Kozłowski <lukasz.kozlowski@goteleport.com> Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com> * `UnixPath` and `WindowsPath` (#14267) * `SharedDirectoryMoveRequest` and `SharedDirectoryMoveResponse` (#14959) * `SharedDirectoryCreateResponse` update (#15289) * Fix `process_irp_set_information` (#15364) * Sanitize Rust Debug Logs (#15743) * updates rdp-rs ref to include licensing changes * Updates rdp-rs ref and fixes Cargo Co-authored-by: Łukasz Kozłowski <lukasz.kozlowski@goteleport.com> Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com>

ibeckermayer added backport/branch/v10 desktop-access rdp labels Jun 17, 2022

ibeckermayer requested review from LKozlowski, xacrimon and zmb3 July 6, 2022 15:32

ibeckermayer force-pushed the windows-desktop-directory-sharing branch 2 times, most recently from 725f742 to 2cd91e5 Compare July 6, 2022 16:02

ibeckermayer marked this pull request as ready for review July 6, 2022 16:02

github-actions Bot requested review from gabrielcorado and rosstimothy July 6, 2022 16:02

ibeckermayer added the backport/branch/v9 label Jul 6, 2022

ibeckermayer removed request for gabrielcorado and rosstimothy July 6, 2022 17:04

Isaiah Becker-Mayer added 10 commits July 13, 2022 17:56

IRP_MJ_CREATE (#12665)

af886a9

IRP_MJ_QUERY_INFORMATION (#12717)

cef7d8e

IRP_MJ_CLOSE (#12729)

2732c2c

Refactor rdpdr client (#12750)

e38a14a

Adding logic for FILE_SUPERSEDE (#12829)

17ba911

Improve process_irp_create (#12830)

60e5c5c

adds return statements that got lost in a merge

b598b8e

IRP_MJ_DIRECTORY_CONTROL (#12870)

c09ecb5

FileFullDirectoryInformation (#12908)

c717131

Improve ClientDriveQueryDirectoryResponse.encode() (#12912)

4b74c82

Isaiah Becker-Mayer added 2 commits July 26, 2022 17:35

Merge branch 'master' into windows-desktop-directory-sharing

474a3f6

adds documentation

bfdc8aa

ibeckermayer enabled auto-merge (squash) July 26, 2022 21:48

Merge branch 'master' into windows-desktop-directory-sharing

f3aad5e

ibeckermayer requested a review from zmb3 July 27, 2022 17:15

Merge branch 'master' into windows-desktop-directory-sharing

2dbea5c

zmb3 approved these changes Jul 29, 2022

View reviewed changes

Merge branch 'master' into windows-desktop-directory-sharing

10596fa

LKozlowski approved these changes Aug 3, 2022

View reviewed changes

Merge branch 'master' into windows-desktop-directory-sharing

9f10dca

github-actions Bot removed the request for review from xacrimon August 3, 2022 18:47

Updates faulty error messages

1542ba3

Co-authored-by: Łukasz Kozłowski <admin@lkozlowski.com>

ibeckermayer mentioned this pull request Aug 3, 2022

UnixPath and WindowsPath #14267

Merged

Isaiah Becker-Mayer added 3 commits August 4, 2022 14:30

Merge branch 'master' into windows-desktop-directory-sharing

f0d9a7d

Merge branch 'master' into windows-desktop-directory-sharing

e59de72

Merge branch 'master' into windows-desktop-directory-sharing

e2d8dde

ibeckermayer merged commit 361ea8e into master Aug 4, 2022

ibeckermayer mentioned this pull request Aug 23, 2022

[v10] backports for 13630 14267 14959 15289 15364 15789 15743 (directory sharing) #15767

Merged

ibeckermayer mentioned this pull request Aug 23, 2022

[v9] backports for 13630 14267 14959 15289 15364 15743 (directory sharing) #15770

Merged

zmb3 deleted the windows-desktop-directory-sharing branch September 1, 2022 21:15

	// # Safety
	//
	// This function MUST NOT hang on to any of the pointers passed in to it after it returns.
	// In other words, all pointer data that needs to persist after this function returns MUST
	// be copied into Rust-owned memory.

	#[no_mangle]
	pub unsafe extern "C" fn handle_tdp_sd_read_response(
	client_ptr: *mut Client,
	res: CGOSharedDirectoryReadResponse,
	) -> CGOErrCode {
	let client = match Client::from_ptr(client_ptr) {
	Ok(client) => client,
	Err(cgo_error) => {
	return cgo_error;
	}
	};

	let mut rdp_client = client.rdp_client.lock().unwrap();
	match rdp_client.handle_tdp_sd_read_response(SharedDirectoryReadResponse::from(res)) {
	Ok(()) => CGOErrCode::ErrCodeSuccess,
	Err(e) => {
	error!("failed to handle Shared Directory Read Response: {:?}", e);
	CGOErrCode::ErrCodeFailure
	}
	}
	}

Conversation

ibeckermayer commented Jun 17, 2022

Uh oh!

github-actions Bot commented Jul 6, 2022

Uh oh!

github-actions Bot commented Jul 7, 2022

Uh oh!

github-actions Bot commented Jul 7, 2022

Uh oh!

github-actions Bot commented Jul 8, 2022

Uh oh!

github-actions Bot commented Jul 8, 2022

Uh oh!

github-actions Bot commented Jul 11, 2022

Uh oh!

github-actions Bot commented Jul 26, 2022

Uh oh!

github-actions Bot commented Jul 26, 2022

Uh oh!

github-actions Bot commented Jul 26, 2022

Uh oh!

github-actions Bot commented Jul 27, 2022

Uh oh!

github-actions Bot commented Jul 28, 2022

Uh oh!

github-actions Bot commented Jul 29, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LKozlowski Aug 3, 2022

Choose a reason for hiding this comment

Uh oh!

ibeckermayer Aug 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LKozlowski Aug 5, 2022

Choose a reason for hiding this comment

Uh oh!

ibeckermayer Aug 5, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Aug 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ibeckermayer Aug 3, 2022 •

edited

Loading