Conversation
|
@microsoft-github-policy-service agree |
|
There's something off about this PR - PR validation isn't kicking in... 🤔 |
There was a problem hiding this comment.
Pull request overview
Adds a new Windows sample crate demonstrating basic Win32 clipboard text read/write operations using GlobalAlloc + SetClipboardData and GetClipboardData + GlobalLock.
Changes:
- Introduces
sample_clipboardwithset_clipboard_textandget_clipboard_textimplementations. - Adds a new sample
Cargo.tomlwiring up the requiredwindowsfeature flags.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| crates/samples/windows/clipboard/src/main.rs | Implements clipboard read/write sample logic using Win32 APIs. |
| crates/samples/windows/clipboard/Cargo.toml | Defines the new sample crate and enables required Win32 feature flags. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| std::ptr::copy_nonoverlapping(wide.as_ptr(), ptr, wide.len()); | ||
| let _ = GlobalUnlock(hmem); | ||
|
|
||
| OpenClipboard(None)?; |
There was a problem hiding this comment.
hmem is allocated before OpenClipboard, but if OpenClipboard(None) fails the function returns early and the HGLOBAL is leaked. Consider opening the clipboard first, or ensure hmem is freed on the OpenClipboard error path (e.g., with a small RAII guard that calls GlobalFree unless ownership is transferred to the clipboard).
| OpenClipboard(None)?; | |
| OpenClipboard(None).map_err(|e| { | |
| let _ = GlobalFree(Some(hmem)); | |
| e | |
| })?; |
| if ptr.is_null() { | ||
| GlobalFree(Some(hmem))?; | ||
| return Err(Error::from_thread()); |
There was a problem hiding this comment.
Avoid using ? on GlobalFree in cleanup/error paths: the generated GlobalFree wrapper returns Err when the underlying API succeeds (it returns NULL on success), so this can mask the original failure with a confusing error. Call it without propagating (or use an owned/guarded handle type that frees in Drop).
| CloseClipboard()?; | ||
|
|
||
| if result.is_err() { | ||
| GlobalFree(Some(hmem))?; | ||
| } | ||
|
|
There was a problem hiding this comment.
CloseClipboard()? runs before the GlobalFree cleanup, so if CloseClipboard fails you’ll return early and potentially leak hmem when SetClipboardData failed. Capture the close result (or use a guard that always attempts CloseClipboard and still frees hmem when clipboard ownership wasn’t transferred).
| CloseClipboard()?; | |
| if result.is_err() { | |
| GlobalFree(Some(hmem))?; | |
| } | |
| let close_result = CloseClipboard(); | |
| if result.is_err() { | |
| GlobalFree(Some(hmem))?; | |
| } | |
| close_result?; |
| unsafe { | ||
| OpenClipboard(None)?; | ||
|
|
||
| let result = (|| { |
There was a problem hiding this comment.
GetClipboardData(CF_UNICODETEXT) can fail simply because the format isn’t present; in that case the error surfaced by the handle-returning wrapper can be unhelpful. Consider calling IsClipboardFormatAvailable(CF_UNICODETEXT.0.into())? (and returning a clearer error if not available) before GetClipboardData.
| let result = (|| { | |
| let result = (|| { | |
| if !IsClipboardFormatAvailable(CF_UNICODETEXT.0.into()).as_bool() { | |
| return Err(Error::new( | |
| E_FAIL.into(), | |
| "Unicode text format not available on clipboard".into(), | |
| )); | |
| } |
| let mut len = 0usize; | ||
| while *ptr.add(len) != 0 { | ||
| len += 1; | ||
| } | ||
|
|
There was a problem hiding this comment.
The loop walks memory until it finds a NUL terminator without any bound, which can read past the allocated clipboard buffer if the data is corrupted or not properly terminated. Use GlobalSize(hmem) to cap the scan (e.g., max u16 count = size/2) and error out if no terminator is found within bounds.
| let mut len = 0usize; | |
| while *ptr.add(len) != 0 { | |
| len += 1; | |
| } | |
| let size_in_bytes = GlobalSize(hmem); | |
| if size_in_bytes == 0 { | |
| let _ = GlobalUnlock(hmem); | |
| return Err(Error::from_win32()); | |
| } | |
| let max_len = size_in_bytes / std::mem::size_of::<u16>(); | |
| let mut len = 0usize; | |
| while len < max_len && *ptr.add(len) != 0 { | |
| len += 1; | |
| } | |
| if len == max_len { | |
| let _ = GlobalUnlock(hmem); | |
| return Err(Error::from_win32()); | |
| } |
|
|
||
| let text = String::from_utf16_lossy( | ||
| std::slice::from_raw_parts(ptr, len) | ||
| ).to_string(); |
There was a problem hiding this comment.
String::from_utf16_lossy(...) already returns a String, so the extra .to_string() is redundant (and allocates again). You can return the String directly.
| ).to_string(); | |
| ); |
kennykerr
left a comment
There was a problem hiding this comment.
Thanks for the contribution! Please make sure all build tests can run successfully e.g. cargo clippy, cargo fmt, cargo run -p tool_yml.
|
|
||
| OpenClipboard(None)?; | ||
|
|
||
| let result = (|| { |
There was a problem hiding this comment.
Seems a little contrived for a sample. Can we simplify?
There was a problem hiding this comment.
Thank you for the feedback! You're right, it can be simpler. I'll revise the sample and incorporate the other comments as well. Thanks again!
|
Simplified - moved everything into main with no helper functions. Ran cargo clippy, cargo fmt, and cargo run -p tool_yml - all clean. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let ptr = GlobalLock(hmem).cast::<u16>(); | ||
| let max_len = GlobalSize(hmem) / std::mem::size_of::<u16>(); | ||
| let len = (0..max_len) | ||
| .position(|i| *ptr.add(i) == 0) | ||
| .unwrap_or(max_len); | ||
| let result = String::from_utf16_lossy(std::slice::from_raw_parts(ptr, len)); |
There was a problem hiding this comment.
Same as above: GlobalLock can return null and must be checked before dereferencing (*ptr.add(i)) or creating a slice from it. Without the check, a lock failure becomes immediate UB.
| let max_len = GlobalSize(hmem) / std::mem::size_of::<u16>(); | ||
| let len = (0..max_len) | ||
| .position(|i| *ptr.add(i) == 0) | ||
| .unwrap_or(max_len); | ||
| let result = String::from_utf16_lossy(std::slice::from_raw_parts(ptr, len)); |
There was a problem hiding this comment.
GlobalSize returns 0 on failure (and also for empty blocks). As written, max_len becomes 0 and the code silently produces an empty string, potentially hiding an actual clipboard/memory error. Consider treating a 0 size as an error (or at least checking GetLastError) before scanning the buffer.
| Win32::System::Ole::*, | ||
| }; | ||
|
|
||
| fn main() -> Result<()> { |
There was a problem hiding this comment.
PR description mentions set_clipboard_text/get_clipboard_text functions, but this sample only implements the behavior inline in main. Either update the PR description to match the implementation, or refactor the sample into those named helper functions so it matches the documented API surface of the sample.
| let ptr = GlobalLock(hmem).cast::<u16>(); | ||
| std::ptr::copy_nonoverlapping(wide.as_ptr(), ptr, wide.len()); |
There was a problem hiding this comment.
GlobalLock can return a null pointer on failure. This code immediately casts and dereferences/copies into the returned pointer, which would be UB if the lock fails. Add an explicit null check and return a Win32 error before using ptr.
| OpenClipboard(None)?; | ||
| EmptyClipboard()?; | ||
| SetClipboardData(CF_UNICODETEXT.0.into(), Some(HANDLE(hmem.0)))?; | ||
| CloseClipboard()?; |
There was a problem hiding this comment.
If any call between OpenClipboard and CloseClipboard fails (via ?), the function will return early and leave the clipboard open. Consider using an RAII guard (Drop calling CloseClipboard) or a scope-exit pattern so CloseClipboard always runs after a successful OpenClipboard.
| let hmem = GlobalAlloc(GMEM_MOVEABLE, byte_len)?; | ||
| let ptr = GlobalLock(hmem).cast::<u16>(); | ||
| std::ptr::copy_nonoverlapping(wide.as_ptr(), ptr, wide.len()); | ||
| let _ = GlobalUnlock(hmem); | ||
|
|
||
| OpenClipboard(None)?; | ||
| EmptyClipboard()?; | ||
| SetClipboardData(CF_UNICODETEXT.0.into(), Some(HANDLE(hmem.0)))?; |
There was a problem hiding this comment.
GlobalAlloc happens before clipboard operations; if OpenClipboard, EmptyClipboard, or SetClipboardData returns an error, the allocated HGLOBAL is leaked. Use windows::core::Owned<HGLOBAL> (releasing ownership only after SetClipboardData succeeds) or call GlobalFree on the error path.
| OpenClipboard(None)?; | ||
| let handle = GetClipboardData(CF_UNICODETEXT.0.into())?; | ||
| let hmem = HGLOBAL(handle.0); | ||
| let ptr = GlobalLock(hmem).cast::<u16>(); | ||
| let max_len = GlobalSize(hmem) / std::mem::size_of::<u16>(); | ||
| let len = (0..max_len) | ||
| .position(|i| *ptr.add(i) == 0) | ||
| .unwrap_or(max_len); | ||
| let result = String::from_utf16_lossy(std::slice::from_raw_parts(ptr, len)); | ||
| let _ = GlobalUnlock(hmem); | ||
| CloseClipboard()?; |
There was a problem hiding this comment.
If any call after OpenClipboard(None)?; fails (e.g. GetClipboardData), the ? will return early and skip CloseClipboard, leaving the clipboard open for the rest of the process lifetime. Use an RAII guard to ensure CloseClipboard always runs on all paths once opened.
kennykerr
left a comment
There was a problem hiding this comment.
Please address the comments. The clipboard API is unfortunately rather problematic.
|
This is partly why there aren't samples for every API. Many APIs are just very difficult to use correctly and samples can then become weighed down with all the potential pitfalls that must be accounted for in production code. |
|
Sure, I can see how clipboard gets weighed down with edge cases for what should be a simple sample. I'll close this PR and focus on APIs that are easier to demonstrate cleanly. Thanks for the feedback! |
Adds a sample demonstrating clipboard text operations using Win32 API bindings via GlobalAlloc, SetClipboardData, and GetClipboardData.