RFR: 8153213: Jar manifest attribute "Multi-Release" accepts any value
steve.drach at oracle.com
Fri Apr 8 18:34:59 UTC 2016
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> 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.
>>>> 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?
>>>>>>> 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/~sdrach/8153213/webrev/index.html>
More information about the core-libs-dev