Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1514093002:
Fix memory leaks involving InsertIndirectObject() (Closed)
|
Created:
5 years ago by jun_fang Modified:
4 years, 10 months ago CC:
pdfium-reviews_googlegroups.com Base URL:
https://pdfium.googlesource.com/pdfium.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionFix memory leaks involving InsertIndirectObject()
BUG=447331
R=tsepez@chromium.org
Committed: https://pdfium.googlesource.com/pdfium/+/4ea4459ecaf66043f0a8b29f60439a0056ff6036
Patch Set 1 #Patch Set 2 : #
Total comments: 11
Patch Set 3 : #
Total comments: 7
Patch Set 4 : #Patch Set 5 : Rebase #
Messages
Total messages: 16 (3 generated)
jun_fang@foxitsoftware.com changed reviewers: + kai_jing@foxitsoftware.com, thestig@chromium.org, tsepez@chromium.org
Hi Tom and Lei, Please help to review it. Thanks! https://codereview.chromium.org/1514093002/diff/20001/core/src/fpdfapi/fpdf_p... File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp (right): https://codereview.chromium.org/1514093002/diff/20001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:1167: if (pObj->GetGenNum() <= pValue->GetGenNum()) { The current object is older than the one in m_IndirectObjs. Don't insert but release it. https://codereview.chromium.org/1514093002/diff/20001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:1171: pValue->Destroy(); We need to destroy the older one before inserting the newer version of object.
Just a few nit to fix. Thanks. https://codereview.chromium.org/1514093002/diff/20001/core/include/fpdfapi/fp... File core/include/fpdfapi/fpdf_objects.h (right): https://codereview.chromium.org/1514093002/diff/20001/core/include/fpdfapi/fp... core/include/fpdfapi/fpdf_objects.h:612: FX_BOOL InsertIndirectObject(FX_DWORD objnum, CPDF_Object* pObj); nit: add a comment // Takes ownership of |pObj|. https://codereview.chromium.org/1514093002/diff/20001/core/src/fpdfapi/fpdf_p... File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp (right): https://codereview.chromium.org/1514093002/diff/20001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:1167: if (pObj->GetGenNum() <= pValue->GetGenNum()) { On 2015/12/10 16:02:40, jun_fang wrote: > The current object is older than the one in m_IndirectObjs. Don't insert but > release it. nit: You mean younger? It might be clearer if pValue were name pExistingObj or somthing. Then pObj->GetGenNum() <= pExistingObj->GetGenNum() clearly reads as "pObj younger". https://codereview.chromium.org/1514093002/diff/20001/core/src/fpdfapi/fpdf_p... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): https://codereview.chromium.org/1514093002/diff/20001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:993: if ((!pDict || pDict->GetObjNum() != pObject->m_ObjNum)) nit: overparenthesized. nit: use { here. The rule is that if the else has braces, then so does the if, even if it would be a single-line if otherwise and wouldn't get braces. https://codereview.chromium.org/1514093002/diff/20001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:998: bInserted = FALSE; nit: bInserted is already false.
Isn't there one more InsertIndirectObject() caller in core/src/fpdfapi/fpdf_parser/fpdf_parser_fdf.cpp that need to check the return value?
https://codereview.chromium.org/1514093002/diff/20001/core/src/fpdfapi/fpdf_p... File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp (right): https://codereview.chromium.org/1514093002/diff/20001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:1167: if (pObj->GetGenNum() <= pValue->GetGenNum()) { On 2015/12/10 17:32:13, Tom Sepez wrote: > On 2015/12/10 16:02:40, jun_fang wrote: > > The current object is older than the one in m_IndirectObjs. Don't insert but > > release it. > nit: You mean younger? It might be clearer if pValue were name pExistingObj or > somthing. Then pObj->GetGenNum() <= pExistingObj->GetGenNum() clearly reads as > "pObj younger". I think Jun is saying old vs new, whereas you are thinking of young vs old.
On 2015/12/10 18:57:05, Lei Zhang wrote: > https://codereview.chromium.org/1514093002/diff/20001/core/src/fpdfapi/fpdf_p... > File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp (right): > > https://codereview.chromium.org/1514093002/diff/20001/core/src/fpdfapi/fpdf_p... > core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:1167: if (pObj->GetGenNum() > <= pValue->GetGenNum()) { > On 2015/12/10 17:32:13, Tom Sepez wrote: > > On 2015/12/10 16:02:40, jun_fang wrote: > > > The current object is older than the one in m_IndirectObjs. Don't insert but > > > release it. > > nit: You mean younger? It might be clearer if pValue were name pExistingObj > or > > somthing. Then pObj->GetGenNum() <= pExistingObj->GetGenNum() clearly reads as > > "pObj younger". > > I think Jun is saying old vs new, whereas you are thinking of young vs old. Yeah, ok, but I can't help but map "generation" => "age" in my mind.
On 2015/12/10 20:28:00, Tom Sepez wrote: > On 2015/12/10 18:57:05, Lei Zhang wrote: > > > https://codereview.chromium.org/1514093002/diff/20001/core/src/fpdfapi/fpdf_p... > > File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp (right): > > > > > https://codereview.chromium.org/1514093002/diff/20001/core/src/fpdfapi/fpdf_p... > > core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:1167: if > (pObj->GetGenNum() > > <= pValue->GetGenNum()) { > > On 2015/12/10 17:32:13, Tom Sepez wrote: > > > On 2015/12/10 16:02:40, jun_fang wrote: > > > > The current object is older than the one in m_IndirectObjs. Don't insert > but > > > > release it. > > > nit: You mean younger? It might be clearer if pValue were name pExistingObj > > or > > > somthing. Then pObj->GetGenNum() <= pExistingObj->GetGenNum() clearly reads > as > > > "pObj younger". > > > > I think Jun is saying old vs new, whereas you are thinking of young vs old. > > Yeah, ok, but I can't help but map "generation" => "age" in my mind. Below is the description about generation number from pdf spec: In a newly created file, all indirect objects shall have generation numbers of 0. Nonzero generation numbers may be introduced when the file is later updated;
https://codereview.chromium.org/1514093002/diff/20001/core/include/fpdfapi/fp... File core/include/fpdfapi/fpdf_objects.h (right): https://codereview.chromium.org/1514093002/diff/20001/core/include/fpdfapi/fp... core/include/fpdfapi/fpdf_objects.h:612: FX_BOOL InsertIndirectObject(FX_DWORD objnum, CPDF_Object* pObj); On 2015/12/10 17:32:12, Tom Sepez wrote: > nit: add a comment > // Takes ownership of |pObj|. Acknowledged. https://codereview.chromium.org/1514093002/diff/20001/core/src/fpdfapi/fpdf_p... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): https://codereview.chromium.org/1514093002/diff/20001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:993: if ((!pDict || pDict->GetObjNum() != pObject->m_ObjNum)) On 2015/12/10 17:32:13, Tom Sepez wrote: > nit: overparenthesized. > nit: use { here. The rule is that if the else has braces, then so does the if, > even if it would be a single-line if otherwise and wouldn't get braces. Acknowledged. https://codereview.chromium.org/1514093002/diff/20001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:998: bInserted = FALSE; On 2015/12/10 17:32:13, Tom Sepez wrote: > nit: bInserted is already false. Acknowledged.
https://codereview.chromium.org/1514093002/diff/20001/core/src/fpdfapi/fpdf_p... File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp (right): https://codereview.chromium.org/1514093002/diff/20001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:1167: if (pObj->GetGenNum() <= pValue->GetGenNum()) { On 2015/12/10 17:32:13, Tom Sepez wrote: > On 2015/12/10 16:02:40, jun_fang wrote: > > The current object is older than the one in m_IndirectObjs. Don't insert but > > release it. > nit: You mean younger? It might be clearer if pValue were name pExistingObj or > somthing. Then pObj->GetGenNum() <= pExistingObj->GetGenNum() clearly reads as > "pObj younger". A non-negative integer generation number. In a newly created file, all indirect objects shall have generation numbers of 0. Nonzero generation numbers may be introduced when the file is later updated;
On 2015/12/10 23:55:58, jun_fang wrote: > https://codereview.chromium.org/1514093002/diff/20001/core/src/fpdfapi/fpdf_p... > File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp (right): > > https://codereview.chromium.org/1514093002/diff/20001/core/src/fpdfapi/fpdf_p... > core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:1167: if (pObj->GetGenNum() > <= pValue->GetGenNum()) { > On 2015/12/10 17:32:13, Tom Sepez wrote: > > On 2015/12/10 16:02:40, jun_fang wrote: > > > The current object is older than the one in m_IndirectObjs. Don't insert but > > > release it. > > nit: You mean younger? It might be clearer if pValue were name pExistingObj > or > > somthing. Then pObj->GetGenNum() <= pExistingObj->GetGenNum() clearly reads as > > "pObj younger". > > A non-negative integer generation number. In a newly created file, > all indirect objects shall have generation numbers of 0. Nonzero > generation numbers may be introduced when the file is later updated; Please review patch 3.
https://codereview.chromium.org/1514093002/diff/40001/core/src/fpdfapi/fpdf_p... File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp (right): https://codereview.chromium.org/1514093002/diff/40001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:1163: void* pExistingObj = nullptr; I'd have kept value here, ... https://codereview.chromium.org/1514093002/diff/40001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:1165: if (pExistingObj) { and declared pExistingObj here with the static cast ... https://codereview.chromium.org/1514093002/diff/40001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:1171: static_cast<CPDF_Object*>(pExistingObj)->Destroy(); which avoids having to write static_cast twice. https://codereview.chromium.org/1514093002/diff/40001/core/src/fpdfapi/fpdf_p... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): https://codereview.chromium.org/1514093002/diff/40001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:993: if ((!pDict || pDict->GetObjNum() != pObject->m_ObjNum)) { still overparenthesized.
https://codereview.chromium.org/1514093002/diff/40001/core/src/fpdfapi/fpdf_p... File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp (right): https://codereview.chromium.org/1514093002/diff/40001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:1163: void* pExistingObj = nullptr; On 2015/12/11 00:15:07, Tom Sepez wrote: > I'd have kept value here, ... Acknowledged. https://codereview.chromium.org/1514093002/diff/40001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:1165: if (pExistingObj) { On 2015/12/11 00:15:07, Tom Sepez wrote: > and declared pExistingObj here with the static cast ... Acknowledged. https://codereview.chromium.org/1514093002/diff/40001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:1171: static_cast<CPDF_Object*>(pExistingObj)->Destroy(); On 2015/12/11 00:15:07, Tom Sepez wrote: > which avoids having to write static_cast twice. Acknowledged.
lgtm
Description was changed from ========== Fix memory leaks involving InsertIndirectObject() BUG=447331 ========== to ========== Fix memory leaks involving InsertIndirectObject() BUG=447331 R=tsepez@chromium.org Committed: https://pdfium.googlesource.com/pdfium/+/4ea4459ecaf66043f0a8b29f60439a0056ff... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as 4ea4459ecaf66043f0a8b29f60439a0056ff6036 (presubmit successful).
Message was sent while issue was closed.
Patchset #6 (id:100001) has been deleted |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
