Marco Trudel wrote:
> Casey Marshall wrote:
>> Marco Trudel wrote:
>>> 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)...
>>>
>>
>> Sure. But you said yourself that you still thought the code was too
>> slow, and I worry a little that your code may be fragile.
>
> I think it's too slow because of the crypto stuff. Parsing the manifest
That's possible. We did a lot to optimize the symmetric crypto, but not
the asymmetric stuff (that is, digital signatures). They rely on
BigInteger, the Java version of which that's in Classpath has not been
optimized, AFAIK.
Again, Raif has a patch that uses the native GMP library, which is a lot
faster.
> once isn't that much overhead. I still have problem that my code might
> be fragile, I see no more fragilness than we had before. Maybe you can
> explain that a little bit more in detail (see below).
>
It's not more or less fragile, I'm just pointing out that it is (erm,
might be).
>> I do think it is a better approach than what we have now, though, so I
>> think it should go in.
>
> I will try to take a look at the mainfest class next week. Maybe I can
> spare a couple of hours and do it right. So please do not yet commit it.
>
OK. We can check this in now, though, regardless (beauty of CVS, etc.).
I think I have a fix for the "enumeration" issue (JarEntry aren't
reusable) too.
>> No. The bytes used in the signature verification use the toByteArray
>> method of ByteArrayOutputStream. The code may mistakenly not match the
>> Name: line because it uses String.getBytes().
>
> Yes, you create a byte[] by calling byteArrayOutputStream.toByteArray().
> But later, you create the "Name: " entry this way:
> byte[] target = ("Name: " + entry.getName()).getBytes();
>
> So you rely on the default byte representation as well. That's the only
> reason why I dared to do that too. Or do I miss something here?
>
No, I wasn't saying the original code was correct. My point was that the
original code doesn't introduce this conversion step for the bytes that
are fed into the signature verifier, just for matching the Name: line;
this would probably still produce false negatives just as often. And
anyway, why repeat my mistakes? ;-)
Also, I think the bikeshed should be painted Hunter Green.
