Re: RFR: 8300038: Make new version of JNU_GetStringPlatformChars which checks for null characters [v4]
On Thu, 25 May 2023 14:21:34 GMT, Michael McMahon wrote: > I'll integrate this today unless there are further comments. Thanks for addressing my comments. - PR Comment: https://git.openjdk.org/jdk/pull/14083#issuecomment-1563002770
Re: RFR: 8300038: Make new version of JNU_GetStringPlatformChars which checks for null characters [v4]
On Wed, 24 May 2023 20:46:23 GMT, Michael McMahon wrote: >> This PR creates a new version of the JNI utility function >> JNU_GetStringPlatformChars called JNU_GetStringPlatformCharsStrict, which >> performs additional validation of the returned string, namely that it does >> not contain any embedded NULL characters. If any such characters are found >> the function returns NULL with an IAE pending. The change also switches >> usage in the networking native code to use the new function. >> >> This cautious approach was taken rather than changing the behavior of the >> existing function as each native code area needs to review the effect of >> making the switch. Otherwise, surprising behavior changes might occur (eg >> undocumented IAE being thrown to user code instead of some other exception). > > Michael McMahon has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 12 commits: > > - Merge branch 'master' into nullStrings > - error message and test update > - Merge branch 'master' into nullStrings > - test comment update > - test update > - Merge branch 'master' into nullStrings > - exception message update > - test update > - remve whitespace > - update > - ... and 2 more: https://git.openjdk.org/jdk/compare/207fbcb0...35df1a67 I'll integrate this today unless there are further comments. - PR Comment: https://git.openjdk.org/jdk/pull/14083#issuecomment-1562998991
Re: RFR: 8300038: Make new version of JNU_GetStringPlatformChars which checks for null characters [v3]
On Wed, 24 May 2023 14:30:01 GMT, Alan Bateman wrote: > I skimmed through the changes and mostly look okay. There's a few usage of > "NULL" in exception messages that I assume should be "NUL character". > Probably need to bump the copyright date on several files. You might want to > find a better name for the test as NullChar isn't too descriptive, the test > is really doing a lookup of a hostname that contains a NUL character. I > assume there will be follow up issues to change other parts of the system to > use the strict functions. Thanks, I've updated per these comments. - PR Comment: https://git.openjdk.org/jdk/pull/14083#issuecomment-1561899890
Re: RFR: 8300038: Make new version of JNU_GetStringPlatformChars which checks for null characters [v4]
> This PR creates a new version of the JNI utility function > JNU_GetStringPlatformChars called JNU_GetStringPlatformCharsStrict, which > performs additional validation of the returned string, namely that it does > not contain any embedded NULL characters. If any such characters are found > the function returns NULL with an IAE pending. The change also switches usage > in the networking native code to use the new function. > > This cautious approach was taken rather than changing the behavior of the > existing function as each native code area needs to review the effect of > making the switch. Otherwise, surprising behavior changes might occur (eg > undocumented IAE being thrown to user code instead of some other exception). Michael McMahon has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 12 commits: - Merge branch 'master' into nullStrings - error message and test update - Merge branch 'master' into nullStrings - test comment update - test update - Merge branch 'master' into nullStrings - exception message update - test update - remve whitespace - update - ... and 2 more: https://git.openjdk.org/jdk/compare/207fbcb0...35df1a67 - Changes: https://git.openjdk.org/jdk/pull/14083/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14083&range=03 Stats: 188 lines in 9 files changed: 163 ins; 1 del; 24 mod Patch: https://git.openjdk.org/jdk/pull/14083.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14083/head:pull/14083 PR: https://git.openjdk.org/jdk/pull/14083
Re: RFR: 8300038: Make new version of JNU_GetStringPlatformChars which checks for null characters [v3]
On Wed, 24 May 2023 10:14:23 GMT, Michael McMahon wrote: >> This PR creates a new version of the JNI utility function >> JNU_GetStringPlatformChars called JNU_GetStringPlatformCharsStrict, which >> performs additional validation of the returned string, namely that it does >> not contain any embedded NULL characters. If any such characters are found >> the function returns NULL with an IAE pending. The change also switches >> usage in the networking native code to use the new function. >> >> This cautious approach was taken rather than changing the behavior of the >> existing function as each native code area needs to review the effect of >> making the switch. Otherwise, surprising behavior changes might occur (eg >> undocumented IAE being thrown to user code instead of some other exception). > > Michael McMahon has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 10 commits: > > - Merge branch 'master' into nullStrings > - test comment update > - test update > - Merge branch 'master' into nullStrings > - exception message update > - test update > - remve whitespace > - update > - Merge branch 'master' into nullStrings > - first impl I skimmed through the changes and mostly look okay. There's a few usage of "NULL" in exception messages that I assume should be "NUL character". Probably need to bump the copyright date on several files. You might want to find a better name for the test as NullChar isn't too descriptive, the test is really doing a lookup of a hostname that contains a NUL character. I assume there will be follow up issues to change other parts of the system to use the strict functions. - PR Comment: https://git.openjdk.org/jdk/pull/14083#issuecomment-1561271438
Re: RFR: 8300038: Make new version of JNU_GetStringPlatformChars which checks for null characters [v3]
> This PR creates a new version of the JNI utility function > JNU_GetStringPlatformChars called JNU_GetStringPlatformCharsStrict, which > performs additional validation of the returned string, namely that it does > not contain any embedded NULL characters. If any such characters are found > the function returns NULL with an IAE pending. The change also switches usage > in the networking native code to use the new function. > > This cautious approach was taken rather than changing the behavior of the > existing function as each native code area needs to review the effect of > making the switch. Otherwise, surprising behavior changes might occur (eg > undocumented IAE being thrown to user code instead of some other exception). Michael McMahon has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 10 commits: - Merge branch 'master' into nullStrings - test comment update - test update - Merge branch 'master' into nullStrings - exception message update - test update - remve whitespace - update - Merge branch 'master' into nullStrings - first impl - Changes: https://git.openjdk.org/jdk/pull/14083/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14083&range=02 Stats: 183 lines in 9 files changed: 163 ins; 1 del; 19 mod Patch: https://git.openjdk.org/jdk/pull/14083.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14083/head:pull/14083 PR: https://git.openjdk.org/jdk/pull/14083
Re: RFR: 8300038: Make new version of JNU_GetStringPlatformChars which checks for null characters [v2]
On Tue, 23 May 2023 15:31:49 GMT, Michael McMahon wrote: >> This PR creates a new version of the JNI utility function >> JNU_GetStringPlatformChars called JNU_GetStringPlatformCharsStrict, which >> performs additional validation of the returned string, namely that it does >> not contain any embedded NULL characters. If any such characters are found >> the function returns NULL with an IAE pending. The change also switches >> usage in the networking native code to use the new function. >> >> This cautious approach was taken rather than changing the behavior of the >> existing function as each native code area needs to review the effect of >> making the switch. Otherwise, surprising behavior changes might occur (eg >> undocumented IAE being thrown to user code instead of some other exception). > > Michael McMahon has updated the pull request incrementally with one > additional commit since the last revision: > > test comment update Looks good - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14083#pullrequestreview-1440123393
Re: RFR: 8300038: Make new version of JNU_GetStringPlatformChars which checks for null characters [v2]
On Tue, 23 May 2023 15:31:49 GMT, Michael McMahon wrote: >> This PR creates a new version of the JNI utility function >> JNU_GetStringPlatformChars called JNU_GetStringPlatformCharsStrict, which >> performs additional validation of the returned string, namely that it does >> not contain any embedded NULL characters. If any such characters are found >> the function returns NULL with an IAE pending. The change also switches >> usage in the networking native code to use the new function. >> >> This cautious approach was taken rather than changing the behavior of the >> existing function as each native code area needs to review the effect of >> making the switch. Otherwise, surprising behavior changes might occur (eg >> undocumented IAE being thrown to user code instead of some other exception). > > Michael McMahon has updated the pull request incrementally with one > additional commit since the last revision: > > test comment update Looks good to me, but it would be good to get this reviewed by someone with more knowledge of native character encoding. - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14083#pullrequestreview-1439991999
Re: RFR: 8300038: Make new version of JNU_GetStringPlatformChars which checks for null characters [v2]
On Tue, 23 May 2023 15:18:36 GMT, Michael McMahon wrote: >> This PR creates a new version of the JNI utility function >> JNU_GetStringPlatformChars called JNU_GetStringPlatformCharsStrict, which >> performs additional validation of the returned string, namely that it does >> not contain any embedded NULL characters. If any such characters are found >> the function returns NULL with an IAE pending. The change also switches >> usage in the networking native code to use the new function. >> >> This cautious approach was taken rather than changing the behavior of the >> existing function as each native code area needs to review the effect of >> making the switch. Otherwise, surprising behavior changes might occur (eg >> undocumented IAE being thrown to user code instead of some other exception). > > Michael McMahon has updated the pull request incrementally with one > additional commit since the last revision: > > test comment update test/jdk/java/net/InetAddress/NullCharDriver.java line 29: > 27: * @modules java.base/java.net > 28: * @compile/module=java.base java/net/NullChar.java > 29: * @summary foo It would be good to have a better summary ;-) - PR Review Comment: https://git.openjdk.org/jdk/pull/14083#discussion_r1202541587
Re: RFR: 8300038: Make new version of JNU_GetStringPlatformChars which checks for null characters [v2]
> This PR creates a new version of the JNI utility function > JNU_GetStringPlatformChars called JNU_GetStringPlatformCharsStrict, which > performs additional validation of the returned string, namely that it does > not contain any embedded NULL characters. If any such characters are found > the function returns NULL with an IAE pending. The change also switches usage > in the networking native code to use the new function. > > This cautious approach was taken rather than changing the behavior of the > existing function as each native code area needs to review the effect of > making the switch. Otherwise, surprising behavior changes might occur (eg > undocumented IAE being thrown to user code instead of some other exception). Michael McMahon has updated the pull request incrementally with one additional commit since the last revision: test comment update - Changes: - all: https://git.openjdk.org/jdk/pull/14083/files - new: https://git.openjdk.org/jdk/pull/14083/files/8cf24635..0acc456a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14083&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14083&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14083.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14083/head:pull/14083 PR: https://git.openjdk.org/jdk/pull/14083
RFR: 8300038: Make new version of JNU_GetStringPlatformChars which checks for null characters
This PR creates a new version of the JNI utility function JNU_GetStringPlatformChars called JNU_GetStringPlatformCharsStrict, which performs additional validation of the returned string, namely that it does not contain any embedded NULL characters. If any such characters are found the function returns NULL with an IAE pending. The change also switches usage in the networking native code to use the new function. This cautious approach was taken rather than changing the behavior of the existing function as each native code area needs to review the effect of making the switch. Otherwise, surprising behavior changes might occur (eg undocumented IAE being thrown to user code instead of some other exception). - Commit messages: - test update - Merge branch 'master' into nullStrings - exception message update - test update - remve whitespace - update - Merge branch 'master' into nullStrings - first impl Changes: https://git.openjdk.org/jdk/pull/14083/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14083&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8300038 Stats: 183 lines in 9 files changed: 163 ins; 1 del; 19 mod Patch: https://git.openjdk.org/jdk/pull/14083.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14083/head:pull/14083 PR: https://git.openjdk.org/jdk/pull/14083
