[rfc][icedtea-web] fix the empty string in codebase
Jiri Vanek
jvanek at redhat.com
Wed Sep 17 15:39:13 UTC 2014
On 09/16/2014 08:25 PM, Jacob Wisor wrote:
> On 09/16/2014 at 03:32 PM, Jiri Vanek wrote:
>> I have recently encountered an app, which failed to run, because of malformed
>> jnlp. They had codebase="" and apparently wonted to have ".". Afaik ITW should
>> work around this (oracle plugin does :( ) This patch is handling the "" for
>> codebase as it was "."
>
> Yeah, too bad Oracle's plug-in accepts this sort of "URL" because it is actually invalid. :-( An
> empty string URL is actually not equal nor equivalent to a missing attribute (null string). I
> suppose Oracle's plug-in turns those URLs into File objects internally...
>
>> The app failed for me in alaca dialogue on NPE, teh patch contains small
>> workaround around possible NPE here too + unittest to affected method
>>
>> ok for head? 1.5?
>>
>>
>> Accordingly with investigating this failure, I have enhanced
>> http://icedtea.classpath.org/wiki/IcedTea-Web#Filing_bugs for offline debugging.
>>
>> Volunteer for reproducer?-)
>>
>> Thanx,
>> J.
>
>> diff -r d8e057783109 netx/net/sourceforge/jnlp/Parser.java
>> --- a/netx/net/sourceforge/jnlp/Parser.java Mon Sep 15 14:09:30 2014 -0400
>> +++ b/netx/net/sourceforge/jnlp/Parser.java Tue Sep 16 15:25:44 2014 +0200
>> @@ -1065,10 +1065,21 @@
>> * @throws ParseException if the JNLP file is invalid
>> */
>> public URL getURL(Node node, String name, URL base) throws ParseException {
>> - String href = getAttribute(node, name, null);
>> - if (href == null)
>> + String href = null;
>> + if ("codebase".equals(name)) {
>
> Hmm, strange that the JNLP parser does not have an enum of or a /list/ of valid JNLP element and
> attribute name constants.
I have intorduced nconstant for codebase String. Abd yes.. The parser is one fo the oldest untouched
parts of original netx:( Terrible terribly old code :)
>
>> + href = getCleanAttribute(node, name);
>> + if (href != null) {// so that code can throw an exceptionlater
>> + //some bogus jnlps have codebase as "" and expect it behaving as "."
>> + if (href.trim().isEmpty()) {
>
> Try "href != null && href.trim().isEmpty()". ;-) It can be even further optimized into one statement.
Ouch. yup. Fixed
>
>> + href = ".";
>> + }
>> + }
>> + } else {
>> + href = getAttribute(node, name, null);
>> + }
>> + if (href == null) {
>> return null; // so that code can throw an exception if attribute was required
>> -
>> + }
>> try {
>> if (base == null)
>> return new URL(href);
>> @@ -1254,11 +1265,17 @@
>> public String getAttribute(Node node, String name, String defaultValue) {
>> // SAX
>> // String result = ((Element) node).getAttribute(name);
>> + String result = getCleanAttribute(node, name);
>> +
>> + if (result == null || result.length() == 0) {
>> + return defaultValue;
>
> I am not sure this is correct at all. What if an attribute was intended to accept an empty string?
> The default value actually depends on the particular attribute's semantics. As I said before, a
> missing attribute equates to null and thus always to a default value. An empty string value equates
> either to a default value or /the/ empty string value, depending on the attribute's semantics.
>
Unluckily it is correct.It is keeping "before patch" behaviour. It is not clear from patch, but is
is only refactoring - extracting of method to avoid duplication
before:
ublic String getAttribute(Node node, String name, String defaultValue) {
// SAX
// String result = ((Element) node).getAttribute(name);
String result = getCleanAttribute(node, name);
if (result == null || result.length() == 0) {
return defaultValue;
}
return result;
}
now:
public String getAttribute(Node node, String name, String defaultValue) {
// SAX
// String result = ((Element) node).getAttribute(name);
String result = getCleanAttribute(node, name);
if (result == null || result.length() == 0) {
return defaultValue;
}
return result;
}
private String getCleanAttribute(Node node, String name) {
String result = node.getAttribute(name);
return result;
}
So although your concerns below are valid, they are not valid in scope of this patch. And I'm afraid
I do not dare to change the handling of attribute. Many parts depends on null return from here, and
NPE later (tried :( )
>> + }
>> +
>> + return result;
>> + }
>> +
>> + public String getCleanAttribute(Node node, String name) {
>
> Err, what is a clean attribute? Did it wash before? :-D So, please check your naming. Besides,
> documentation is missing on a public method. And, judging by what it seems to be doing maybe it
> should be even private.
deffinitely! Made private.
>
>> String result = node.getAttribute(name);
>> -
>> - if (result == null || result.length() == 0)
>> - return defaultValue;
>> -
>> return result;
>> }
>>
>> diff -r d8e057783109 netx/net/sourceforge/jnlp/security/SecurityDialogs.java
>> --- a/netx/net/sourceforge/jnlp/security/SecurityDialogs.java Mon Sep 15 14:09:30 2014 -0400
>> +++ b/netx/net/sourceforge/jnlp/security/SecurityDialogs.java Tue Sep 16 15:25:44 2014 +0200
>> @@ -44,6 +44,8 @@
>> import java.net.URL;
>> import java.security.AccessController;
>> import java.security.PrivilegedAction;
>> +import java.util.Arrays;
>> +import java.util.Comparator;
>> import java.util.Set;
>> import java.util.concurrent.Semaphore;
>>
>> @@ -285,7 +287,13 @@
>>
>> SecurityDialogMessage message = new SecurityDialogMessage();
>> message.dialogType = DialogType.MISSING_ALACA;
>> - message.extras = new Object[]{title, codeBase.toString(),
>> UrlUtils.setOfUrlsToHtmlList(remoteUrls)};
>> + String urlToShow = "unknown url";
>> + if (codeBase != null) {
>> + urlToShow = codeBase.toString();
>> + } else {
>> + OutputController.getLogger().log("Warning, null codebase wants to show in ALACA!");
>> + }
>
> Formatting.
should be fixed
>
>> + message.extras = new Object[]{title, urlToShow, UrlUtils.setOfUrlsToHtmlList(remoteUrls)};
>> Object selectedValue = getUserResponse(message);
>> return getIntegerResponseAsBoolean(selectedValue);
>> }
>> diff -r d8e057783109 tests/netx/unit/net/sourceforge/jnlp/ParserTest.java
>> --- a/tests/netx/unit/net/sourceforge/jnlp/ParserTest.java Mon Sep 15 14:09:30 2014 -0400
>> +++ b/tests/netx/unit/net/sourceforge/jnlp/ParserTest.java Tue Sep 16 15:25:44 2014 +0200
>> @@ -1413,4 +1413,30 @@
>>
>> Assert.assertEquals(overwrittenCodebase.toExternalForm(),
>> parser.getCodeBase().toExternalForm());
>> }
>> +
>> +
>> + @Test
>> + public void testEmptyCodebase() throws Exception {
>> + String data = "<?xml version=\"1.0\"?>\n"
>> + + "<jnlp spec=\"1.5+\"\n"
>> + + "codebase=\"\" aaa=\"\" "
>> + + ">\n"
>> + + "</jnlp>";
>
> Although this is technically not wrong, it could probably go into a .jnlp file, just for clarity and
> maintainability.
>
Uf sorry. I do not understand what you would like me to do So this remained same.
>> +
>> + Node root = Parser.getRootNode(new ByteArrayInputStream(data.getBytes()), defaultParser);
>> + Assert.assertEquals("Root name is not jnlp", "jnlp", root.getNodeName());
>> + MockJNLPFile file = new MockJNLPFile(LANG_LOCALE);
>> + Parser parser = new Parser(file, null, root, defaultParser, null);
>> + ParseException eex = null;
>> + //non codebase element is unaffected
>> + URL u = parser.getURL(root, "aaa", null);
>> + Assert.assertEquals(null, u);
>> + try {
>> + parser.getURL(root, "codebase", null);
>> + } catch (ParseException ex) {
>> + eex = ex;
>> + }
>> + Assert.assertEquals(true, eex != null);
>> + Assert.assertEquals(true, eex instanceof ParseException);
>> + }
>> }
TRhank you fr review!
J.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: emptyCodebaseFixAndalacaNPEworkarround2.patch
Type: text/x-patch
Size: 4742 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140917/3dd36adb/emptyCodebaseFixAndalacaNPEworkarround2.patch>
More information about the distro-pkg-dev
mailing list