Re: RFR: 7065228: To interpret case-insensitive string locale independently [v2]
On Fri, 19 May 2023 11:24:30 GMT, Michael McMahon wrote: > Seems like a useful change and I can see how issues could arise if strings > were stored somewhere after being upper/lower cased and then reused in a > different locale. > > Is it correct to say that the assumption is these strings are all supposed to > be US ASCII (eg protocol defined identifiers, or hostnames etc) rather than > user generated text strings? That seems to be the case as far as I can see. Yes - PR Comment: https://git.openjdk.org/jdk/pull/14006#issuecomment-1556955595
Re: RFR: 7065228: To interpret case-insensitive string locale independently [v2]
On Wed, 17 May 2023 13:53:55 GMT, Darragh Clarke wrote: >> Updated instances of `toLowerCase` and `toUpperCase` in several net and io >> files to specify `Locale.ROOT` to ensure that case conversion issues don't >> occur, >> >> I didn't add any new tests but ran tier 1-3 with no issues > > Darragh Clarke has updated the pull request incrementally with two additional > commits since the last revision: > > - Update > src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java > >Co-authored-by: Daniel Jelinski > - removed StreamTokenizer changes, will make seperate ticket for those Marked as reviewed by michaelm (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14006#pullrequestreview-1434277855
Re: RFR: 7065228: To interpret case-insensitive string locale independently [v2]
On Wed, 17 May 2023 13:53:55 GMT, Darragh Clarke wrote: >> Updated instances of `toLowerCase` and `toUpperCase` in several net and io >> files to specify `Locale.ROOT` to ensure that case conversion issues don't >> occur, >> >> I didn't add any new tests but ran tier 1-3 with no issues > > Darragh Clarke has updated the pull request incrementally with two additional > commits since the last revision: > > - Update > src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java > >Co-authored-by: Daniel Jelinski > - removed StreamTokenizer changes, will make seperate ticket for those Seems like a useful change and I can see how issues could arise if strings were stored somewhere after being upper/lower cased and then reused in a different locale. Is it correct to say that the assumption is these strings are all supposed to be US ASCII (eg protocol defined identifiers, or hostnames etc) rather than user generated text strings? That seems to be the case as far as I can see. - PR Comment: https://git.openjdk.org/jdk/pull/14006#issuecomment-1554431858
Re: RFR: 7065228: To interpret case-insensitive string locale independently [v2]
On Wed, 17 May 2023 13:53:55 GMT, Darragh Clarke wrote: >> Updated instances of `toLowerCase` and `toUpperCase` in several net and io >> files to specify `Locale.ROOT` to ensure that case conversion issues don't >> occur, >> >> I didn't add any new tests but ran tier 1-3 with no issues > > Darragh Clarke has updated the pull request incrementally with two additional > commits since the last revision: > > - Update > src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java > >Co-authored-by: Daniel Jelinski > - removed StreamTokenizer changes, will make seperate ticket for those These changes look fine to me. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14006#pullrequestreview-1433788568
Re: RFR: 7065228: To interpret case-insensitive string locale independently [v2]
On Wed, 17 May 2023 13:53:55 GMT, Darragh Clarke wrote: >> Updated instances of `toLowerCase` and `toUpperCase` in several net and io >> files to specify `Locale.ROOT` to ensure that case conversion issues don't >> occur, >> >> I didn't add any new tests but ran tier 1-3 with no issues > > Darragh Clarke has updated the pull request incrementally with two additional > commits since the last revision: > > - Update > src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java > >Co-authored-by: Daniel Jelinski > - removed StreamTokenizer changes, will make seperate ticket for those LGTM - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14006#pullrequestreview-1432410653
Re: RFR: 7065228: To interpret case-insensitive string locale independently [v2]
On Wed, 17 May 2023 13:53:55 GMT, Darragh Clarke wrote: >> Updated instances of `toLowerCase` and `toUpperCase` in several net and io >> files to specify `Locale.ROOT` to ensure that case conversion issues don't >> occur, >> >> I didn't add any new tests but ran tier 1-3 with no issues > > Darragh Clarke has updated the pull request incrementally with two additional > commits since the last revision: > > - Update > src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java > >Co-authored-by: Daniel Jelinski > - removed StreamTokenizer changes, will make seperate ticket for those Marked as reviewed by djelinski (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14006#pullrequestreview-1432388069
Re: RFR: 7065228: To interpret case-insensitive string locale independently [v2]
On Wed, 17 May 2023 10:41:57 GMT, Daniel Jeliński wrote:
>> Darragh Clarke has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Update
>> src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java
>>
>>Co-authored-by: Daniel Jelinski
>> - removed StreamTokenizer changes, will make seperate ticket for those
>
> src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java
> line 516:
>
>> 514:
>> 515: if (nccount != -1) {
>> 516: ncstring = Integer.toHexString
>> (nccount).toLowerCase(Locale.ROOT);
>
> Suggestion:
>
> ncstring = Integer.toHexString(nccount);
>
> `toHexString` returns lowercase string
Thanks for pointing that out!
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14006#discussion_r1196555183
Re: RFR: 7065228: To interpret case-insensitive string locale independently [v2]
> Updated instances of `toLowerCase` and `toUpperCase` in several net and io > files to specify `Locale.ROOT` to ensure that case conversion issues don't > occur, > > I didn't add any new tests but ran tier 1-3 with no issues Darragh Clarke has updated the pull request incrementally with two additional commits since the last revision: - Update src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java Co-authored-by: Daniel Jelinski - removed StreamTokenizer changes, will make seperate ticket for those - Changes: - all: https://git.openjdk.org/jdk/pull/14006/files - new: https://git.openjdk.org/jdk/pull/14006/files/a79969c2..6d40e1c8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14006=01 - incr: https://webrevs.openjdk.org/?repo=jdk=14006=00-01 Stats: 3 lines in 2 files changed: 0 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/14006.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14006/head:pull/14006 PR: https://git.openjdk.org/jdk/pull/14006
Re: RFR: 7065228: To interpret case-insensitive string locale independently
On Tue, 16 May 2023 10:38:52 GMT, Darragh Clarke wrote:
> Updated instances of `toLowerCase` and `toUpperCase` in several net and io
> files to specify `Locale.ROOT` to ensure that case conversion issues don't
> occur,
>
> I didn't add any new tests but ran tier 1-3 with no issues
src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java
line 516:
> 514:
> 515: if (nccount != -1) {
> 516: ncstring = Integer.toHexString
> (nccount).toLowerCase(Locale.ROOT);
Suggestion:
ncstring = Integer.toHexString(nccount);
`toHexString` returns lowercase string
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14006#discussion_r1196296029
Re: RFR: 7065228: To interpret case-insensitive string locale independently
On Tue, 16 May 2023 10:38:52 GMT, Darragh Clarke wrote: > Updated instances of `toLowerCase` and `toUpperCase` in several net and io > files to specify `Locale.ROOT` to ensure that case conversion issues don't > occur, > > I didn't add any new tests but ran tier 1-3 with no issues LGTM. Nice clean-up! - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14006#pullrequestreview-1428917297
Re: RFR: 7065228: To interpret case-insensitive string locale independently
On Tue, 16 May 2023 10:38:52 GMT, Darragh Clarke wrote: > Updated instances of `toLowerCase` and `toUpperCase` in several net and io > files to specify `Locale.ROOT` to ensure that case conversion issues don't > occur, > > I didn't add any new tests but ran tier 1-3 with no issues Looks generally like a good improvement. In those places `toLowerCase` is called on strings (or substrings) that are expected to be ASCII, so using Locale.ROOT looks appropriate. Would be good to get @Michael-Mc-Mahon validate these changes too. src/java.base/share/classes/java/io/StreamTokenizer.java line 632: > 630: sval = String.copyValueOf(buf, 0, i); > 631: if (forceLower) > 632: sval = sval.toLowerCase(Locale.ROOT); This one gave me pause. AFAICS it's probably OK - but it might warant a CSR and an update of the specification to explicitly state how the lower case conversion is performed (update the `lowerCaseMode` method spec). Could we handle that in a separate PR? - PR Review: https://git.openjdk.org/jdk/pull/14006#pullrequestreview-1428569684 PR Review Comment: https://git.openjdk.org/jdk/pull/14006#discussion_r1195156280
