[Hadoop-common-dev] [jira] Created: (HADOOP-4475) The (deprecated) method NetUtils.getServerAddress() doesn't validate configuration values enough

[ https://issues.apache.org/jira/browse/HADOOP-4475?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12641376#action_12641376 ]

Steve Loughran commented on HADOOP-4475:
----------------------------------------

Here's the problem

public static String getServerAddress(Configuration conf,
String oldBindAddressName,
String oldPortName,
String newBindAddressName) {

//The method reads in the old and new binding values
String oldAddr = conf.get(oldBindAddressName);
String oldPort = conf.get(oldPortName);
String newAddrPort = conf.get(newBindAddressName);

//if the old values are null: the new value is returned without any validation
if (oldAddr == null && oldPort == null) {
return newAddrPort;
}

//only if either of the old binding values are unset, is there any attempt to validate the newAddPort value
String[] newAddrPortParts = newAddrPort.split(":",2);
if (newAddrPortParts.length != 2) {
throw new IllegalArgumentException("Invalid address/port: " +
newAddrPort);
}
...

[ https://issues.apache.org/jira/browse/HADOOP-4475?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12641378#action_12641378 ]

Steve Loughran commented on HADOOP-4475:
----------------------------------------

Looking at where this method is used, bad binding values could trigger NPEs whenever the return value is fed straight into NetUtils.createSocketAddr(), which is most places it is used: JobTracker, TaskTracker, SecondaryNameNode, FSNameSystem. While an NPE is a valid failure, it would be better for diagnoscis for getServerAddress() to detect the null pointer and bail out early, including in the exception the name of the attribute that was undefined.

[ https://issues.apache.org/jira/browse/HADOOP-4475?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12666148#action_12666148 ]

Steve Loughran commented on HADOOP-4475:
----------------------------------------

That's HADOOP-2827

[ https://issues.apache.org/jira/browse/HADOOP-4475?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Steve Loughran updated HADOOP-4475:
-----------------------------------

Attachment: hadoop-4475.patch

This patch detects null address values early, and fails with a meaningful error, rather than an NPE, whenever there is no value for the newBindAddressName defined in the configuration. Currently a NPE is triggered either in {{NetUtils.getServerAddress()}}, or in the code to create an InetAddress which invariably follows it.


It includes tests that verify that
* attempts to retrieve an undefined value fail
* you cannot use the oldBindAddress/oldBindPort values unless the newBindAddressName value is set in the configuration
* A non-tuple "hostname" is valid as the newBindAddressName, it is up to {{NetUtils.createSocketAddr()}} to do the final parsing and validaton
* invalid triples "host:port:port" get picked up later in {{NetUtils.createSocketAddr()}}
* invalid port numbers "host:twelve" are picked up in {{NetUtils.createSocketAddr()}}
* negative port numbers "host:-23" are picked up in {{NetUtils.createSocketAddr()}}
These are all the existing semantics of the method, now with tests to catch regressions.

[ https://issues.apache.org/jira/browse/HADOOP-4475?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Steve Loughran updated HADOOP-4475:
-----------------------------------

Status: Patch Available (was: Open)

patch with tests.

[ https://issues.apache.org/jira/browse/HADOOP-4475?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Tom White updated HADOOP-4475:
------------------------------

Status: Open (was: Patch Available)

[ https://issues.apache.org/jira/browse/HADOOP-4475?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Tom White updated HADOOP-4475:
------------------------------

Assignee: Steve Loughran
Hadoop Flags: [Reviewed]
Status: Patch Available (was: Open)

+1 This looks good to me. Resubmitting to try to get Hudson to pick it up.

[ https://issues.apache.org/jira/browse/HADOOP-4475?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12671876#action_12671876 ]

Hadoop QA commented on HADOOP-4475:
-----------------------------------

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12398475/hadoop-4475.patch
against trunk revision 742409.

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 3 new or modified tests.

+1 javadoc. The javadoc tool did not generate any warning messages.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 findbugs. The patch does not introduce any new Findbugs warnings.

+1 Eclipse classpath. The patch retains Eclipse classpath integrity.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

-1 core tests. The patch failed core unit tests.

-1 contrib tests. The patch failed contrib unit tests.

Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3817/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3817/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3817/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3817/console

This message is automatically generated.

[ https://issues.apache.org/jira/browse/HADOOP-4475?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12671941#action_12671941 ]

Steve Loughran commented on HADOOP-4475:
----------------------------------------

chukwa failed here with a JVM exit; unlikely to be related

[ https://issues.apache.org/jira/browse/HADOOP-4475?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Chris Douglas updated HADOOP-4475:
----------------------------------

Status: Open (was: Patch Available)

Browsing through HADOOP-2404 where this was introduced, this looks like it's adding tests for a compatibility bridge we crossed long ago... I favor removing this method per HADOOP-2827, unless there are some concrete use cases...

[ https://issues.apache.org/jira/browse/HADOOP-4475?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Chris Douglas resolved HADOOP-4475.
-----------------------------------

Resolution: Invalid

NetUtils::getServerAddress was removed in HADOOP-2827