<html> <head> <base href="https://bugs.freedesktop.org/"> </head> <body> <div> <a class="bz_bug_link bz_status_NEW " title="NEW - Implement digital signature support (glib frontend)" href="https://bugs.freedesktop.org/show_bug.cgi?id=94376#c2">Comment # 2</a> on <a class="bz_bug_link bz_status_NEW " title="NEW - Implement digital signature support (glib frontend)" href="https://bugs.freedesktop.org/show_bug.cgi?id=94376">bug 94376</a> from <a class="email" href="mailto:carlosgc@gnome.org" title="Carlos Garcia Campos <carlosgc@gnome.org>"> Carlos Garcia Campos</a> <pre>Comment on <a href="attachment.cgi?id=124622" name="attach_124622" title="Expose signature verification in the glib frontend">attachment 124622</a> <a href="attachment.cgi?id=124622&action=edit" title="Expose signature verification in the glib frontend">[details]</a> <a href='page.cgi?id=splinter.html&bug=94376&attachment=124622'>[review]</a> Expose signature verification in the glib frontend Review of <a href="attachment.cgi?id=124622" name="attach_124622" title="Expose signature verification in the glib frontend">attachment 124622</a> <a href="attachment.cgi?id=124622&action=edit" title="Expose signature verification in the glib frontend">[details]</a> <a href='page.cgi?id=splinter.html&bug=94376&attachment=124622'>[review]</a>: ----------------------------------------------------------------- Thanks for the patch and sorry for the delay to review it. I haven't followed the core implementation, so I'm not sure how this API is expected to be used by applications. I guess the general idea is that the document might contain one or more signatures (defined as form fields), and applications can verify the document by validating every form field, right? ::: poppler-tmp/poppler-0.45.0/glib/poppler-document.cc @@ +1105,4 @@ > } > > /** > + * poppler_document_count_signatures: In glib API we normally use n_signatures instead of count @@ +1119,5 @@ > +poppler_document_count_signatures(PopplerDocument *document) > +{ > + g_return_val_if_fail (POPPLER_IS_DOCUMENT (document), 0); > + return document->doc->getSignatureWidgets().size(); > +} What's the point of providing the number of signatures, how is the user expected to use this? Just to know whether the document needs to be verified? @@ +1478,5 @@ > FALSE, > G_PARAM_READABLE)); > + /** > + * PopplerDocument::signed: > + * Whether the document is signed. This doesn't match the property definition. I'm not sure we need a property for this, though. ::: poppler-tmp/poppler-0.45.0/glib/poppler-form-field.cc @@ +703,5 @@ > + > + new_info = g_new0(PopplerSignatureInfo, 1); > + new_info->sig_status = sig_info->sig_status; > + new_info->cert_status = sig_info->cert_status; > + new_info->signer_name = sig_info->signer_name; This is owned by the other PopplerSignatureInfo, if you don't duplicate the string, freeing one structure would leave the other one with an invalid pointer. @@ +704,5 @@ > + new_info = g_new0(PopplerSignatureInfo, 1); > + new_info->sig_status = sig_info->sig_status; > + new_info->cert_status = sig_info->cert_status; > + new_info->signer_name = sig_info->signer_name; > + new_info->signing_time = sig_info->signing_time; You can do *new_info = *sig_info and then copy manually only the fields needed, the signer name in this case. @@ +712,5 @@ > + > +void > +poppler_signature_info_free (PopplerSignatureInfo *siginfo) > +{ > + g_free(siginfo); And here you should also free the signer name @@ +716,5 @@ > + g_free(siginfo); > +} > + > +PopplerSignatureInfo * > +poppler_form_field_signature_validate (PopplerFormField *sigField, gboolean doVerifyCert, gboolean forceRevalidation) This method also need to be documented. What do the parameters mean? I prefer to use flags oinstead of boolean parameters if possible. @@ +725,5 @@ > + FormFieldSignature * sig_field = static_cast<FormFieldSignature*>(sigField->widget->getField()); > + SignatureInfo * sig_info = sig_field->validateSignature(doVerifyCert, forceRevalidation); > + > + PopplerSignatureInfo * poppler_sig_info; > + poppler_sig_info = g_new0(PopplerSignatureInfo, 1); Maybe this could be an out parameter, so that it can be stack allocated by the caller. And the method could return the validation result. @@ +772,5 @@ > + case CERTIFICATE_GENERIC_ERROR: > + poppler_sig_info->cert_status = POPPLER_CERTIFICATE_GENERIC_ERROR; > + break; > + case CERTIFICATE_NOT_VERIFIED: > + poppler_sig_info->cert_status = POPPLER_CERTIFICATE_NOT_VERIFIED; So we are validating two things in the same function? Are really sig and cert status part of the signature information? @@ +776,5 @@ > + poppler_sig_info->cert_status = POPPLER_CERTIFICATE_NOT_VERIFIED; > + break; > + } > + > + poppler_sig_info->signer_name = sig_info->getSignerName(); The return value is owned by SignatureInfo, we should copy it, and SignatureInfo should return a const char* instead.</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are the assignee for the bug.</li> </ul> </body> </html>