Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2466023002:
Unify some code (Closed)
DescriptionUnify some code
Move parsing of linearized header into separate CPDF_Linearized class.
Committed: https://pdfium.googlesource.com/pdfium/+/71333dc57ac7e4cf7963c83333730b3882ab371f
Patch Set 1 #Patch Set 2 : Add missing changes #
Total comments: 9
Patch Set 3 : Fix review issues #
Total comments: 2
Patch Set 4 : Fix review issues. #
Total comments: 13
Patch Set 5 : Fix review issues. #Patch Set 6 : Fix compilation #
Total comments: 10
Patch Set 7 : Fix review issues. #Patch Set 8 : Fix review issues. #
Total comments: 7
Patch Set 9 : Fix review issues. #
Total comments: 10
Patch Set 10 : Fix review issues. #Patch Set 11 : Fix review issues. #
Total comments: 14
Patch Set 12 : Fix review issues. #
Total comments: 5
Patch Set 13 : Fix review issues. #Patch Set 14 : Rebase #
Messages
Total messages: 53 (20 generated)
art-snake@yandex-team.ru changed reviewers: + brucedawson@chromium.org, dsinclair@chromium.org, thestig@chromium.org
Description was changed from ========== Unify some code ========== to ========== Unify some code Move parsing of linearized header into separate CPDF_Linearized class. ==========
The CQ bit was checked by thestig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux on master.tryserver.client.pdfium (JOB_FAILED, https://build.chromium.org/p/tryserver.client.pdfium/builders/linux/builds/2613) linux_xfa_asan_lsan on master.tryserver.client.pdfium (JOB_FAILED, https://build.chromium.org/p/tryserver.client.pdfium/builders/linux_xfa_asan_...) mac_xfa on master.tryserver.client.pdfium (JOB_FAILED, https://build.chromium.org/p/tryserver.client.pdfium/builders/mac_xfa/builds/...)
Need to fix the build... https://codereview.chromium.org/2466023002/diff/20001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_linearized.cpp (right): https://codereview.chromium.org/2466023002/diff/20001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_linearized.cpp:9: #include "core/fpdfapi/parser/cpdf_dictionary.h" nit: alphabetical order https://codereview.chromium.org/2466023002/diff/20001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_linearized.cpp:20: return std::unique_ptr<CPDF_Linearized>(new CPDF_Linearized(pDict)); We have MakeUnique() in PDFium too. https://codereview.chromium.org/2466023002/diff/20001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_linearized.h (right): https://codereview.chromium.org/2466023002/diff/20001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_linearized.h:38: FX_FILESIZE m_szFileSize = 0; Given the values for these member variables come from CPDFObject::GetInteger(), shouldn't this be of type int? i.e. CPDF_HintTables::GetEndOfFirstPageOffset() and friends should not need to cast.
https://codereview.chromium.org/2466023002/diff/20001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_linearized.cpp (right): https://codereview.chromium.org/2466023002/diff/20001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_linearized.cpp:9: #include "core/fpdfapi/parser/cpdf_dictionary.h" On 2016/11/01 00:29:53, Lei Zhang wrote: > nit: alphabetical order Done. https://codereview.chromium.org/2466023002/diff/20001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_linearized.cpp:20: return std::unique_ptr<CPDF_Linearized>(new CPDF_Linearized(pDict)); On 2016/11/01 00:29:53, Lei Zhang wrote: > We have MakeUnique() in PDFium too. I can not use pdfium::MakeUnique in this case, because the constructor of CPDF_Linearized is private. https://codereview.chromium.org/2466023002/diff/20001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_linearized.h (right): https://codereview.chromium.org/2466023002/diff/20001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_linearized.h:38: FX_FILESIZE m_szFileSize = 0; On 2016/11/01 00:29:53, Lei Zhang wrote: > Given the values for these member variables come from CPDFObject::GetInteger(), > shouldn't this be of type int? i.e. CPDF_HintTables::GetEndOfFirstPageOffset() > and friends should not need to cast. Now, in some places its are Unsigned (size_t/uint32_t/..) in other places it is signed (int). For exaple see _uint32_t_ CPDF_Parser::m_dwFirstPageNo and _int_ CPDF_HintTables::GetFirstPageNumber I will unify this in next CLs. In this CL i just want to move Linearized header related code into separate class. And after that, fix types by tiny CLs, one by one.
The CQ bit was checked by thestig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2466023002/diff/20001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_linearized.cpp (right): https://codereview.chromium.org/2466023002/diff/20001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_linearized.cpp:20: return std::unique_ptr<CPDF_Linearized>(new CPDF_Linearized(pDict)); On 2016/11/01 14:39:24, snake wrote: > On 2016/11/01 00:29:53, Lei Zhang wrote: > > We have MakeUnique() in PDFium too. > > I can not use pdfium::MakeUnique in this case, because the constructor of > CPDF_Linearized is private. Yes, I'm being silly. https://codereview.chromium.org/2466023002/diff/20001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_linearized.h (right): https://codereview.chromium.org/2466023002/diff/20001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_linearized.h:38: FX_FILESIZE m_szFileSize = 0; On 2016/11/01 14:39:24, snake wrote: > On 2016/11/01 00:29:53, Lei Zhang wrote: > > Given the values for these member variables come from > CPDFObject::GetInteger(), > > shouldn't this be of type int? i.e. CPDF_HintTables::GetEndOfFirstPageOffset() > > and friends should not need to cast. > > Now, in some places its are Unsigned (size_t/uint32_t/..) in other places it is > signed (int). > For exaple see _uint32_t_ CPDF_Parser::m_dwFirstPageNo and _int_ > CPDF_HintTables::GetFirstPageNumber > > I will unify this in next CLs. > In this CL i just want to move Linearized header related code into separate > class. > And after that, fix types by tiny CLs, one by one. Yes, there are some inconsistencies, but my question is, what should CPDF_Linearized return? Given the numbers, as stored in the PDF are simply "integers", should CPDF_Linearized just return the raw values and let the callers take it for what it is? Or should it interpret the values and return those to the callers? https://codereview.chromium.org/2466023002/diff/40001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_dataavail_embeddertest.cpp (right): https://codereview.chromium.org/2466023002/diff/40001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_dataavail_embeddertest.cpp:10: class CPDFDataAvailEmbeddertest : public EmbedderTest {}; Did you actually want to add this empty test?
https://codereview.chromium.org/2466023002/diff/20001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_linearized.h (right): https://codereview.chromium.org/2466023002/diff/20001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_linearized.h:38: FX_FILESIZE m_szFileSize = 0; On 2016/11/01 22:51:29, Lei Zhang wrote: > On 2016/11/01 14:39:24, snake wrote: > > On 2016/11/01 00:29:53, Lei Zhang wrote: > > > Given the values for these member variables come from > > CPDFObject::GetInteger(), > > > shouldn't this be of type int? i.e. > CPDF_HintTables::GetEndOfFirstPageOffset() > > > and friends should not need to cast. > > > > Now, in some places its are Unsigned (size_t/uint32_t/..) in other places it > is > > signed (int). > > For exaple see _uint32_t_ CPDF_Parser::m_dwFirstPageNo and _int_ > > CPDF_HintTables::GetFirstPageNumber > > > > I will unify this in next CLs. > > In this CL i just want to move Linearized header related code into separate > > class. > > And after that, fix types by tiny CLs, one by one. > > Yes, there are some inconsistencies, but my question is, what should > CPDF_Linearized return? Given the numbers, as stored in the PDF are simply > "integers", should CPDF_Linearized just return the raw values and let the > callers take it for what it is? Or should it interpret the values and return > those to the callers? I think, it should interpret the values. CPDF_Linearized should hide internal representation of Raw data, and simplify access to it. https://codereview.chromium.org/2466023002/diff/40001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_dataavail_embeddertest.cpp (right): https://codereview.chromium.org/2466023002/diff/40001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_dataavail_embeddertest.cpp:10: class CPDFDataAvailEmbeddertest : public EmbedderTest {}; On 2016/11/01 22:51:29, Lei Zhang wrote: > Did you actually want to add this empty test? Was added by mistake. Removed.
Lei Zhang, Can you send email to committers@chromium.org for nominating me for try job access. I need this, for more fast checking comilation/ASAN/... errors, without waiting the reviewers. My email: art-snake@yandex-team.ru I'm from Yandex company. Based on: http://www.chromium.org/getting-involved/become-a-committer
The CQ bit was checked by thestig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2466023002/diff/60001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_data_avail.cpp (left): https://codereview.chromium.org/2466023002/diff/60001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_data_avail.cpp:871: m_bLinearized = TRUE; Is this variable still needed at all? This is the only place in the original code that sets it to true, but you have removed this code, and many other references to it. https://codereview.chromium.org/2466023002/diff/60001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_linearized.cpp (right): https://codereview.chromium.org/2466023002/diff/60001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_linearized.cpp:15: if (!pObj) You can write this as: UniqueDictionary pDict = ToDictionary(pObj); if (!pDict || !pDict->GetObjectFor("Linearized")) return nullptr; return pdfium::WrapUnique(new CPDF_Linearized(std::move(pDict))); https://codereview.chromium.org/2466023002/diff/60001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_linearized.cpp:16: return std::unique_ptr<CPDF_Linearized>(); In https://codereview.chromium.org/2453163002 we converted a bunch of these to just nullptr. Do the same here and save some typing? https://codereview.chromium.org/2466023002/diff/60001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_linearized.h (right): https://codereview.chromium.org/2466023002/diff/60001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_linearized.h:24: FX_FILESIZE GetFileSize() const { return m_szFileSize; } If CPDF_Linearized is going to interpret the values, then it needs to define what the valid ranges are for these values. It may be helpful to have a "bool IsValid() const" method, so callers can easily determine whether to even use the values contained within or not. https://codereview.chromium.org/2466023002/diff/60001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_parser.cpp (right): https://codereview.chromium.org/2466023002/diff/60001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_parser.cpp:1464: m_dwFirstPageNo = m_pLinearized->GetFirstPageNo(); You may want just get rid of |m_dwFirstPageNo| and replace the one place that reads it with a call to m_pLinearized->GetFirstPageNo().
https://codereview.chromium.org/2466023002/diff/60001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_data_avail.cpp (left): https://codereview.chromium.org/2466023002/diff/60001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_data_avail.cpp:871: m_bLinearized = TRUE; On 2016/11/02 21:34:22, Lei Zhang wrote: > Is this variable still needed at all? This is the only place in the original > code that sets it to true, but you have removed this code, and many other > references to it. Done. https://codereview.chromium.org/2466023002/diff/60001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_linearized.cpp (right): https://codereview.chromium.org/2466023002/diff/60001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_linearized.cpp:15: if (!pObj) On 2016/11/02 21:34:22, Lei Zhang wrote: > You can write this as: > > UniqueDictionary pDict = ToDictionary(pObj); > if (!pDict || !pDict->GetObjectFor("Linearized")) > return nullptr; > return pdfium::WrapUnique(new CPDF_Linearized(std::move(pDict))); Done. https://codereview.chromium.org/2466023002/diff/60001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_linearized.cpp:16: return std::unique_ptr<CPDF_Linearized>(); On 2016/11/02 21:34:22, Lei Zhang wrote: > In https://codereview.chromium.org/2453163002 we converted a bunch of these to > just nullptr. Do the same here and save some typing? Done. https://codereview.chromium.org/2466023002/diff/60001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_linearized.h (right): https://codereview.chromium.org/2466023002/diff/60001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_linearized.h:24: FX_FILESIZE GetFileSize() const { return m_szFileSize; } On 2016/11/02 21:34:22, Lei Zhang wrote: > If CPDF_Linearized is going to interpret the values, then it needs to define > what the valid ranges are for these values. It may be helpful to have a "bool > IsValid() const" method, so callers can easily determine whether to even use the > values contained within or not. Done. https://codereview.chromium.org/2466023002/diff/60001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_parser.cpp (right): https://codereview.chromium.org/2466023002/diff/60001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_parser.cpp:1464: m_dwFirstPageNo = m_pLinearized->GetFirstPageNo(); On 2016/11/02 21:34:22, Lei Zhang wrote: > You may want just get rid of |m_dwFirstPageNo| and replace the one place that > reads it with a call to m_pLinearized->GetFirstPageNo(). Done.
https://codereview.chromium.org/2466023002/diff/60001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_linearized.cpp (right): https://codereview.chromium.org/2466023002/diff/60001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_linearized.cpp:15: if (!pObj) On 2016/11/02 21:34:22, Lei Zhang wrote: > You can write this as: > > UniqueDictionary pDict = ToDictionary(pObj); > if (!pDict || !pDict->GetObjectFor("Linearized")) > return nullptr; > return pdfium::WrapUnique(new CPDF_Linearized(std::move(pDict))); Do not compile with "UniqueDictionary pDict = ToDictionary(pObj);"
Oh, conflicting CLs landed that replaced FX_BOOL, TRUE and FALSE with bool, true and false, respectively. https://codereview.chromium.org/2466023002/diff/60001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_linearized.cpp (right): https://codereview.chromium.org/2466023002/diff/60001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_linearized.cpp:15: if (!pObj) On 2016/11/02 22:35:16, snake wrote: > On 2016/11/02 21:34:22, Lei Zhang wrote: > > You can write this as: > > > > UniqueDictionary pDict = ToDictionary(pObj); > > if (!pDict || !pDict->GetObjectFor("Linearized")) > > return nullptr; > > return pdfium::WrapUnique(new CPDF_Linearized(std::move(pDict))); > Do not compile with > "UniqueDictionary pDict = ToDictionary(pObj);" Err, UniqueDictionary pDict = ToDictionary(std::move(pObj)); https://codereview.chromium.org/2466023002/diff/100001/core/fpdfapi/parser/cp... File core/fpdfapi/parser/cpdf_linearized.cpp (right): https://codereview.chromium.org/2466023002/diff/100001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_linearized.cpp:24: m_dwFirstPageNo = pDict->GetIntegerFor("P"); I'm still worried about some of these signed/unsigned conversations. For instance, if the "P" field contains -1, GetFirstPageNo() will happily return 4294967295. https://codereview.chromium.org/2466023002/diff/100001/core/fpdfapi/parser/cp... File core/fpdfapi/parser/cpdf_linearized.h (right): https://codereview.chromium.org/2466023002/diff/100001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_linearized.h:24: bool IsValid() const; Ok, so now that you have IsValid(), can we make it a private method? Then let CreateForObject() call it, and return a nullptr when the object is invalid? That way, CreateForObject() callers can safely assume the object they got back contains valid data. https://codereview.chromium.org/2466023002/diff/100001/core/fpdfapi/parser/cp... File core/fpdfapi/parser/cpdf_parser.cpp (right): https://codereview.chromium.org/2466023002/diff/100001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_parser.cpp:1463: auto pLinearized = CPDF_Linearized::CreateForObject( Do we want to directly assign to |m_pLinearized| here?
https://codereview.chromium.org/2466023002/diff/60001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_linearized.cpp (right): https://codereview.chromium.org/2466023002/diff/60001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_linearized.cpp:15: if (!pObj) On 2016/11/03 00:46:26, Lei Zhang wrote: > On 2016/11/02 22:35:16, snake wrote: > > On 2016/11/02 21:34:22, Lei Zhang wrote: > > > You can write this as: > > > > > > UniqueDictionary pDict = ToDictionary(pObj); > > > if (!pDict || !pDict->GetObjectFor("Linearized")) > > > return nullptr; > > > return pdfium::WrapUnique(new CPDF_Linearized(std::move(pDict))); > > Do not compile with > > "UniqueDictionary pDict = ToDictionary(pObj);" > > Err, UniqueDictionary pDict = ToDictionary(std::move(pObj)); Done. https://codereview.chromium.org/2466023002/diff/100001/core/fpdfapi/parser/cp... File core/fpdfapi/parser/cpdf_linearized.cpp (right): https://codereview.chromium.org/2466023002/diff/100001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_linearized.cpp:24: m_dwFirstPageNo = pDict->GetIntegerFor("P"); On 2016/11/03 00:46:26, Lei Zhang wrote: > I'm still worried about some of these signed/unsigned conversations. For > instance, if the "P" field contains -1, GetFirstPageNo() will happily return > 4294967295. If it contains "-1" , that pdf file is corruped. Also corrupted file can contains any bad values, which we can not detect. I think, we should not worry about this, because the pdf parser will return error, while parse it. https://codereview.chromium.org/2466023002/diff/100001/core/fpdfapi/parser/cp... File core/fpdfapi/parser/cpdf_linearized.h (right): https://codereview.chromium.org/2466023002/diff/100001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_linearized.h:24: bool IsValid() const; On 2016/11/03 00:46:26, Lei Zhang wrote: > Ok, so now that you have IsValid(), can we make it a private method? Then let > CreateForObject() call it, and return a nullptr when the object is invalid? That > way, CreateForObject() callers can safely assume the object they got back > contains valid data. Done. https://codereview.chromium.org/2466023002/diff/100001/core/fpdfapi/parser/cp... File core/fpdfapi/parser/cpdf_parser.cpp (right): https://codereview.chromium.org/2466023002/diff/100001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_parser.cpp:1463: auto pLinearized = CPDF_Linearized::CreateForObject( On 2016/11/03 00:46:26, Lei Zhang wrote: > Do we want to directly assign to |m_pLinearized| here? What do you mean? If we have not valid CPDF_Linearized we should not store it.
https://codereview.chromium.org/2466023002/diff/100001/core/fpdfapi/parser/cp... File core/fpdfapi/parser/cpdf_linearized.cpp (right): https://codereview.chromium.org/2466023002/diff/100001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_linearized.cpp:24: m_dwFirstPageNo = pDict->GetIntegerFor("P"); On 2016/11/03 01:39:27, snake wrote: > On 2016/11/03 00:46:26, Lei Zhang wrote: > > I'm still worried about some of these signed/unsigned conversations. For > > instance, if the "P" field contains -1, GetFirstPageNo() will happily return > > 4294967295. > If it contains "-1" , that pdf file is corruped. > Also corrupted file can contains any bad values, which we can not detect. > I think, we should not worry about this, because the pdf parser will return > error, while parse it. Oh, but I worry about this a lot. If you look at PDFium security bugs, [1] many of them contain intentionally badly made PDFs from security researchers. If you say not to worry, can you show me where the parser detects negative values for the N, P, and O keys? If we want this class to interpret the values, then it needs be more careful. [1] https://bugs.chromium.org/p/chromium/issues/list?can=1&q=component%3AInternal... https://codereview.chromium.org/2466023002/diff/100001/core/fpdfapi/parser/cp... File core/fpdfapi/parser/cpdf_parser.cpp (right): https://codereview.chromium.org/2466023002/diff/100001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_parser.cpp:1463: auto pLinearized = CPDF_Linearized::CreateForObject( On 2016/11/03 01:39:27, snake wrote: > On 2016/11/03 00:46:26, Lei Zhang wrote: > > Do we want to directly assign to |m_pLinearized| here? > > What do you mean? > If we have not valid CPDF_Linearized we should not store it. But with the IsValid() check that we just added, CreateForObject() will either return something valid or nullptr. So if |m_pLinearized| starts out as nullptr, and you assign nullptr to it, there is no harm.
https://codereview.chromium.org/2466023002/diff/100001/core/fpdfapi/parser/cp... File core/fpdfapi/parser/cpdf_linearized.cpp (right): https://codereview.chromium.org/2466023002/diff/100001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_linearized.cpp:24: m_dwFirstPageNo = pDict->GetIntegerFor("P"); On 2016/11/03 01:59:31, Lei Zhang wrote: > On 2016/11/03 01:39:27, snake wrote: > > On 2016/11/03 00:46:26, Lei Zhang wrote: > > > I'm still worried about some of these signed/unsigned conversations. For > > > instance, if the "P" field contains -1, GetFirstPageNo() will happily return > > > 4294967295. > > If it contains "-1" , that pdf file is corruped. > > Also corrupted file can contains any bad values, which we can not detect. > > I think, we should not worry about this, because the pdf parser will return > > error, while parse it. > > Oh, but I worry about this a lot. If you look at PDFium security bugs, [1] many > of them contain intentionally badly made PDFs from security researchers. If you > say not to worry, can you show me where the parser detects negative values for > the N, P, and O keys? If we want this class to interpret the values, then it > needs be more careful. > > [1] > https://bugs.chromium.org/p/chromium/issues/list?can=1&q=component%3AInternal... "N" 1) It is already casted to unsigned. Check, that it valid https://cs.chromium.org/chromium/src/third_party/pdfium/core/fpdfapi/parser/c... "P" 1) https://cs.chromium.org/chromium/src/third_party/pdfium/core/fpdfapi/parser/c... 2) Check index before compare https://cs.chromium.org/chromium/src/third_party/pdfium/core/fpdfapi/parser/c... 3) Also check index before compare https://cs.chromium.org/chromium/src/third_party/pdfium/core/fpdfapi/parser/c... "O": https://cs.chromium.org/chromium/src/third_party/pdfium/core/fpdfapi/parser/c... Also m_FirstPageObjNum, this is just object ID, which should be exists in cross ref table. If it is negative and exists, we can parse it and use, if not we will have parse error. https://codereview.chromium.org/2466023002/diff/100001/core/fpdfapi/parser/cp... File core/fpdfapi/parser/cpdf_parser.cpp (right): https://codereview.chromium.org/2466023002/diff/100001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_parser.cpp:1463: auto pLinearized = CPDF_Linearized::CreateForObject( On 2016/11/03 01:59:31, Lei Zhang wrote: > On 2016/11/03 01:39:27, snake wrote: > > On 2016/11/03 00:46:26, Lei Zhang wrote: > > > Do we want to directly assign to |m_pLinearized| here? > > > > What do you mean? > > If we have not valid CPDF_Linearized we should not store it. > > But with the IsValid() check that we just added, CreateForObject() will either > return something valid or nullptr. So if |m_pLinearized| starts out as nullptr, > and you assign nullptr to it, there is no harm. Done.
I need to look at the callers some more. It is not your fault of course, but some of this code does silly things, e.g. CPDF_DataAvail::CheckFirstPage() is casting back and forth between signed and unsigned types. @_@ https://codereview.chromium.org/2466023002/diff/140001/core/fpdfapi/parser/cp... File core/fpdfapi/parser/cpdf_data_avail.cpp (right): https://codereview.chromium.org/2466023002/diff/140001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_data_avail.cpp:635: bNeedDownLoad = false; Accidental change? Now |bNeedDownload| is always false.
> I need to look at the callers some more. It is not your > fault of course, but > some of this code does silly things, e.g. > CPDF_DataAvail::CheckFirstPage() is > casting back and forth between signed and unsigned types. > @_@ I want to fix this in separate CLs, one by one. https://codereview.chromium.org/2466023002/diff/140001/core/fpdfapi/parser/cp... File core/fpdfapi/parser/cpdf_data_avail.cpp (right): https://codereview.chromium.org/2466023002/diff/140001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_data_avail.cpp:635: bNeedDownLoad = false; On 2016/11/03 15:38:41, Lei Zhang wrote: > Accidental change? Now |bNeedDownload| is always false. Was incorrect rebasing. FIxed
https://codereview.chromium.org/2466023002/diff/140001/core/fpdfapi/parser/cp... File core/fpdfapi/parser/cpdf_hint_tables.cpp (right): https://codereview.chromium.org/2466023002/diff/140001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_hint_tables.cpp:499: return static_cast<int>(m_pLinearized->GetFirstPageObjNum()); There's a subtle difference here: all the callers check for < 0 on failure, but now this never returns < 0. Unlike GetEndOfFirstPageOffset() above, CPDF_Linearized::IsValid() does not check this value. https://codereview.chromium.org/2466023002/diff/140001/core/fpdfapi/parser/cp... File core/fpdfapi/parser/cpdf_linearized.cpp (right): https://codereview.chromium.org/2466023002/diff/140001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_linearized.cpp:47: GetPageCount() > 0 && (GetPageCount() == 1 || GetLastXRefOffset() > 0); Why did you decide GetLastXRefOffset() can be 0 when the page count is 1?
https://codereview.chromium.org/2466023002/diff/140001/core/fpdfapi/parser/cp... File core/fpdfapi/parser/cpdf_hint_tables.cpp (right): https://codereview.chromium.org/2466023002/diff/140001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_hint_tables.cpp:499: return static_cast<int>(m_pLinearized->GetFirstPageObjNum()); On 2016/11/03 17:18:00, Lei Zhang wrote: > There's a subtle difference here: all the callers check for < 0 on failure, but > now this never returns < 0. > > Unlike GetEndOfFirstPageOffset() above, CPDF_Linearized::IsValid() does not > check this value. This checks not save us from errors value (say 9999999). If we have any incorrect first page num, we can not parse this page, in CPDF_Document::GetPage, because we have not this obj-num in ref table or this object have incorrect format. Again, I want to update checks like this, and simplify/unify in spearate CLs. As you see, much of checking in seperate places, with separate conditions. https://codereview.chromium.org/2466023002/diff/140001/core/fpdfapi/parser/cp... File core/fpdfapi/parser/cpdf_linearized.cpp (right): https://codereview.chromium.org/2466023002/diff/140001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_linearized.cpp:47: GetPageCount() > 0 && (GetPageCount() == 1 || GetLastXRefOffset() > 0); On 2016/11/03 17:18:00, Lei Zhang wrote: > Why did you decide GetLastXRefOffset() can be 0 when the page count is 1? LastXRefOffset is offset to MainXRefTable, but if linearized document contains only one page, that we can have only first page xref table. If we have main xref table, it will be empty, and not necessary to parse it.
What should i do, to commit this CL? May be i will remove the IsValid method, and back original checks on its places for now?
I think we can make a bit more progress on this CL and land it safely. I'm mostly happy with CPDF_Linearized. https://codereview.chromium.org/2466023002/diff/140001/core/fpdfapi/parser/cp... File core/fpdfapi/parser/cpdf_hint_tables.cpp (right): https://codereview.chromium.org/2466023002/diff/140001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_hint_tables.cpp:499: return static_cast<int>(m_pLinearized->GetFirstPageObjNum()); On 2016/11/03 17:50:34, snake wrote: > On 2016/11/03 17:18:00, Lei Zhang wrote: > > There's a subtle difference here: all the callers check for < 0 on failure, > but > > now this never returns < 0. > > > > Unlike GetEndOfFirstPageOffset() above, CPDF_Linearized::IsValid() does not > > check this value. > > This checks not save us from errors value (say 9999999). > If we have any incorrect first page num, we can not parse this page, in > CPDF_Document::GetPage, because we have not this obj-num in ref table or this > object have incorrect format. Sure, I know it will not save us from other bad values. Hopefully some other code is doing those checks. I guess what I'd like is for the 3 callers to check for <= 0, instead of < 0. > Again, I want to update checks like this, and simplify/unify in spearate CLs. As > you see, much of checking in seperate places, with separate conditions. Yes, I understand. I agree many improvements can be made in follow up Cls. Though I do want this CL to be able to stand on its own, so it does not cause a regression and get reverted. https://codereview.chromium.org/2466023002/diff/160001/core/fpdfapi/parser/cp... File core/fpdfapi/parser/cpdf_data_avail.cpp (right): https://codereview.chromium.org/2466023002/diff/160001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_data_avail.cpp:619: if (!m_pLinearized->GetFirstPageEndOffset() || GetFirstPageEndOffset() can't return 0 now. https://codereview.chromium.org/2466023002/diff/160001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_data_avail.cpp:620: !m_pLinearized->GetFileSize() || !m_pLinearized->GetLastXRefOffset()) { GetFileSize() can't return 0 either. https://codereview.chromium.org/2466023002/diff/160001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_data_avail.cpp:620: !m_pLinearized->GetFileSize() || !m_pLinearized->GetLastXRefOffset()) { The original code didn't care about the value of the LastXRefOffset - it just wanted to make sure the dictionary value exists. Should we try to stick with that? If yes, we may need to modify CPDF_Linearized to check and make sure all the required keys in the dict exist, bail out early if some of them are missing. Maybe set a m_bIsValid member and have IsValid() return that instead. https://codereview.chromium.org/2466023002/diff/160001/core/fpdfapi/parser/cp... File core/fpdfapi/parser/cpdf_linearized.h (right): https://codereview.chromium.org/2466023002/diff/160001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_linearized.h:24: FX_FILESIZE GetFileSize() const { return m_szFileSize; } Can you briefly document these getters, and mention things like GetFirstPageEndOffset() will only return values > 0. https://codereview.chromium.org/2466023002/diff/160001/core/fpdfapi/parser/cp... File core/fpdfapi/parser/cpdf_parser.cpp (right): https://codereview.chromium.org/2466023002/diff/160001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_parser.cpp:1491: m_pSyntax->GetNextWord(nullptr); This is the same call that used to be in CPDF_Parser::IsLinearizedFile(), right? I don't know if this is the right place to call GetNextWord(), or if it should remain in IsLinearizedFile().
https://codereview.chromium.org/2466023002/diff/160001/core/fpdfapi/parser/cp... File core/fpdfapi/parser/cpdf_data_avail.cpp (right): https://codereview.chromium.org/2466023002/diff/160001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_data_avail.cpp:619: if (!m_pLinearized->GetFirstPageEndOffset() || On 2016/11/04 01:10:15, Lei Zhang wrote: > GetFirstPageEndOffset() can't return 0 now. At zero offset we have PDF signature, it is have not be FirstPageEndOffset. https://codereview.chromium.org/2466023002/diff/160001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_data_avail.cpp:620: !m_pLinearized->GetFileSize() || !m_pLinearized->GetLastXRefOffset()) { On 2016/11/04 01:10:15, Lei Zhang wrote: > The original code didn't care about the value of the LastXRefOffset - it just > wanted to make sure the dictionary value exists. Should we try to stick with > that? > > If yes, we may need to modify CPDF_Linearized to check and make sure all the > required keys in the dict exist, bail out early if some of them are missing. > Maybe set a m_bIsValid member and have IsValid() return that instead. I have modified CPDF_Linearized. https://codereview.chromium.org/2466023002/diff/160001/core/fpdfapi/parser/cp... File core/fpdfapi/parser/cpdf_linearized.h (right): https://codereview.chromium.org/2466023002/diff/160001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_linearized.h:24: FX_FILESIZE GetFileSize() const { return m_szFileSize; } On 2016/11/04 01:10:15, Lei Zhang wrote: > Can you briefly document these getters, and mention things like > GetFirstPageEndOffset() will only return values > 0. Done. https://codereview.chromium.org/2466023002/diff/160001/core/fpdfapi/parser/cp... File core/fpdfapi/parser/cpdf_parser.cpp (right): https://codereview.chromium.org/2466023002/diff/160001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_parser.cpp:1491: m_pSyntax->GetNextWord(nullptr); On 2016/11/04 01:10:15, Lei Zhang wrote: > This is the same call that used to be in CPDF_Parser::IsLinearizedFile(), right? > > I don't know if this is the right place to call GetNextWord(), or if it should > remain in IsLinearizedFile(). Moved back for now. Will think how will be better.
https://codereview.chromium.org/2466023002/diff/160001/core/fpdfapi/parser/cp... File core/fpdfapi/parser/cpdf_parser.cpp (right): https://codereview.chromium.org/2466023002/diff/160001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_parser.cpp:1491: m_pSyntax->GetNextWord(nullptr); On 2016/11/04 22:39:14, snake wrote: > On 2016/11/04 01:10:15, Lei Zhang wrote: > > This is the same call that used to be in CPDF_Parser::IsLinearizedFile(), > right? > > > > I don't know if this is the right place to call GetNextWord(), or if it should > > remain in IsLinearizedFile(). > > Moved back for now. Will think how will be better. There's still a subtle difference. In the old code, the "Linearized" dictionary just has to exist. Now it has to parse correctly before we call GetNextWord(). I'm not sure if this matters. https://codereview.chromium.org/2466023002/diff/200001/core/fpdfapi/parser/cp... File core/fpdfapi/parser/cpdf_document_unittest.cpp (right): https://codereview.chromium.org/2466023002/diff/200001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_document_unittest.cpp:86: TestLinearized(CPDF_Dictionary* dict) : CPDF_Linearized(dict) {} add "explicit" https://codereview.chromium.org/2466023002/diff/200001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_document_unittest.cpp:130: UniqueDictionary dict(new CPDF_Dictionary()); pdfium::MakeUnique<CPDF_Dictionary>() https://codereview.chromium.org/2466023002/diff/200001/core/fpdfapi/parser/cp... File core/fpdfapi/parser/cpdf_linearized.cpp (right): https://codereview.chromium.org/2466023002/diff/200001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_linearized.cpp:20: bool must_be_exists = true) { must_exist https://codereview.chromium.org/2466023002/diff/200001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_linearized.cpp:21: const CPDF_Object* pNum = pDict->GetObjectFor(key); You can write: const CPDF_Number* pNum = ToNumber(pDict->GetObjectFor(key)); if (!pNum || !pNum->IsInteger()) return !must_exist; const int raw_value = pNum->GetInteger(); ... https://codereview.chromium.org/2466023002/diff/200001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_linearized.cpp:22: if (!pNum || !pNum->IsNumber()) Also check IsInteger() ? https://codereview.chromium.org/2466023002/diff/200001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_linearized.cpp:29: } // namespace nit: blank line above https://codereview.chromium.org/2466023002/diff/200001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_linearized.cpp:35: if (!pDict || !pDict->GetObjectFor("Linearized") || Use KeyExist() instead of GetObjectFor() since you don't care about the actual returned object.
https://codereview.chromium.org/2466023002/diff/200001/core/fpdfapi/parser/cp... File core/fpdfapi/parser/cpdf_document_unittest.cpp (right): https://codereview.chromium.org/2466023002/diff/200001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_document_unittest.cpp:86: TestLinearized(CPDF_Dictionary* dict) : CPDF_Linearized(dict) {} On 2016/11/04 23:02:15, Lei Zhang wrote: > add "explicit" Done. https://codereview.chromium.org/2466023002/diff/200001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_document_unittest.cpp:130: UniqueDictionary dict(new CPDF_Dictionary()); On 2016/11/04 23:02:15, Lei Zhang wrote: > pdfium::MakeUnique<CPDF_Dictionary>() CPDF_Dictionary should use custom deleter. https://codereview.chromium.org/2466023002/diff/200001/core/fpdfapi/parser/cp... File core/fpdfapi/parser/cpdf_linearized.cpp (right): https://codereview.chromium.org/2466023002/diff/200001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_linearized.cpp:20: bool must_be_exists = true) { On 2016/11/04 23:02:15, Lei Zhang wrote: > must_exist Done. https://codereview.chromium.org/2466023002/diff/200001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_linearized.cpp:21: const CPDF_Object* pNum = pDict->GetObjectFor(key); On 2016/11/04 23:02:15, Lei Zhang wrote: > You can write: > > const CPDF_Number* pNum = ToNumber(pDict->GetObjectFor(key)); > if (!pNum || !pNum->IsInteger()) > return !must_exist; > const int raw_value = pNum->GetInteger(); > ... Done. https://codereview.chromium.org/2466023002/diff/200001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_linearized.cpp:22: if (!pNum || !pNum->IsNumber()) On 2016/11/04 23:02:15, Lei Zhang wrote: > Also check IsInteger() ? Done. https://codereview.chromium.org/2466023002/diff/200001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_linearized.cpp:29: } // namespace On 2016/11/04 23:02:15, Lei Zhang wrote: > nit: blank line above Done. https://codereview.chromium.org/2466023002/diff/200001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_linearized.cpp:35: if (!pDict || !pDict->GetObjectFor("Linearized") || On 2016/11/04 23:02:15, Lei Zhang wrote: > Use KeyExist() instead of GetObjectFor() since you don't care about the actual > returned object. Done.
lgtm https://codereview.chromium.org/2466023002/diff/220001/core/fpdfapi/parser/cp... File core/fpdfapi/parser/cpdf_linearized.cpp (right): https://codereview.chromium.org/2466023002/diff/220001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_linearized.cpp:20: bool must_exists = true) { I was thinking about this some more. Why bother? "P" is optional anyway. Just drop this, don't bother validating "P" in CreateForObject(). Validate in the ctor instead, where if it's invalid, default to 0.
https://codereview.chromium.org/2466023002/diff/220001/core/fpdfapi/parser/cp... File core/fpdfapi/parser/cpdf_linearized.cpp (right): https://codereview.chromium.org/2466023002/diff/220001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_linearized.cpp:20: bool must_exists = true) { On 2016/11/04 23:38:18, Lei Zhang wrote: > I was thinking about this some more. Why bother? "P" is optional anyway. Just > drop this, don't bother validating "P" in CreateForObject(). Validate in the > ctor instead, where if it's invalid, default to 0. But if it is exists, and have incorrect format, that linearized header is icorrect, and we should not use it, is'n it?
https://codereview.chromium.org/2466023002/diff/220001/core/fpdfapi/parser/cp... File core/fpdfapi/parser/cpdf_linearized.cpp (right): https://codereview.chromium.org/2466023002/diff/220001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_linearized.cpp:20: bool must_exists = true) { On 2016/11/04 23:46:49, snake wrote: > On 2016/11/04 23:38:18, Lei Zhang wrote: > > I was thinking about this some more. Why bother? "P" is optional anyway. Just > > drop this, don't bother validating "P" in CreateForObject(). Validate in the > > ctor instead, where if it's invalid, default to 0. > > But if it is exists, and have incorrect format, that linearized header is > icorrect, and we should not use it, is'n it? That depends on how strict we want to be. So one possibility is to relax the check for "P" since it is optional. If it's invalid in any way, pretend it is the default value 0. Another possibility is to be as strict as possible. In that case, you would want to do the following, to check for optional existance vs correct type separately. if (!pDict->KeyExist(key)) return !must_exist; const CPDF_Number* pNum = ToNumber(pDict->GetObjectFor(key)); if (!pNum || !pNum->IsInteger()) return false;
https://codereview.chromium.org/2466023002/diff/220001/core/fpdfapi/parser/cp... File core/fpdfapi/parser/cpdf_linearized.cpp (right): https://codereview.chromium.org/2466023002/diff/220001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_linearized.cpp:20: bool must_exists = true) { On 2016/11/05 00:00:52, Lei Zhang wrote: > On 2016/11/04 23:46:49, snake wrote: > > On 2016/11/04 23:38:18, Lei Zhang wrote: > > > I was thinking about this some more. Why bother? "P" is optional anyway. > Just > > > drop this, don't bother validating "P" in CreateForObject(). Validate in the > > > ctor instead, where if it's invalid, default to 0. > > > > But if it is exists, and have incorrect format, that linearized header is > > icorrect, and we should not use it, is'n it? > > That depends on how strict we want to be. So one possibility is to relax the > check for "P" since it is optional. If it's invalid in any way, pretend it is > the default value 0. > > Another possibility is to be as strict as possible. In that case, you would want > to do the following, to check for optional existance vs correct type separately. > > if (!pDict->KeyExist(key)) > return !must_exist; > const CPDF_Number* pNum = ToNumber(pDict->GetObjectFor(key)); > if (!pNum || !pNum->IsInteger()) > return false; I think for linearized header we should be as strict as possible. Because we can use other way (default for non linearized) to parse.
Thanks for addressing all of my concerns. https://codereview.chromium.org/2466023002/diff/220001/core/fpdfapi/parser/cp... File core/fpdfapi/parser/cpdf_linearized.cpp (right): https://codereview.chromium.org/2466023002/diff/220001/core/fpdfapi/parser/cp... core/fpdfapi/parser/cpdf_linearized.cpp:20: bool must_exists = true) { On 2016/11/05 00:15:57, snake wrote: > On 2016/11/05 00:00:52, Lei Zhang wrote: > > On 2016/11/04 23:46:49, snake wrote: > > > On 2016/11/04 23:38:18, Lei Zhang wrote: > > > > I was thinking about this some more. Why bother? "P" is optional anyway. > > Just > > > > drop this, don't bother validating "P" in CreateForObject(). Validate in > the > > > > ctor instead, where if it's invalid, default to 0. > > > > > > But if it is exists, and have incorrect format, that linearized header is > > > icorrect, and we should not use it, is'n it? > > > > That depends on how strict we want to be. So one possibility is to relax the > > check for "P" since it is optional. If it's invalid in any way, pretend it is > > the default value 0. > > > > Another possibility is to be as strict as possible. In that case, you would > want > > to do the following, to check for optional existance vs correct type > separately. > > > > if (!pDict->KeyExist(key)) > > return !must_exist; > > const CPDF_Number* pNum = ToNumber(pDict->GetObjectFor(key)); > > if (!pNum || !pNum->IsInteger()) > > return false; > > I think for linearized header we should be as strict as possible. Because we can > use other way (default for non linearized) to parse. Sure.
The CQ bit was checked by art-snake@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2466023002/#ps240001 (title: "Fix review issues.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_no_v8 on master.tryserver.client.pdfium (JOB_FAILED, https://build.chromium.org/p/tryserver.client.pdfium/builders/linux_no_v8/bui...) mac on master.tryserver.client.pdfium (JOB_FAILED, https://build.chromium.org/p/tryserver.client.pdfium/builders/mac/builds/2667) mac_xfa on master.tryserver.client.pdfium (JOB_FAILED, https://build.chromium.org/p/tryserver.client.pdfium/builders/mac_xfa/builds/...)
Patch failed to apply probably because CPDF_Object and sub-classes now no longer need the ReleaseDeleter.
The CQ bit was checked by art-snake@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2466023002/#ps260001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Unify some code Move parsing of linearized header into separate CPDF_Linearized class. ========== to ========== Unify some code Move parsing of linearized header into separate CPDF_Linearized class. Committed: https://pdfium.googlesource.com/pdfium/+/71333dc57ac7e4cf7963c83333730b3882ab... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as https://pdfium.googlesource.com/pdfium/+/71333dc57ac7e4cf7963c83333730b3882ab...
Message was sent while issue was closed.
A revert of this CL (patchset #14 id:260001) has been created in https://codereview.chromium.org/2474283005/ by dsinclair@chromium.org. The reason for reverting is: Breaking the chrome roll. See https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium.... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
