poppler/Catalog.cc | 79 ++++++++++++----------------------------------------- poppler/Catalog.h | 6 +--- 2 files changed, 21 insertions(+), 64 deletions(-)
New commits: commit bcfa08d007596876bff20dcac0ca84fee69fe56d Author: Adam Reichold <[email protected]> Date: Sat Sep 1 12:35:20 2018 +0200 Fix memory leak in Catalog by tracking pages (and page refs) using std::vector and std::unique_ptr instead of manually allocating them. oss-fuzz/10119 diff --git a/poppler/Catalog.cc b/poppler/Catalog.cc index d9edf6b1..4ded496b 100644 --- a/poppler/Catalog.cc +++ b/poppler/Catalog.cc @@ -76,10 +76,7 @@ Catalog::Catalog(PDFDoc *docA) { ok = gTrue; doc = docA; xref = doc->getXRef(); - pages = nullptr; - pageRefs = nullptr; numPages = -1; - pagesSize = 0; baseURI = nullptr; pageLabelInfo = nullptr; form = nullptr; @@ -96,7 +93,6 @@ Catalog::Catalog(PDFDoc *docA) { pagesRefList = nullptr; attrsList = nullptr; kidsIdxList = nullptr; - lastCachedPage = 0; markInfo = markInfoNull; Object catDict = xref->getCatalog(); @@ -145,15 +141,6 @@ Catalog::~Catalog() { } delete pagesRefList; delete pagesList; - if (pages) { - for (int i = 0; i < pagesSize; ++i) { - if (pages[i]) { - delete pages[i]; - } - } - gfree(pages); - } - gfree(pageRefs); delete destNameTree; delete embeddedFileNameTree; delete jsNameTree; @@ -198,13 +185,13 @@ Page *Catalog::getPage(int i) if (i < 1) return nullptr; catalogLocker(); - if (i > lastCachedPage) { + if (std::size_t(i) > pages.size()) { GBool cached = cachePageTree(i); if ( cached == gFalse) { return nullptr; } } - return pages[i-1]; + return pages[i-1].first.get(); } Ref *Catalog::getPageRef(int i) @@ -212,13 +199,13 @@ Ref *Catalog::getPageRef(int i) if (i < 1) return nullptr; catalogLocker(); - if (i > lastCachedPage) { + if (std::size_t(i) > pages.size()) { GBool cached = cachePageTree(i); if ( cached == gFalse) { return nullptr; } } - return &pageRefs[i-1]; + return &pages[i-1].second; } GBool Catalog::cachePageTree(int page) @@ -252,20 +239,7 @@ GBool Catalog::cachePageTree(int page) return gFalse; } - pagesSize = getNumPages(); - pages = (Page **)gmallocn_checkoverflow(pagesSize, sizeof(Page *)); - pageRefs = (Ref *)gmallocn_checkoverflow(pagesSize, sizeof(Ref)); - if (pages == nullptr || pageRefs == nullptr ) { - error(errSyntaxError, -1, "Cannot allocate page cache"); - pagesSize = 0; - return gFalse; - } - for (int i = 0; i < pagesSize; ++i) { - pages[i] = nullptr; - pageRefs[i].num = -1; - pageRefs[i].gen = -1; - } - + pages.clear(); attrsList = new std::vector<PageAttrs *>(); attrsList->push_back(new PageAttrs(nullptr, obj.getDict())); pagesList = new std::vector<Object>(); @@ -274,21 +248,19 @@ GBool Catalog::cachePageTree(int page) pagesRefList->push_back(pagesRef); kidsIdxList = new std::vector<int>(); kidsIdxList->push_back(0); - lastCachedPage = 0; - } while(1) { - if (page <= lastCachedPage) return gTrue; + if (std::size_t(page) <= pages.size()) return gTrue; if (pagesList->empty()) return gFalse; Object pagesDict = pagesList->back().copy(); Object kids = pagesDict.dictLookup("Kids"); if (!kids.isArray()) { - error(errSyntaxError, -1, "Kids object (page {0:d}) is wrong type ({1:s})", - lastCachedPage+1, kids.getTypeName()); + error(errSyntaxError, -1, "Kids object (page {0:uld}) is wrong type ({1:s})", + pages.size()+1, kids.getTypeName()); return gFalse; } @@ -305,8 +277,8 @@ GBool Catalog::cachePageTree(int page) Object kidRef = kids.arrayGetNF(kidsIdx); if (!kidRef.isRef()) { - error(errSyntaxError, -1, "Kid object (page {0:d}) is not an indirect reference ({1:s})", - lastCachedPage+1, kidRef.getTypeName()); + error(errSyntaxError, -1, "Kid object (page {0:uld}) is not an indirect reference ({1:s})", + pages.size()+1, kidRef.getTypeName()); return gFalse; } @@ -326,25 +298,20 @@ GBool Catalog::cachePageTree(int page) Object kid = kids.arrayGet(kidsIdx); if (kid.isDict("Page") || (kid.isDict() && !kid.getDict()->hasKey("Kids"))) { PageAttrs *attrs = new PageAttrs(attrsList->back(), kid.getDict()); - Page *p = new Page(doc, lastCachedPage+1, &kid, - kidRef.getRef(), attrs, form); + auto p = std::make_unique<Page>(doc, pages.size()+1, &kid, + kidRef.getRef(), attrs, form); if (!p->isOk()) { - error(errSyntaxError, -1, "Failed to create page (page {0:d})", lastCachedPage+1); - delete p; + error(errSyntaxError, -1, "Failed to create page (page {0:uld})", pages.size()+1); return gFalse; } - if (lastCachedPage >= numPages) { + if (pages.size() >= std::size_t(numPages)) { error(errSyntaxError, -1, "Page count in top-level pages object is incorrect"); - delete p; return gFalse; } - pages[lastCachedPage] = p; - pageRefs[lastCachedPage].num = kidRef.getRefNum(); - pageRefs[lastCachedPage].gen = kidRef.getRefGen(); + pages.emplace_back(std::move(p), kidRef.getRef()); - lastCachedPage++; kidsIdxList->back()++; // This should really be isDict("Pages"), but I've seen at least one @@ -355,8 +322,8 @@ GBool Catalog::cachePageTree(int page) pagesList->push_back(std::move(kid)); kidsIdxList->push_back(0); } else { - error(errSyntaxError, -1, "Kid object (page {0:d}) is wrong type ({1:s})", - lastCachedPage+1, kid.getTypeName()); + error(errSyntaxError, -1, "Kid object (page {0:uld}) is wrong type ({1:s})", + pages.size()+1, kid.getTypeName()); kidsIdxList->back()++; } } @@ -777,20 +744,12 @@ int Catalog::getNumPages() Dict *pageDict = pagesDict.getDict(); if (pageRootRef.isRef()) { const Ref pageRef = pageRootRef.getRef(); - Page *p = new Page(doc, 1, &pagesDict, pageRef, new PageAttrs(nullptr, pageDict), form); + auto p = std::make_unique<Page>(doc, 1, &pagesDict, pageRef, new PageAttrs(nullptr, pageDict), form); if (p->isOk()) { - pages = (Page **)gmallocn(1, sizeof(Page *)); - pageRefs = (Ref *)gmallocn(1, sizeof(Ref)); - - pages[0] = p; - pageRefs[0].num = pageRef.num; - pageRefs[0].gen = pageRef.gen; + pages.emplace_back(std::move(p), pageRef); numPages = 1; - lastCachedPage = 1; - pagesSize = 1; } else { - delete p; numPages = 0; } } else { diff --git a/poppler/Catalog.h b/poppler/Catalog.h index ed058f31..253b53f0 100644 --- a/poppler/Catalog.h +++ b/poppler/Catalog.h @@ -45,6 +45,7 @@ #include "Object.h" #include <vector> +#include <memory> class PDFDoc; class XRef; @@ -253,9 +254,7 @@ private: PDFDoc *doc; XRef *xref; // the xref table for this PDF file - Page **pages; // array of pages - Ref *pageRefs; // object ID for each page - int lastCachedPage; + std::vector<std::pair<std::unique_ptr<Page>, Ref>> pages; std::vector<Object> *pagesList; std::vector<Ref> *pagesRefList; std::vector<PageAttrs *> *attrsList; @@ -263,7 +262,6 @@ private: Form *form; ViewerPreferences *viewerPrefs; int numPages; // number of pages - int pagesSize; // size of pages array Object dests; // named destination dictionary Object names; // named names dictionary NameTree *destNameTree; // named destination name-tree _______________________________________________ poppler mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/poppler
