Hello F. E., hello all, > On 15 April 2019 at 09:54 "F. E." <exler7...@gmail.com> wrote: > > Hello again, > > I couldn't help myself and created yet another patch ^^.
thank you for the patch. I've tested with GCC 4.8 and clang 3.8 on one (GNU/Linux) system and with GCC 7.3 and clang 7.0 on another that it fixes the memory misuse (heap use-after-free) in the one method named in the subject. Therefore (and because it avoids an unnecessary, as now shown, object copy compared to your first patch) I've accepted it, meaning I committed it to PoDoFo svn r1980 [1]. > Apart from comments, this patch does only change a single line: > > It moves the 'pObj->GetArray().erase( it );' call out of the while > loop to the end of the function, to keep 'ref' valid until it can be > safely erased from the array. This approach is possible, since the while > loop is canceled anyway in this case, so the iterator remains unchanged > and can be used for erase at the end. We could also remove the bFound > bool and perform its check via 'it == pObj->GetArray().end()', but I left I understand that, and in the interest of a minimal change I also left it out. What I haven't tested is whether the annotation object really is deleted from the document, only that it's no longer found by podofopdfinfo (in addition to the above, of course): I didn't want to delay the commit further (and then, I considered that somewhat secondary to correcting the memory issue). > that out of the patch. Maybe my comments are a bit redundant and could be > shortened. I lightly typo-corrected them but other left them in as I regard them as informative in each of their locations. > > Greetings F.E. > Best regards, mabri [1] https://sourceforge.net/p/podofo/code/1980/ > Am Di., 9. Apr. 2019 um 08:09 Uhr schrieb F. E. < exler7...@gmail.com>: > > Hello again, > > > IIRC the object wasn't deleted to allow it to be reused, but I'm not sure > > > if that > > > was really correct (the docs aren't clear about this). Unlikely. IMO the > > > object was clearly meant to be removed, but since the used PdfReference > > > is destroyed in the process, > > that never applied. See the last two lines of the function, it is even > > stated in the comment:> // delete the PdfObject in the file > > > delete this->GetObject()->GetOwner()->RemoveObject( ref ); > > > Conclusion: Please wait for my acceptance of your first patch with some > > > cosmetic > > > and comment changes (I first need to test it), I reject v2 and v3 > > > herewith. All right. I understand the reasoning behind rejecting v2 and > > > v3. I would like to see v2 being implemented for v0.9.7, > > but I fear the info will be forgotten until then ^^. > > > > Greetings,F.E. > > > > Am Sa., 6. Apr. 2019 um 20:47 Uhr schrieb Matthew Brincke < > > ma...@mailbox.org>: > > > Hello F.E., hello all, > > > > On 03 April 2019 at 12:11 "F. E." < exler7...@gmail.com> wrote: > > > > > > > > Hello again, > > > > I created two alternative patches for this issue, since the first one > > > > feels > > > > rather ugly. With patch v2, I switched the PdfReference parameter to > > > > call-by-value instead of call-by-reference. > > > > > > I'd like to refrain from changing API before 0.9.7 unless it's really > > > necessary > > > to fix a bug, here it isn't (see your first patch, which I'm going to > > > accept > > > after some testing going OK, from reading it it's fine). > > > > With patch v3, I kept the parameter as it is, but passed a local copy > > > > of the > > > > required PdfReference to the function. > > > > > > That looks clean (and the comment I'd like to put into the first one) but > > > there's > > > one problem: The method called is public so all users would need to > > > change their > > > calls to fix the problem, so that's clearly impractical (+ docs'd need to > > > change). > > > > > > > With either of the three patches, the annotations are deleted without > > > > leftover > > > > objects, as it should be. > > > > > > IIRC the object wasn't deleted to allow it to be reused, but I'm not sure > > > if that > > > was really correct (the docs aren't clear about this). > > > > > > Conclusion: Please wait for my acceptance of your first patch with some > > > cosmetic > > > and comment changes (I first need to test it), I reject v2 and v3 > > > herewith. > > > > > > > Greetings, F.E. > > > > > > > > > > Best regards, mabri > > > > > > > > > _______________________________________________ > > > Podofo-users mailing list > > > Podofo-users@lists.sourceforge.net > > > https://lists.sourceforge.net/lists/listinfo/podofo-users _______________________________________________ Podofo-users mailing list Podofo-users@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/podofo-users
