The new webrev is http://cr.openjdk.java.net/~sdrach/8153213/webrev/index.html <http://cr.openjdk.java.net/~sdrach/8153213/webrev/index.html> and the issue is https://bugs.openjdk.java.net/browse/JDK-8153213 <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.dr...@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.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 >
