Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2419553002:
Verify number of ex flags matches number of ex items. (Closed)
DescriptionVerify number of ex flags matches number of ex items.
Currently the JBig2 decoder can leak subimages in the case where we mark
more items in EXFLAGS then we have SDNUMEXSYMS. This Cl checks for this
condition and fails the decode if it happens.
BUG=chromium:654365
Committed: https://pdfium.googlesource.com/pdfium/+/6e5239c6e3891d78e7b9e8262c23cd129f0cdbb7
Patch Set 1 #
Total comments: 5
Patch Set 2 : Review feedback #Messages
Total messages: 17 (8 generated)
The CQ bit was checked by dsinclair@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...
dsinclair@chromium.org changed reviewers: + kcwu@chromium.org, tsepez@chromium.org
PTAL. This is a guess as to the right functionality. In the specific file given we have SDNUMEXSYMS = 0 but there are 11 items which we mark in EXINDEX. It seems wrong that we'd have more items flagged as EX then we have EXSYMS entries.
lgtm https://codereview.chromium.org/2419553002/diff/1/core/fxcodec/jbig2/JBig2_Sd... File core/fxcodec/jbig2/JBig2_SddProc.cpp (right): https://codereview.chromium.org/2419553002/diff/1/core/fxcodec/jbig2/JBig2_Sd... core/fxcodec/jbig2/JBig2_SddProc.cpp:252: if (EXINDEX > SDNUMEXSYMS) { This is fine, but generally when writing this "C-Style" error handling, I like to null out my pointers out top, then always delete them at |failed:|. Then you don't have to remember to do this before each |goto failed|
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2419553002/diff/1/core/fxcodec/jbig2/JBig2_Sd... File core/fxcodec/jbig2/JBig2_SddProc.cpp (right): https://codereview.chromium.org/2419553002/diff/1/core/fxcodec/jbig2/JBig2_Sd... core/fxcodec/jbig2/JBig2_SddProc.cpp:252: if (EXINDEX > SDNUMEXSYMS) { Since SDNUMEXSYMS is always zero, this check means rejecting EXINDEX > 0. I don't know what these variables are but I feel this seems too aggressive. I'd suggest do more tests if possible before commit.
On 2016/10/12 16:41:47, kcwu wrote: > https://codereview.chromium.org/2419553002/diff/1/core/fxcodec/jbig2/JBig2_Sd... > File core/fxcodec/jbig2/JBig2_SddProc.cpp (right): > > https://codereview.chromium.org/2419553002/diff/1/core/fxcodec/jbig2/JBig2_Sd... > core/fxcodec/jbig2/JBig2_SddProc.cpp:252: if (EXINDEX > SDNUMEXSYMS) { > Since SDNUMEXSYMS is always zero, this check means rejecting EXINDEX > 0. I > don't know what these variables are but I feel this seems too aggressive. > > I'd suggest do more tests if possible before commit. The SDNUMEXSYSMS isn't always zero, it depends on what it reads from the file: if (m_pStream->readInteger(&pSymbolDictDecoder->SDNUMEXSYMS) != 0 In the case of this file, generated by cluserfuzz, it is zero.
https://codereview.chromium.org/2419553002/diff/1/core/fxcodec/jbig2/JBig2_Sd... File core/fxcodec/jbig2/JBig2_SddProc.cpp (right): https://codereview.chromium.org/2419553002/diff/1/core/fxcodec/jbig2/JBig2_Sd... core/fxcodec/jbig2/JBig2_SddProc.cpp:252: if (EXINDEX > SDNUMEXSYMS) { On 2016/10/12 16:41:47, kcwu wrote: > Since SDNUMEXSYMS is always zero, this check means rejecting EXINDEX > 0. I > don't know what these variables are but I feel this seems too aggressive. > > I'd suggest do more tests if possible before commit. So, please ignore my previous stupid comment. https://codereview.chromium.org/2419553002/diff/1/core/fxcodec/jbig2/JBig2_Sd... core/fxcodec/jbig2/JBig2_SddProc.cpp:580: if (EXINDEX > SDNUMEXSYMS) { As your commit message said, it will leak if the number of EXFLAGS[]==true is more than SDNUMEXSYMS. But why do you check EXINDEX > SDNUMEXSYMS here? EXINDEX includes both 0 and 1 since CUREXFLAG is flipped for each run.
PTAL. https://codereview.chromium.org/2419553002/diff/1/core/fxcodec/jbig2/JBig2_Sd... File core/fxcodec/jbig2/JBig2_SddProc.cpp (right): https://codereview.chromium.org/2419553002/diff/1/core/fxcodec/jbig2/JBig2_Sd... core/fxcodec/jbig2/JBig2_SddProc.cpp:580: if (EXINDEX > SDNUMEXSYMS) { On 2016/10/12 23:49:58, kcwu wrote: > As your commit message said, it will leak if the number of EXFLAGS[]==true is > more than SDNUMEXSYMS. But why do you check EXINDEX > SDNUMEXSYMS here? > > EXINDEX includes both 0 and 1 since CUREXFLAG is flipped for each run. Ack, good point. Fixed.
lgtm
The CQ bit was checked by dsinclair@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/2419553002/#ps20001 (title: "Review feedback")
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 ========== Verify number of ex flags matches number of ex items. Currently the JBig2 decoder can leak subimages in the case where we mark more items in EXFLAGS then we have SDNUMEXSYMS. This Cl checks for this condition and fails the decode if it happens. BUG=chromium:654365 ========== to ========== Verify number of ex flags matches number of ex items. Currently the JBig2 decoder can leak subimages in the case where we mark more items in EXFLAGS then we have SDNUMEXSYMS. This Cl checks for this condition and fails the decode if it happens. BUG=chromium:654365 Committed: https://pdfium.googlesource.com/pdfium/+/6e5239c6e3891d78e7b9e8262c23cd129f0c... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://pdfium.googlesource.com/pdfium/+/6e5239c6e3891d78e7b9e8262c23cd129f0c... |
