RFR: 8153213: Jar manifest attribute "Multi-Release" accepts any value
Claes Redestad
claes.redestad at oracle.com
Sun Apr 10 04:36:03 UTC 2016
Hi Steve,
checking that there's a newline after the true seems OK according to the
grammar[1], so that seems like a good call.
Not sure it really matters, but perhaps setting isMultiRelease to true
once the test passes improves readability:
if (MULTI_RELEASE_ENABLED && version != BASE_VERSION) {
int i = match(MULTIRELEASE_CHARS, b,
MULTIRELEASE_LASTOCC);
if (i != -1) {
i += MULTIRELEASE_CHARS.length;
if (i < b.length) {
byte c = b[i];
if (c == '\r' || c == '\n') {
isMultiRelease = true;
}
}
}
}
For completeness, maybe we should also check that the value is not part
of a continuation:
// Check that the value is followed by a
// newline and does not have a continuation
if ((c == '\r' || c == '\n') &&
(i + 1 == b.length || b[i + 1] != '
')) {
isMultiRelease = true;
}
Thanks!
/Claes
[1]
http://docs.oracle.com/javase/8/docs/technotes/guides/jar/jar.html#Name-Value_pairs_and_Sections
On 2016-04-08 21:23, Steve Drach wrote:
> The new webrev is
> http://cr.openjdk.java.net/~sdrach/8153213/webrev/index.html
> <http://cr.openjdk.java.net/%7Esdrach/8153213/webrev/index.html>
> and the issue is https://bugs.openjdk.java.net/browse/JDK-8153213
>
> Now the value “true” followed by either ‘\r’ or ‘\n’ is the only
> acceptable value.
>
>> On Apr 8, 2016, at 11:34 AM, Steve Drach <steve.drach at oracle.com
>> <mailto:steve.drach at oracle.com>> wrote:
>>
>> Okay, I’ll prepare a new webrev. I think all we need to check for
>> after “true” is \r or \n. If the manifest just ends without at least
>> one of those, it’s not a legal manifest.
>>
>>> On Apr 8, 2016, at 11:25 AM, Claes Redestad
>>> <claes.redestad at oracle.com <mailto:claes.redestad at oracle.com>> wrote:
>>>
>>> On 04/08/2016 07:54 PM, Steve Drach wrote:
>>>>>>>> I’ve put up a new webrev that addresses the issue of having
>>>>>>>> spaces before (and after) the value “true” in the Multi-Release
>>>>>>>> attribute.
>>>>>>>>
>>>>>>> Is some or all of that really necessary? since the we can
>>>>>>> specify domain of values.
>>>>>> I think it is. The spec states that one can have an arbitrary
>>>>>> amount of leading/trailing spaces in the value part of the
>>>>>> attribute. Apparently attribute values could also have “hidden”
>>>>>> characters such as vertical tab or nak for example. I wish the
>>>>>> spec was tighter here. I’m merely stating the the domain can be
>>>>>> “ *TRUE *” for upper/lower case letters.
>>>>>>
>>>>> AFAICT the so called “spec" says nothing about trimming white
>>>>> space from values and although not explicitly called out the
>>>>> actual value has to be what constitutes the character sequence for
>>>>> the “otherchar" definition (otherwise the continuation space and
>>>>> newline characters would also be part of the actual value and that
>>>>> makes no sense).
>>>> No it doesn’t say you can trim white space, but if one doesn’t,
>>>> then do we accept “true”, “ true”, “ true”, etc.?
>>>>
>>>>> So it seems you may have quite a bit of wiggle room here :-) and
>>>>> it’s up to each attribute definition to state what constitutes its
>>>>> domain of possible valid values based on “other char”.
>>>> So, we can say *otherchar is upper/lower case “true” only?
>>>>
>>>>>
>>>>>>> For example, the Sealed attribute can take on a value of “true”
>>>>>>> or “false” (case ignored), but there is no white space trimming.
>>>>>> Well then “Sealed: true” won’t work, but the spec says it
>>>>>> should work.
>>>>>>
>>>>> Where does the “spec” state that it should?
>>>> It really comes down to the interpretation of otherchar. If it can
>>>> be interpreted on an attribute by attribute basis then it’s not a
>>>> problem.
>>>>
>>>>> The value is literally the string “ true”, at least in one
>>>>> interpretation of the “spec” :-)
>>>> I’m fine with that. And we really should do something about the
>>>> spec, it’s too loose and ambiguous.
>>>>
>>>>> I suspect it’s ok to go with finding “Multi-Release: true”
>>>>> (lower/upper case) as in your first patch, then check if it is
>>>>> terminated with a newline (and ignore the continuation case)
>>>> Okay. I’d like Claes to weigh in because he’s the one who brought
>>>> it up. He’s traveling today, so I don’t expect to hear from him soon.
>>>
>>> Yeah, I'm guilty of raising the silly question of whether or not
>>> accepting untrimmed true is OK according to spec.
>>>
>>> With a generous interpretation and the presence of prior art
>>> (Sealed) I think it's OK to go back to the first version of the
>>> patch (with the addition to check that Multi-Release: true is
>>> followed by a LF, CR or the end of the manifest) and add a similar
>>> note to the spec/notes that Multi-Release has the same value
>>> restrictions as Sealed.
>>>
>>> Perhaps both cases should be clarified in the notes to say that
>>> leading/trailing whitespace is not accepted, since that isn't
>>> directly obvious to a casual reader.
>>>
>>> Thanks!
>>>
>>> /Claes
>>>
>>>>
>>>>> Paul.
>>>>>
>>>>>
>>>>>> I am fine without doing this nonsense, but I think we need to
>>>>>> change the spec to make it correct. We could change the
>>>>>> definition of "otherchar” to something like this
>>>>>>
>>>>>> otherchar:=ALPHA EXTALPHANUM*
>>>>>> ALPHA:=[A-Z | a-z | _]
>>>>>> EXTALPHANUM:=[ALPHA | 0-9 | SPACE]
>>>>>>
>>>>>> Even this will still allow trailing spaces, so after matching
>>>>>> TRUE we’d need to make sure the next char is SPACE, \r, or \n.
>>>>>>
>>>>>> Other ideas?
>>>>>>
>>>>>>> Paul.
>>>>>>>
>>>>>>>>> Hi,
>>>>>>>>> Please review this simple fix to require that the jar manifest
>>>>>>>>> Multi-Release attribute has a value of “true" in order to be
>>>>>>>>> effective, i.e. to assert the jar is a multi-release jar.
>>>>>>>>> issue: https://bugs.openjdk.java.net/browse/JDK-8153213
>>>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8153213>
>>>>>>>>> webrev:
>>>>>>>>> http://cr.openjdk.java.net/~sdrach/8153213/webrev/index.html
>>>>>>>>> <http://cr.openjdk.java.net/%7Esdrach/8153213/webrev/index.html>
>>>>>>>>> <http://cr.openjdk.java.net/~sdrach/8153213/webrev/index.html
>>>>>>>>> <http://cr.openjdk.java.net/%7Esdrach/8153213/webrev/index.html>>
>>>>>>>>> Thanks
>>>>>>>>> Steve
>>
>
More information about the core-libs-dev
mailing list