Casey Marshall wrote:
Marco Trudel wrote:
<snip>
Back to the patch. Comments? Hints?
Your approach seems OK, but a hand-coded parser may still be faster (if
only done once for the whole manifest, and if it uses a buffered stream,
instead of reading a byte at a time) than using regexps for it.
I don't think implementing a hand-coded parser is the right thing to do
just because regexp is slow. This would need a lot of error-prone code
and actually, regexp is here to do exactly parsing stuff like that. So
instead of avoiding regexp because it's slow, it should be optimized
(IMHO)...
I'd also
be wary of byte[]->String->byte[] conversions, because you are wrapping
each entry+hash in a String, then using String.getBytes() (what if the
default character set is not what you expect?). This byte representation
must match what's in the file exactly, because it is used to verify the
entry.
You did a String->byte[] conversion:
byte[] target = ("Name: " + entry.getName()).getBytes();
So only the byte[]->String conversion is new and should be checked
(assuming you checked your String->byte[] conversion). From the
ByteArrayOutputStream class:
toString()
Converts the buffer's contents into a string, translating bytes
into characters according to the platform's default character encoding.
I'm no expert in different charsets. But for me, it looks like it's
doing the same as the String.getBytes() method. Thus I think this is ok.
Or shall we verify the complete conversion part? As I said, I'm not an
expert in default character encodings. So there might be a problem (that
was already there before my patch).
It may make sense instead to change the Manifest class to retain the
byte-level representation of each entry, along with the hash value,
instead of re-parsing the manifest like this -- having two parsers for
the same thing is always trouble.
That would really be the best solution. But I'm not interested in
reimplementing the whole signing part. I just wanted to add support for
entries with line breaks and run into the speed problem.
I'm not sure why I didn't try using the existing Manifest class when I
wrote the signed Jar stuff originally.
I don't know neither ;-)
Marco
