Expose the core code from https://bugs.freedesktop.org/show_bug.cgi?id=16770 for the qt frontends
Created attachment 128276 [details] [review] Implement digital signature support (qt5 frontend) Let me know what needs to be polished/refactored/whatever. Qt5 forms test prints signature type now. ana poppler-9999 # ../poppler-9999_build/utils/pdfsig sample-signed.pdf Digital Signature Info of: sample-signed.pdf Syntax Error (0): Invalid or missing Signature string Signature #1: - Signer Certificate Common Name: (null) - Signing Time: Nov 23 2011 18:09:42 - Signature Validation: Signature has not yet been verified. ana poppler-9999 # ../poppler-9999_build/qt5/tests/poppler-qt5-forms sample-signed.pdf Forms for file sample-signed.pdf Page 1 Form Type: Signature Rect: top: 0.653595 left: -0.010101 width: 0.163399 height: 0.126263 ID: 65536 Name: Signature1 FullyQualifiedName: Signature1 UIName: ReadOnly: 0 Visible: 1 "Error (0): Invalid or missing Signature string" SignatureStatus: NotVerifiedYet CertificateStatus: NotVerifiedYet SignerName: SigningTime: 1322068182
Actually, I have two questions: - An empty signer name should be replaced by '(null)' or '(empty)' or such? - How do you transform time_t to a proper date/time with Qt5? Well, I guess easily fixable.
(In reply to Hanno Meyer-Thurow from comment #2) > Actually, I have two questions: > > - An empty signer name should be replaced by '(null)' or '(empty)' or such? Not on the library level, that's something that the app has to decided imho > - How do you transform time_t to a proper date/time with Qt5? QDateTime QDateTime::fromTime_t ( uint seconds ) [static] > Well, I guess easily fixable. I would prefer if you mimiced the API of the core where validate is const and returns a class with the validation result instead of modifying the FormFieldSignature itself.
Created attachment 128280 [details] [review] Implement digital signature support (qt5 frontend) - v2 Changes: - added helper class for signature validation info - moved status enums to signature validation info class, to where they can be queried - signature validation info instance can be created on stack like this - do not expose SignatureInfo core class in qt5/src/poppler-forms.h, therefore no passing of SignatureInfo instance to signature validation info instance.
Almost there, ValidateOptions should be an enum of FormFieldSignature since it's only used there. And SignatureValidationInfo should be use a d pointer for making keeping ABI compatibility easier, do you know how to implement that?
(In reply to Albert Astals Cid from comment #5) > Almost there, ValidateOptions should be an enum of FormFieldSignature since > it's only used there. O ha, that one I have overseen. > And SignatureValidationInfo should be use a d pointer for making keeping ABI > compatibility easier, do you know how to implement that? SignatureValidationInfo instance be on heap and return a pointer for keeping ABI compatibility, sure do. On a second thought, that makes sense. If there is more to comment, please do. Let's see if I have time tonight.
There is one thing I just cannot wrap my head around/thinking to much: Variant I: Do you want FormFieldSignature to return a SignatureValidationInfo instance pointer, that the user has to delete? Variant 2: Or should the FormFieldSignature instance manage the SignatureValidationInfo instance, where the user gets a const instance pointer (get and forget)? If the SignatureValidationInfo class ever changes, application linked to old ABI or mixing compiler ABI(name mangling), I question the proper destruction[0][1]. Therefore I recommend variant II; expecting same c++ name mangling; keep it simple. How do you want it? [0] http://stackoverflow.com/questions/232926/how-to-make-consistent-dll-binaries-across-vs-versions#232959 [1] http://stackoverflow.com/questions/331045/using-c-dlls-with-different-compiler-versions#331088
O well, variant II would mean to sacrifice constness. So, its variant I unless I do not see the correct way. :-)
Created attachment 128332 [details] [review] Implement digital signature support (qt5 frontend) - v3 Changes: - moved enum ValidateOptions from SignatureValidationInfo to FormFieldSignature - FormFieldSignature::validate(): return pointer to SignatureValidationInfo instance created on heap
+ private: + SignatureStatus signature_status; + CertificateStatus certificate_status; + + QString signer_name; + time_t signing_time; This is still bad, you need to have just a d-pointer as private member in there and then that pointer containing the rest of members. I'd prefer if it didn't return a pointer you have to delete but just an object, people will make less leaks that way.
(In reply to Albert Astals Cid from comment #10) > + private: > + SignatureStatus signature_status; > + CertificateStatus certificate_status; > + > + QString signer_name; > + time_t signing_time; > > This is still bad, you need to have just a d-pointer as private member in > there and then that pointer containing the rest of members. Whatever a d-pointer is. To simplify, SignatureValidationInfo class may just receive the pointer to core SignatureInfo instance. Query and return the info from core SignatureInfo. Would that be okay?
(In reply to Hanno Meyer-Thurow from comment #11) > (In reply to Albert Astals Cid from comment #10) > > + private: > > + SignatureStatus signature_status; > > + CertificateStatus certificate_status; > > + > > + QString signer_name; > > + time_t signing_time; > > > > This is still bad, you need to have just a d-pointer as private member in > > there and then that pointer containing the rest of members. > > Whatever a d-pointer is. google for it, it's a quite common name/construct, that's why i asked if you knew what it was a few comments ago. > To simplify, SignatureValidationInfo class may just > receive the pointer to core SignatureInfo instance. Query and return the > info from core SignatureInfo. > > Would that be okay? You can store the SignatureInfo in the d-pointer if you want, we still want a d-pointer to make it easier to maintain binary compatibility.
Created attachment 128362 [details] [review] Implement digital signature support (qt5 frontend) - v4 Changes: - add d-pointer Sadly, I cannot test right now, other than that it compiles fine. I updated my box and now libnss3 crashes trying to validate. O well ...
You need to implemement the copy/assignment operators of SignatureValidationInfo or make it not copiable/assignable, otherwise bad stuff happens since the default implementations just do a = on the d_ptr, so when you destroy it, there's a double delete on the d_ptr. As proof, just add Poppler::SignatureValidationInfo svi2 = svi; in poppler-forms.cpp after the svi assignment and then running it with valgrind (on a file that actually has signatures) and you'll get stuff like ==4234== Invalid read of size 8 ==4234== at 0x4E85599: load (atomic_base.h:396) ==4234== by 0x4E85599: load<int> (qatomic_cxx11.h:96) ==4234== by 0x4E85599: load (qbasicatomic.h:116) ==4234== by 0x4E85599: deref (qrefcount.h:60) ==4234== by 0x4E85599: ~QString (qstring.h:1065) ==4234== by 0x4E85599: ~SignatureValidationInfoPrivate (poppler-form.cc:420) ==4234== by 0x4E85599: Poppler::SignatureValidationInfo::~SignatureValidationInfo() (poppler-form.cc:436) ==4234== by 0x10B18D: main (poppler-forms.cpp:189) ==4234== Address 0xf053f88 is 8 bytes inside a block of size 24 free'd ==4234== at 0x4C2E26B: operator delete(void*) (vg_replace_malloc.c:576) ==4234== by 0x10B185: main (poppler-forms.cpp:190) ==4234== Block was alloc'd at ==4234== at 0x4C2D1AF: operator new(unsigned long) (vg_replace_malloc.c:334) ==4234== by 0x4E856F2: Poppler::FormFieldSignature::validate(Poppler::FormFieldSignature::ValidateOptions) const (poppler-form.cc:483) ==4234== by 0x10AF29: main (poppler-forms.cpp:189) ==4234== ==4234== Invalid free() / delete / delete[] / realloc() ==4234== at 0x4C2E26B: operator delete(void*) (vg_replace_malloc.c:576) ==4234== by 0x10B18D: main (poppler-forms.cpp:189) ==4234== Address 0xf053f80 is 0 bytes inside a block of size 24 free'd ==4234== at 0x4C2E26B: operator delete(void*) (vg_replace_malloc.c:576) ==4234== by 0x10B185: main (poppler-forms.cpp:190) ==4234== Block was alloc'd at ==4234== at 0x4C2D1AF: operator new(unsigned long) (vg_replace_malloc.c:334) ==4234== by 0x4E856F2: Poppler::FormFieldSignature::validate(Poppler::FormFieldSignature::ValidateOptions) const (poppler-form.cc:483) ==4234== by 0x10AF29: main (poppler-forms.cpp:189)
Created attachment 128366 [details] [review] Implement digital signature support (qt5 frontend) - v5 Changes: - SignatureValidationInfo: disable copy c'tor/assignment operator No need to proof basics. Obviously no Qt dev here and work in downtime is imperfect. ;-) p.s.: edited patch partially manually, I will test after my box rebuilt all packages after toolchain update to hopefully fix my libnss3 issue.
Created attachment 128386 [details] [review] Implement digital signature support (qt5 frontend) - v6 Changes: - utilize QScopedPointer as recommended by d-pointer Application has to pass scoped pointer into validate() to make clear that ownership of resource data is transfered; scoped pointer cannot be returned(copied) anyway(for its use-case). If you use this patch for qt4 with scoped pointer, it requires Qt 4.6.
Just pushed your code and a few modifications on my own over, hope you don't mind of the changes https://cgit.freedesktop.org/poppler/poppler/commit/?id=8bb90fc828a3400a2464a38f0ec9e592754197dd And sorry it took so much.
All fine, and to clarify my stupid sentence "Obviously no Qt dev here ...". I meant to say "Obviously I am no Qt dev ...". O well ... The changes are just fine. I was just reluctant to use shared versus scoped pointer. Scoped seems to fit better but disallows copy on return. One cosmetic change may be applied in qt5/tests/poppler-forms.cpp: - // QDateTime::fromTime_t(svi->signingTime(), Qt::UTC).toString(); + // QDateTime::fromTime_t(svi.signingTime(), Qt::UTC).toString();
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.