Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v5]
On Thu, 12 May 2022 10:08:23 GMT, Daniel Fuchs wrote:
>> src/java.net.http/share/classes/jdk/internal/net/http/websocket/Frame.java
>> line 291:
>>
>>> 289:
>>> 290: HeaderWriter noMask() {
>>> 291: // The negation "~" sets the high order bits
>>
>> Rubber-stamping this in front of every of the four closely sitting casts
>> seems excessive:
>>
>> // The negation "~" sets the high order bits
>> // so the value is more than 16 bits and the
>> // compiler will emit a warning if not cast
>
> I don't disagree. I had the same feeling. But I don't necessarily read a file
> from top to bottom - and someone just glancing at one of these methods might
> wonder why one line has the cast and the other hasn't.
Readers should always consider as much of the context as practical; but I like
your solution.
-
PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v5]
On Thu, 12 May 2022 09:15:19 GMT, Pavel Rappo wrote:
>> Daniel Fuchs has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Revert adding char constants
>
> src/java.net.http/share/classes/jdk/internal/net/http/websocket/Frame.java
> line 291:
>
>> 289:
>> 290: HeaderWriter noMask() {
>> 291: // The negation "~" sets the high order bits
>
> Rubber-stamping this in front of every of the four closely sitting casts
> seems excessive:
>
> // The negation "~" sets the high order bits
> // so the value is more than 16 bits and the
> // compiler will emit a warning if not cast
I don't disagree. I had the same feeling. But I don't necessarily read a file
from top to bottom - and someone just glancing at one of these methods might
wonder why one line has the cast and the other hasn't.
-
PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v5]
On Thu, 12 May 2022 09:00:37 GMT, Pavel Rappo wrote:
>> This is what I mean:
>>
>> jshell> long codeLengthOf = (long)Integer.MAX_VALUE + 1
>> codeLengthOf ==> 2147483648
>>
>> jshell> int bufferLen = 0
>> bufferLen ==> 0
>>
>> jshell> bufferLen + codeLengthOf <= 64
>> $3 ==> false
>>
>> jshell> bufferLen + (int)codeLengthOf <= 64
>> $4 ==> true
>
> Yes, inserting explicit casts seems less clean than changing `codeLengthOf`
> to this:
>
> private static int codeLengthOf(char c) {
> return (int) (codes[c] & 0xL);
> }
>
> There are 256 elements in constant `long[] codes`. One could easily check
> that each element when ANDed with `0xL` results in a value
> that fits into the first 31 bits of `int`.
OK - I will change codeLengthOf as suggested. It was not immediately obvious
to me that the values would fit in the first 31 bits.
-
PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v5]
On Wed, 11 May 2022 18:42:46 GMT, Daniel Fuchs wrote:
>> In relation to
>> [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find
>> here a patch that addresses possibly lossy conversions in java.net.http
>
> Daniel Fuchs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Revert adding char constants
src/java.net.http/share/classes/jdk/internal/net/http/websocket/Frame.java line
291:
> 289:
> 290: HeaderWriter noMask() {
> 291: // The negation "~" sets the high order bits
Rubber-stamping this in front of every of the four closely sitting casts seems
excessive:
// The negation "~" sets the high order bits
// so the value is more than 16 bits and the
// compiler will emit a warning if not cast
-
PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v5]
On Thu, 12 May 2022 08:56:26 GMT, Daniel Fuchs wrote:
>> No because the int returned could be negative, while the long will not.
>> Assuming bufferLen is 0 and codeLengthOf() returns some value that has the
>> 32th bit set to 1 then when codeLengthOf() returns long, bufferLen +
>> codeLengthOf() will be a positive long > 64, and we won't enter the `if`
>> here but if codeLengthOf() returns `int`, then bufferLen + codeLengthOf()
>> would be negative and the `if` would be wrongly entered. I am not 100% sure
>> this is a scenario that might occur (codeLengthOf() returning large
>> "unsigned int" values) - but I'd prefer to stay on the safe side and assume
>> that it can.
>
> This is what I mean:
>
> jshell> long codeLengthOf = (long)Integer.MAX_VALUE + 1
> codeLengthOf ==> 2147483648
>
> jshell> int bufferLen = 0
> bufferLen ==> 0
>
> jshell> bufferLen + codeLengthOf <= 64
> $3 ==> false
>
> jshell> bufferLen + (int)codeLengthOf <= 64
> $4 ==> true
Yes, inserting explicit casts seems less clean than changing `codeLengthOf` to
this:
private static int codeLengthOf(char c) {
return (int) (codes[c] & 0xL);
}
There are 256 elements in constant `long[] codes`. One could easily check that
each element when ANDed with `0xL` results in a value that fits
into the first 31 bits of `int`.
-
PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v5]
On Thu, 12 May 2022 08:42:39 GMT, Daniel Fuchs wrote: >> codeLengthOf() returns long. It could be changed to return int with a cast >> internally and then you could avoid the two new casts. > > No because the int returned could be negative, while the long will not. > Assuming bufferLen is 0 and codeLengthOf() returns some value that has the > 32th bit set to 1 then when codeLengthOf() returns long, bufferLen + > codeLengthOf() will be a positive long > 64, and we won't enter the `if` here > but if codeLengthOf() returns `int`, then bufferLen + codeLengthOf() would be > negative and the `if` would be wrongly entered. I am not 100% sure this is a > scenario that might occur (codeLengthOf() returning large "unsigned int" > values) - but I'd prefer to stay on the safe side and assume that it can. This is what I mean: jshell> long codeLengthOf = (long)Integer.MAX_VALUE + 1 codeLengthOf ==> 2147483648 jshell> int bufferLen = 0 bufferLen ==> 0 jshell> bufferLen + codeLengthOf <= 64 $3 ==> false jshell> bufferLen + (int)codeLengthOf <= 64 $4 ==> true - PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v5]
On Wed, 11 May 2022 20:31:00 GMT, Michael McMahon wrote: >> I believe the method returns an "unsigned int" - having the method return an >> int would then potentially cause `bufferLen + len <= 64` to yield true when >> it shouldn't. Hopefully @pavelrappo will comment. > > codeLengthOf() returns long. It could be changed to return int with a cast > internally and then you could avoid the two new casts. No because the int returned could be negative, while the long will not. Assuming bufferLen is 0 and codeLengthOf() returns some value that has the 32th bit set to 1 then when codeLengthOf() returns long, bufferLen + codeLengthOf() will be a positive long > 64, and we won't enter the `if` here but if codeLengthOf() returns `int`, then bufferLen + codeLengthOf() would be negative and the `if` would be wrongly entered. I am not 100% sure this is a scenario that might occur (codeLengthOf() returning large "unsigned int" values) - but I'd prefer to stay on the safe side and assume that it can. - PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v5]
On Wed, 11 May 2022 17:10:45 GMT, Daniel Fuchs wrote: >> src/java.net.http/share/classes/jdk/internal/net/http/hpack/QuickHuffman.java >> line 739: >> >>> 737: buffer |= (codeValueOf(c) >>> bufferLen); // >>> append >>> 738: bufferLen += (int) len; >>> 739: pos++; >> >> Would it be cleaner to do the cast in the codeLengthOf method, so it returns >> an int? > > I believe the method returns an "unsigned int" - having the method return an > int would then potentially cause `bufferLen + len <= 64` to yield true when > it shouldn't. Hopefully @pavelrappo will comment. codeLengthOf() returns long. It could be changed to return int with a cast internally and then you could avoid the two new casts. - PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v5]
> In relation to > [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find > here a patch that addresses possibly lossy conversions in java.net.http Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision: Revert adding char constants - Changes: - all: https://git.openjdk.java.net/jdk/pull/8656/files - new: https://git.openjdk.java.net/jdk/pull/8656/files/fbf3c9a1..2334b747 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8656=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8656=03-04 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8656.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8656/head:pull/8656 PR: https://git.openjdk.java.net/jdk/pull/8656
