Module file parse API

[Chris Hegarty]

On 20/06/2012 19:07, Sean Mullan wrote:
> On 06/20/2012 12:49 PM, Chris Hegarty wrote:
>> On 20/06/2012 17:28, Sean Mullan wrote:
>>> Hi Chris,
>>>
>>> I've just been reviewing the changes to Signer.java. With the new code,
>>> it looks like the caller is now responsible for verifying the hashes
>>> whereas in the old code, the Reader would automatically take care of it
>>> for you. Can you explain why you changed that?
>>
>> The parser is just that, a parser. It generates the hashes of section
>> data and exposes the section headers hashes, giving flexibility to the
>> caller on how to handle them ( you may decide to have a non validating
>> implementation! ). The previous way of doing this was just a hack that
>> preventing the Reader from actually writing any data, 'extract = false',
>> when the reader is actually an installer.
>
> Should we rename it to Installer then?

I think it should be renamed, but I'd prefer to do that in a separate 
commit/review. It can be difficult to review code that has been 
refactored and then moved to a new file. My intent here was to keep the 
change focused on the parser itself, and push further cleanup in 
subsequent changesets.

>> Now that Signer can invoke the parser directly rather than creating a
>> Reader/Installer we can actually simplify the Reader/Installer so that
>> it always extracts the content. I found this to be a more flexible and
>> cleaner solution.
>>
>> Sorry if I've missed the point of your question. Was this what you were
>> asking?
>
> Signer is an unusual case in that it is reading the module file without
> extracting its contents in order to calculate the hashes that it will
> then sign.

Right, but it is a very good use-case. The new parser API makes the code 
cleaner and more readable.

> I think my concern is that if the hashes inside the module file don't
> actually match what is being read, you would want to know about that
> regardless of whether you are extracting the contents.

Right, but this API is positioned at a low level. It felt very 
restricting to force this requirement at this level. I agree that most 
tools will want to fail if the hashes don't match, but I didn't want to 
preclude the possibility of building a non validating parser. It seems 
overly restrictive for the API to specify this.

> Having said that, I've been somewhat dubious on the overall
> value/purpose of the hashes inside the module file. They don't provide
> any security without something additional such as a signature, but when
> generating the signature, it has to recalculate all of the hashes again
> to be sure they are still correct since the module file was created. So
> their only value is as a checksum, but my experience with checksums is
> that they are usually stored separately from what they are computed over.

I view the hashes in an unsigned module file as a checksum also, but I 
think they are very useful. For example, if a tool is only interested in 
the classes of a module file, it can skip to the classes section, 
extract/process it, validate the hash, and exit without having to finish 
reading the remainder of the module file. This is nice, especially if 
the module file is being read from a remote stream, and only possible 
with per section hashes.

-Chris.

>
> --Sean