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.dr...@oracle.com
<mailto: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 <mailto: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/%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
