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.redes...@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/~sdrach/8153213/webrev/index.html> >>>>>>> Thanks >>>>>>> Steve
