Re: RFR: 8274929: Crash while reading specific clipboard content [v2]
On Thu, 11 Nov 2021 10:46:13 GMT, Pankaj Bansal wrote: >> Kevin Rushforth has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Update check to test that bufferSize is exactly the right size > > tests/system/src/test/java/test/javafx/scene/input/ClipboardTest.java line 47: > >> 45: import sun.awt.datatransfer.SunClipboard; >> 46: >> 47: import static org.junit.Assert.*; > > I am not sure about how strict we are about using wildcard imports in tests > in JavaFX. You can change this or keep it as it is, depending upon the answer > to the first statement. In general wildcard imports are discouraged; static imports (e.g., `Assert.*`) are an exception used in many of our tests. - PR: https://git.openjdk.java.net/jfx/pull/662
Re: RFR: 8274929: Crash while reading specific clipboard content [v2]
On Wed, 10 Nov 2021 12:46:08 GMT, Kevin Rushforth wrote: >> This bug is caused by not sanity checking the data returned by a call to the >> Windows Clipboard `IDataObject::GetData` method. When requesting a file >> descriptor with a format of either `CFSTR_FILEDESCRIPTORA` or >> `CFSTR_FILEDESCRIPTORW`, which returns a list of file names, the first word >> of the returned data buffer is supposed to be the number of items that >> follow. Applications can put data on the clipboard in such a way that it >> will respond to a request to return the list of files from the clipboard >> with data that isn't formatted correctly, so we can't assume that the first >> word is a valid count. >> >> The fix is to check the returned buffer size against the item count. I added >> a regression test that fails before and passes after the fix. > > Kevin Rushforth has updated the pull request incrementally with one > additional commit since the last revision: > > Update check to test that bufferSize is exactly the right size tests/system/src/test/java/test/javafx/scene/input/ClipboardTest.java line 47: > 45: import sun.awt.datatransfer.SunClipboard; > 46: > 47: import static org.junit.Assert.*; I am not sure about how strict we are about using wildcard imports in tests in JavaFX. You can change this or keep it as it is, depending upon the answer to the first statement. - PR: https://git.openjdk.java.net/jfx/pull/662
Re: RFR: 8274929: Crash while reading specific clipboard content [v2]
On Wed, 10 Nov 2021 12:46:08 GMT, Kevin Rushforth wrote: >> This bug is caused by not sanity checking the data returned by a call to the >> Windows Clipboard `IDataObject::GetData` method. When requesting a file >> descriptor with a format of either `CFSTR_FILEDESCRIPTORA` or >> `CFSTR_FILEDESCRIPTORW`, which returns a list of file names, the first word >> of the returned data buffer is supposed to be the number of items that >> follow. Applications can put data on the clipboard in such a way that it >> will respond to a request to return the list of files from the clipboard >> with data that isn't formatted correctly, so we can't assume that the first >> word is a valid count. >> >> The fix is to check the returned buffer size against the item count. I added >> a regression test that fails before and passes after the fix. > > Kevin Rushforth has updated the pull request incrementally with one > additional commit since the last revision: > > Update check to test that bufferSize is exactly the right size The changes look fine to me - Marked as reviewed by pbansal (Committer). PR: https://git.openjdk.java.net/jfx/pull/662
Re: RFR: 8274929: Crash while reading specific clipboard content [v2]
On Wed, 10 Nov 2021 12:46:08 GMT, Kevin Rushforth wrote: >> This bug is caused by not sanity checking the data returned by a call to the >> Windows Clipboard `IDataObject::GetData` method. When requesting a file >> descriptor with a format of either `CFSTR_FILEDESCRIPTORA` or >> `CFSTR_FILEDESCRIPTORW`, which returns a list of file names, the first word >> of the returned data buffer is supposed to be the number of items that >> follow. Applications can put data on the clipboard in such a way that it >> will respond to a request to return the list of files from the clipboard >> with data that isn't formatted correctly, so we can't assume that the first >> word is a valid count. >> >> The fix is to check the returned buffer size against the item count. I added >> a regression test that fails before and passes after the fix. > > Kevin Rushforth has updated the pull request incrementally with one > additional commit since the last revision: > > Update check to test that bufferSize is exactly the right size Marked as reviewed by arapte (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/662
Re: RFR: 8274929: Crash while reading specific clipboard content [v2]
On Wed, 10 Nov 2021 12:46:08 GMT, Kevin Rushforth wrote: >> This bug is caused by not sanity checking the data returned by a call to the >> Windows Clipboard `IDataObject::GetData` method. When requesting a file >> descriptor with a format of either `CFSTR_FILEDESCRIPTORA` or >> `CFSTR_FILEDESCRIPTORW`, which returns a list of file names, the first word >> of the returned data buffer is supposed to be the number of items that >> follow. Applications can put data on the clipboard in such a way that it >> will respond to a request to return the list of files from the clipboard >> with data that isn't formatted correctly, so we can't assume that the first >> word is a valid count. >> >> The fix is to check the returned buffer size against the item count. I added >> a regression test that fails before and passes after the fix. > > Kevin Rushforth has updated the pull request incrementally with one > additional commit since the last revision: > > Update check to test that bufferSize is exactly the right size Marked as reviewed by mstrauss (Author). - PR: https://git.openjdk.java.net/jfx/pull/662
Re: RFR: 8274929: Crash while reading specific clipboard content [v2]
> This bug is caused by not sanity checking the data returned by a call to the > Windows Clipboard `IDataObject::GetData` method. When requesting a file > descriptor with a format of either `CFSTR_FILEDESCRIPTORA` or > `CFSTR_FILEDESCRIPTORW`, which returns a list of file names, the first word > of the returned data buffer is supposed to be the number of items that > follow. Applications can put data on the clipboard in such a way that it will > respond to a request to return the list of files from the clipboard with data > that isn't formatted correctly, so we can't assume that the first word is a > valid count. > > The fix is to check the returned buffer size against the item count. I added > a regression test that fails before and passes after the fix. Kevin Rushforth has updated the pull request incrementally with one additional commit since the last revision: Update check to test that bufferSize is exactly the right size - Changes: - all: https://git.openjdk.java.net/jfx/pull/662/files - new: https://git.openjdk.java.net/jfx/pull/662/files/abcd51c4..ff5f9c1a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=662=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=662=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/662.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/662/head:pull/662 PR: https://git.openjdk.java.net/jfx/pull/662
Re: RFR: 8274929: Crash while reading specific clipboard content [v2]
On Wed, 10 Nov 2021 01:06:40 GMT, Kevin Rushforth wrote: >> Then shouldn't we also not trust the data if `bufferSize` is larger than it >> needs to be? The documentation of `FILEGROUPDESCRIPTORA/W` says that >> `cItems` should correspond exactly to the numer of items in the array that >> follows. > >> Then shouldn't we also not trust the data if bufferSize is larger than it >> needs to be? > > Yes, that's a good point. > > In order to avoid integer overflow, I'll probably leave the two existing > tests, and add a third (or else do the test using a `jlong`). Something like > this should work: > > > jsize bufferSize = me.size() - sizeof(UINT); > if ((pdata->cItems > 0) && > (bufferSize / pdata->cItems >= itemSize) && > (bufferSize == pdata->cItems * itemSize)) > > > I'll update this tomorrow. Fixed. I decided it was cleaner to use `jlong` and just have the equality check. - PR: https://git.openjdk.java.net/jfx/pull/662
