[Poppler-bugs] [Bug 16770] support for digital signatures

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=16770

--- Comment #78 from Adrian Johnson <ajohnson at redneon.com> ---
(In reply to Adam Reichold from comment #77)
> Some minor suggestions:
> 
> * The naming of BLOCK_SIZE and block_len in hashSignedDataBlock seems
> misleading to me, maybe CHUNK_SIZE and block_len?
> 
> * The method hashSignedDataBlock could probably be replaced by a static
> function taking the stream and the handler? This should give the compiler
> more optimization possibilities than if it is visible in other translation
> units.
> 
> * I think the while loop within could become a for loop for better
> readability with the case reduced to computing the number of bytes to read
> instead of two separate calls to doGetChars and updateHash.

I don't think there is much benefit in making that function a static. It is not
something that is called a frequently from an inner loop.

I think the function would read better as "hashSignedByteRange" as "byte range"
is the terminology used in the PDF reference section in signatures. Instead of
setting the stream offset before calling this function it would better easier
to understand the code if the function took an offset and length.

I agree that the two calls to doGetChars and updateHash should be merged but I
don't think a for loop is the best way to process loops where the increment is
not exactly the same on each iteration. Maybe something like this:

  void FormFieldSignature::hashSignedByteRange(SignatureHandler *handler,
     Goffset start, Goffset len)
  {
    const int CHUNK_SIZE = 4096;
    unsigned char buffer[CHUNK_SIZE];
    Goffset i = 0;
    int byte_count = CHUNK_SIZE;

    doc->getBaseStream()->setPos(start);
    while (i < len)
    {
      if (i + CHUNK_SIZE > len)
        byte_count = len - i;

      doc->getBaseStream()->doGetChars(byte_count, buffer);
      handler->updateHash(buffer, byte_count);
      i += byte_count;
    }
  }

I renamed the "signed_data_buffer" to "buffer" as whenever I see "signed" in
C/C++ I think of the integer type. "unsigned char signed_data_buffer" is
confusing to read.

I would prefer the buffer be moved out to the class. It is better not to
allocate large buffers on the stack. We may later increase the buffer size.

I don't mind if we fix all this later. It doesn't have to hold up the initial
release.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/poppler-bugs/attachments/20151008/2a2c9e61/attachment.html>