8315767: InetAddress: constructing objects from BSD literal addresses

[sercher]

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#parseBsdLiteralV4() method is extracted from .isBsdParsableV4()
• an additional check is added, whether an input string is empty
• null is returned whenever the original algorithm fails
• a condition was added to the parser loop, that stores the IPv4 address segment value when it fits in 1 byte (0 <= x < 256)
• an additional check was added to verify that the last field value is non-negative
• when the last field value is multi-byte (the number of fields is less than 4), it is written to the last (4-(fieldNumber-1)) octets

The new method hasn't been added to InetAddress superclass because the change is only related to IPv4 addressing. This reduces the chance that client code will call the wrong factory method.

test/jdk/java/net/InetAddress/OfLiteralTest.java was updated to include .ofPosixLiteral() tests

Javadocs in Inet4Address were updated accordingly

The new method can be used as follows

import java.net.InetAddress;
import java.net.Inet4Address;

public class Test {
    public static void main(String[] args) throws Throwable {
        if (args.length < 1) {
            System.err.println("USAGE: java Test <host>");
            return;
        }
        InetAddress ia = Inet4Address.ofPosixLiteral(args[0]);
        System.out.println(ia.toString());
    }
}

The output would be

$ ./build/images/jdk/bin/java Test 2130706433
/127.0.0.1
$ ./build/images/jdk/bin/java Test 02130706433
/17.99.141.27
$ ./build/images/jdk/bin/java Test 2130706438
/127.0.0.6
$ ./build/images/jdk/bin/java Test 02130706438
Exception in thread "main" java.lang.IllegalArgumentException: Invalid IP address literal: 02130706438
        at java.base/sun.net.util.IPAddressUtil.invalidIpAddressLiteral(IPAddressUtil.java:169)
        at java.base/java.net.Inet4Address.parseAddressStringPosix(Inet4Address.java:302)
        at java.base/java.net.Inet4Address.ofPosixLiteral(Inet4Address.java:239)
        at Test.main(Test.java:10)

[1] https://www.ietf.org/rfc/rfc6943.html#section-3.1.1
[2] https://pubs.opengroup.org/onlinepubs/9699919799/functions/inet_addr.html
[3] https://bugs.openjdk.org/browse/JDK-8272215
[4] cdc1582

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Change requires CSR request JDK-8329876 to be approved
• Commit message must refer to an issue

Issues

• JDK-8315767: InetAddress: constructing objects from BSD literal addresses (Enhancement - P3)
• JDK-8329876: InetAddress: constructing objects from BSD literal addresses (CSR)

Reviewers

• Daniel Fuchs (@dfuch - Reviewer) ⚠️ Review applies to 007807ac
• Aleksei Efimov (@AlekseiEfimov - Reviewer)
• Michael McMahon (@Michael-Mc-Mahon - Reviewer) ⚠️ Review applies to 007807ac
• Jaikiran Pai (@jaikiran - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18493/head:pull/18493
$ git checkout pull/18493

Update a local copy of the PR:
$ git checkout pull/18493
$ git pull https://git.openjdk.org/jdk.git pull/18493/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18493

View PR using the GUI difftool:
$ git pr show -t 18493

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18493.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back schernyshev! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@sercher This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8315767: InetAddress: constructing objects from BSD literal addresses

Reviewed-by: dfuchs, aefimov, michaelm, jpai

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 726 new commits pushed to the master branch:

• 2a11e0d: 8332743: Update comment related to JDK-8320522
• 6829d9a: 8332122: [nmt] Totals for malloc should show total peak
• 9d332e6: 8307193: Several Swing jtreg tests use class.forName on L&F classes
• 98f6a80: 8332490: JMH org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead OOM
• 3d4185a: 8332739: Problemlist compiler/codecache/CheckLargePages until JDK-8332654 is fixed
• c4557a7: 8332463: Byte conditional pattern case element dominates short constant case element
• d59c12f: 8329718: Incorrect @since tags in elements in jdk.compiler and java.compiler
• b4d1454: 8332740: [BACKOUT] JDK-8331081 'internal proprietary API' diagnostics if --system is configured to an earlier JDK version
• 37c4778: 8332096: hotspot-ide-project fails with this-escape
• 2170e99: 8331081: 'internal proprietary API' diagnostics if --system is configured to an earlier JDK version
• ... and 716 more: https://git.openjdk.org/jdk/compare/153410f480c372604e5825bfcd3a63f137e6a013...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@dfuch, @AlekseiEfimov, @Michael-Mc-Mahon, @jaikiran) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[openjdk]

@sercher The following label will be automatically applied to this pull request:

• net

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[AlekseiEfimov]

/csr needed

[openjdk]

@AlekseiEfimov has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@sercher please create a CSR request for issue JDK-8315767 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

[mlbridge]

Webrevs

• 10: Full - Incremental (bdb8f18c)
• 09: Full - Incremental (007807ac)
• 08: Full - Incremental (664fccb1)
• 07: Full - Incremental (6a7a310c)
• 06: Full - Incremental (e07dafad)
• 05: Full - Incremental (2449a331)
• 04: Full - Incremental (d05d9d54)
• 03: Full - Incremental (005fd11e)
• 02: Full - Incremental (ebcbef43)
• 01: Full - Incremental (4230f0f2)
• 00: Full (99ba4544)

[dfuch]

Thanks for taking on the feedback @sercher . I believe we're in a much better place now. I will let @AlekseiEfimov comment on the test and implementation.

[AlekseiEfimov]

The parsing code and the test changes look good to me with a couple of minor suggestions.

[jaikiran]

Should we be setting any expectations by specifying what InetAddress.getHostAddress() will return for an Inet4Address constructed using this new Inet4Address.ofPosixLiteral() method? In its current form I believe it will continue to return a decimal representation of the IP address. My guess is that we want it to continue behaving that way?

[dfuch]

Should we be setting any expectations by specifying what InetAddress.getHostAddress() will return for an Inet4Address constructed using this new Inet4Address.ofPosixLiteral() method? In its current form I believe it will continue to return a decimal representation of the IP address. My guess is that we want it to continue behaving that way?

In the InetAddress class level API documentation we have:

 * <p> For methods that return a textual representation as output
 * value, the first form, i.e. a dotted-quad string, is used.

Do you think it should be reiterated in the @apiNote of the ofPosixLiteral method?

[jaikiran]

Should we be setting any expectations by specifying what InetAddress.getHostAddress() will return for an Inet4Address constructed using this new Inet4Address.ofPosixLiteral() method? In its current form I believe it will continue to return a decimal representation of the IP address. My guess is that we want it to continue behaving that way?

In the InetAddress class level API documentation we have:

 * <p> For methods that return a textual representation as output
 * value, the first form, i.e. a dotted-quad string, is used.

Do you think it should be reiterated in the @apiNote of the ofPosixLiteral method?

The changes proposed in this PR introduce a new paragraph in the class level documentation just before the line which states the dotted-quad string. So I think it may not be clear enough whether dotted-quad string implies decimal values (127.0.0.1) or octal dotted-quad string (0177.0.0.1) or hexadecimal dotted-quad string (0x7F.0.0.1). So I think updating that sentence in class level documentation might help.

[dfuch]

The changes proposed in this PR introduce a new paragraph in the class level documentation just before the line which states the dotted-quad string. So I think it may not be clear enough whether dotted-quad string implies decimal values (127.0.0.1) or octal dotted-quad string (0177.0.0.1) or hexadecimal dotted-quad string (0x7F.0.0.1). So I think updating that sentence in class level documentation might help.

Good point.

[dfuch]

Thanks - LGTM now. @AlekseiEfimov, @Michael-Mc-Mahon are you also good with this version of the changes?

[AlekseiEfimov]

The latest changes LGTM.

[sercher]

Thank you @AlekseiEfimov, @dfuch for your reviews!

[Michael-Mc-Mahon]

I'd just like to have one more look at it today, if that's okay.

[Michael-Mc-Mahon]

Looks good. Just a couple of minor nits to suggest.

[AlanBateman]

I read through the API docs in the latest revision (664fccb) and it looks very good. The new method is not easily discovered, which is okay as it's very much a method for experts, I see wonder if we should put a @see in InetAddress.ofLiteral.

[sercher]

The new method is not easily discovered, which is okay as it's very much a method for experts, I see wonder if we should put a @see in InetAddress.ofLiteral.

Thanks @AlanBateman for your note. There's a link to ofPosixLiteral from the class level doc. I didn't want to touch ofLiteral as it already has the link to the #format subtitle in the class doc

[AlanBateman]

Thanks @AlanBateman for your note. There's a link to ofPosixLiteral from the class level doc. I didn't want to touch ofLiteral as it already has the link to the #format subtitle in the class doc

I think you may have mis-read my comment. I think we need to add @see InetAddress4#ofPosixLiteral(String) to InetAddress.ofLiteral, not Inet4Address.ofLiteral. The reason is that methods defined by Inet4Address are not very discoverable and there is no reference to ofPosixLiteral in InetAddress's class description or anywhere else.

[sercher]

I think you may have mis-read my comment. I think we need to add @see InetAddress4#ofPosixLiteral(String) to InetAddress.ofLiteral, not Inet4Address.ofLiteral. The reason is that methods defined by Inet4Address are not very discoverable and there is no reference to ofPosixLiteral in InetAddress's class description or anywhere else.

Thanks @AlanBateman , apologize for mis-reading your comment. I added @see to InetAddress.ofLiteral, please see the updated doc.

[sercher]

/integrate

[openjdk]

@sercher
Your change (at version bdb8f18) is now ready to be sponsored by a Committer.

[jaikiran]

/sponsor

[openjdk]

Going to push as commit c2180d1.
Since your change was applied there have been 726 commits pushed to the master branch:

• 2a11e0d: 8332743: Update comment related to JDK-8320522
• 6829d9a: 8332122: [nmt] Totals for malloc should show total peak
• 9d332e6: 8307193: Several Swing jtreg tests use class.forName on L&F classes
• 98f6a80: 8332490: JMH org.openjdk.bench.java.util.zip.InflaterInputStreams.inflaterInputStreamRead OOM
• 3d4185a: 8332739: Problemlist compiler/codecache/CheckLargePages until JDK-8332654 is fixed
• c4557a7: 8332463: Byte conditional pattern case element dominates short constant case element
• d59c12f: 8329718: Incorrect @since tags in elements in jdk.compiler and java.compiler
• b4d1454: 8332740: [BACKOUT] JDK-8331081 'internal proprietary API' diagnostics if --system is configured to an earlier JDK version
• 37c4778: 8332096: hotspot-ide-project fails with this-escape
• 2170e99: 8331081: 'internal proprietary API' diagnostics if --system is configured to an earlier JDK version
• ... and 716 more: https://git.openjdk.org/jdk/compare/153410f480c372604e5825bfcd3a63f137e6a013...master

Your commit was automatically rebased without conflicts.

[openjdk]

@jaikiran @sercher Pushed as commit c2180d1.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.