On Thu, 11 Apr 2024 15:27:55 GMT, Sergey Chernyshev <schernys...@openjdk.org>
wrote:
>> There are two distinct approaches to parsing IPv4 literal addresses. One is
>> the Java baseline "strict" syntax (all-decimal d.d.d.d form family), another
>> one is the "loose" syntax of RFC 6943 section 3.1.1 [1] (POSIX `inet_addr`
>> allowing octal and hexadecimal forms [2]). The goal of this PR is to provide
>> interface to construct InetAddress objects from literal addresses in POSIX
>> form, to applications that need to mimic the behavior of `inet_addr` used by
>> standard network utilities such as netcat/curl/wget and the majority of web
>> browsers. At present time, there's no way to construct `InetAddress` object
>> from such literal addresses because the existing APIs such as
>> `InetAddress.getByName()`, `InetAddress#ofLiteral()` and
>> `Inet4Address#ofLiteral()` will consume an address and successfully parse it
>> as decimal, regardless of the octal prefix. Hence, the resulting object will
>> point to a different IP address.
>>
>> Historically `InetAddress.getByName()/.getAllByName()` were the only way to
>> convert a literal address into an InetAddress object. `getAllByName()` API
>> relies on POSIX `getaddrinfo` / `inet_addr` which parses IP address segments
>> with `strtoul` (accepts octal and hexadecimal bases).
>>
>> The fallback to `getaddrinfo` is undesirable as it may end up with network
>> queries (blocking mode), if `inet_addr` rejects the input literal address.
>> The Java standard explicitly says that
>>
>> "If a literal IP address is supplied, only the validity of the address
>> format is checked."
>>
>> @AlekseiEfimov contributed JDK-8272215 [3] that adds new factory methods
>> `.ofLiteral()` to `InetAddress` classes. Although the new API is not
>> affected by the `getaddrinfo` fallback issue, it is not sufficient for an
>> application that needs to interoperate with external tooling that follows
>> POSIX standard. In the current state, `InetAddress#ofLiteral()` and
>> `Inet4Address#ofLiteral()` will consume the input literal address and
>> (regardless of the octal prefix) parse it as decimal numbers. Hence, it's
>> not possible to reliably construct an `InetAddress` object from a literal
>> address in POSIX form that would point to the desired host.
>>
>> It is proposed to extend the factory methods with
>> `Inet4Address#ofPosixLiteral()` that allows parsing literal IP(v4) addresses
>> in "loose" syntax, compatible with `inet_addr` POSIX api. The implementation
>> is based on `.isBsdParsableV4()` method added along with JDK-8277608 [4].
>> The changes in the original algorithm are as follows:
>>
>> - `IPAddressUtil#parseB...
>
> Sergey Chernyshev has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - Update src/java.base/share/classes/java/net/Inet4Address.java
>
> Co-authored-by: Daniel Fuchs <67001856+df...@users.noreply.github.com>
> - Update src/java.base/share/classes/java/net/Inet4Address.java
>
> Co-authored-by: Daniel Fuchs <67001856+df...@users.noreply.github.com>
The parsing code and the test changes look good to me with a couple of minor
suggestions.
src/java.base/share/classes/java/net/Inet4Address.java line 321:
> 319: * cannot be parsed as an IPv4 address literal and {@code throwIAE}
> is {@code true}.
> 320: */
> 321: static Inet4Address parseAddressStringPosix(String addressLiteral,
> boolean throwIAE) {
This method can be made `private` since it's only used in the `Inet4Address`
class.
`throwIAE` is always `true` and therefore can be removed.
src/java.base/share/classes/sun/net/util/IPAddressUtil.java line 711:
> 709: fieldValue = parseV4FieldBsd(radix, charBuffer,
> fieldNumber);
> 710: if (fieldValue >= 0) {
> 711: if (fieldValue < 256) {
Maybe this check can be acccompanied with a comment to clarify the following,
ie if a field value is greater than `255` then it can only be the last field.
If it is not the last one then `parseV4FieldBsd` enforces this limit and will
return `null`.
That would help future code maintainers :)
src/java.base/share/classes/sun/net/util/IPAddressUtil.java line 730:
> 728: return null;
> 729: }
> 730: // If the last fieldValue is greater than 255 (fieldNumer < 4),
typo:
Suggestion:
// If the last fieldValue is greater than 255 (fieldNumber < 4),
test/jdk/java/net/InetAddress/OfLiteralTest.java line 26:
> 24: /* @test
> 25: * @bug 8272215 8315767
> 26: * @summary Test for ofLiteral API in InetAddress classes
The test summary can mention that `ofLiteralPosix` is also tested here
test/jdk/java/net/InetAddress/OfLiteralTest.java line 126:
> 124: byte[] ipv4_0_0_0_34 = new byte[]{0, 0, 0, 34};
> 125:
> 126: // 127.0.1.1 address bytes
Update comment to match the code:
Suggestion:
// 127.0.1.2 address bytes
test/jdk/java/net/InetAddress/OfLiteralTest.java line 129:
> 127: byte[] ipv4_127_0_1_2 = new byte[]{127, 0, 1, 2};
> 128:
> 129: // 127.1.1.1 address bytes
Update comment to match the code:
Suggestion:
// 127.1.2.3 address bytes
-------------
PR Review: https://git.openjdk.org/jdk/pull/18493#pullrequestreview-2004651764
PR Review Comment: https://git.openjdk.org/jdk/pull/18493#discussion_r1567975404
PR Review Comment: https://git.openjdk.org/jdk/pull/18493#discussion_r1567996061
PR Review Comment: https://git.openjdk.org/jdk/pull/18493#discussion_r1567984828
PR Review Comment: https://git.openjdk.org/jdk/pull/18493#discussion_r1568000966
PR Review Comment: https://git.openjdk.org/jdk/pull/18493#discussion_r1567998369
PR Review Comment: https://git.openjdk.org/jdk/pull/18493#discussion_r1567998873
