Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1986453002:
Fix binding of null pointer dereference to reference type (Closed)
DescriptionFix binding of null pointer dereference to reference type
A new Clang warning complains about this, and especially for the function
returning a reference, it seems like a bad idea.
BUG=none
Committed: https://pdfium.googlesource.com/pdfium/+/06f457216f45093dc5dff12055733aa426c26c59
Patch Set 1 #
Total comments: 4
Patch Set 2 : Use Blink/V8-style IMMEDIATE_CRASH instead" #
Total comments: 2
Patch Set 3 : Use PDFIUM_ prefix for the macro #
Messages
Total messages: 23 (4 generated)
hans@chromium.org changed reviewers: + pcc@chromium.org, thakis@chromium.org
I figured instead of jumping through hoops to dereference null here, let's just call abort(). Please take a look.
dsinclair@chromium.org changed reviewers: + dsinclair@chromium.org, tsepez@chromium.org
https://codereview.chromium.org/1986453002/diff/1/core/fxcrt/include/fx_basic.h File core/fxcrt/include/fx_basic.h (right): https://codereview.chromium.org/1986453002/diff/1/core/fxcrt/include/fx_basic... core/fxcrt/include/fx_basic.h:296: abort(); I don't think this is safe to do. We can't assume the current library isn't depending on this behaviour and we're now going to start aborting where we'd have continued previously.
https://codereview.chromium.org/1986453002/diff/1/core/fxcrt/include/fx_basic.h File core/fxcrt/include/fx_basic.h (right): https://codereview.chromium.org/1986453002/diff/1/core/fxcrt/include/fx_basic... core/fxcrt/include/fx_basic.h:296: abort(); On 2016/05/16 17:41:02, dsinclair wrote: > I don't think this is safe to do. We can't assume the current library isn't > depending on this behaviour and we're now going to start aborting where we'd > have continued previously. Getting a reference bound to null as the return value sounds like a horrible thing to depend on :-) I think the intention of the code is clearly to abort here. Grepping the source for callers at ElementAt, I don't see anything that would crash immediately on a reference-to-null.
https://codereview.chromium.org/1986453002/diff/1/core/fxcrt/include/fx_basic.h File core/fxcrt/include/fx_basic.h (right): https://codereview.chromium.org/1986453002/diff/1/core/fxcrt/include/fx_basic... core/fxcrt/include/fx_basic.h:296: abort(); On 2016/05/16 17:41:02, dsinclair wrote: > I don't think this is safe to do. We can't assume the current library isn't > depending on this behaviour and we're now going to start aborting where we'd > have continued previously. But .. they wanted to crash here, so this should be fine. Unfortunately, we wouldn't do so in many platforms since the optimizer can assume that null derefs never occur, and hence remove this deliberate null deref.
https://codereview.chromium.org/1986453002/diff/1/core/fxcrt/include/fx_basic.h File core/fxcrt/include/fx_basic.h (right): https://codereview.chromium.org/1986453002/diff/1/core/fxcrt/include/fx_basic... core/fxcrt/include/fx_basic.h:296: abort(); On 2016/05/16 17:41:02, dsinclair wrote: > I don't think this is safe to do. We can't assume the current library isn't > depending on this behaviour and we're now going to start aborting where we'd > have continued previously. References pointing to null have undefined behavior, and compilers do exploit this. Since this is an inline function in a header file, the compiler could do arbitrary things at calling points if it knows at compile time that nIndex is 0 or >= m_nSize. So that needs to be fixed anyhow. Now this'll provide a clean crash report when that happens and we'll learn about it. It might cause short-term pain, but it's the right change going forward. (Maybe this should use something like blink's IMMEDIATE_CRASH instead of abort though, not sure.)
On 2016/05/16 17:49:34, Nico wrote: > (Maybe this should use something like blink's IMMEDIATE_CRASH instead of abort > though, not sure.) Yeah, I'd be happy to add that one. It's probably more light-weight than abort().
Uploaded a version with IMMEDIATE_CRASH(). Please take another look.
sorry, bikeshedding: https://codereview.chromium.org/1986453002/diff/20001/core/fxcrt/include/fx_s... File core/fxcrt/include/fx_system.h (right): https://codereview.chromium.org/1986453002/diff/20001/core/fxcrt/include/fx_s... core/fxcrt/include/fx_system.h:106: #ifndef IMMEDIATE_CRASH hm, giving this the same name as blink's macro is maybe not the best idea -- maybe call this PDFIUM_IMMEDIATE_CRASH?
On 2016/05/16 18:11:29, Nico wrote: > sorry, bikeshedding: > > https://codereview.chromium.org/1986453002/diff/20001/core/fxcrt/include/fx_s... > File core/fxcrt/include/fx_system.h (right): > > https://codereview.chromium.org/1986453002/diff/20001/core/fxcrt/include/fx_s... > core/fxcrt/include/fx_system.h:106: #ifndef IMMEDIATE_CRASH > hm, giving this the same name as blink's macro is maybe not the best idea -- > maybe call this PDFIUM_IMMEDIATE_CRASH? Or what about just wrapped in #ifndef IMMEDIATE_CRASH ?
New patch uploaded. https://codereview.chromium.org/1986453002/diff/20001/core/fxcrt/include/fx_s... File core/fxcrt/include/fx_system.h (right): https://codereview.chromium.org/1986453002/diff/20001/core/fxcrt/include/fx_s... core/fxcrt/include/fx_system.h:106: #ifndef IMMEDIATE_CRASH On 2016/05/16 18:11:29, Nico wrote: > hm, giving this the same name as blink's macro is maybe not the best idea -- > maybe call this PDFIUM_IMMEDIATE_CRASH? Done.
lgtm from me
The CQ bit was checked by tsepez@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1986453002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1986453002/40001
On 2016/05/16 18:29:11, Nico wrote: > lgtm from me tsepez: what do you think?
On 2016/05/16 18:30:41, hans wrote: > On 2016/05/16 18:29:11, Nico wrote: > > lgtm from me > > tsepez: what do you think? oh, we collided :-) seems this is all good to go
Is the pdfium cq down? https://chromium-cq-status.appspot.com/patch-status/1986453002/40001 has said "pending" for 20 min now.
On 2016/05/16 18:51:31, Nico wrote: > Is the pdfium cq down? > https://chromium-cq-status.appspot.com/patch-status/1986453002/40001 has said > "pending" for 20 min now. I filed http://crbug.com/612217
Message was sent while issue was closed.
Description was changed from ========== Fix binding of null pointer dereference to reference type A new Clang warning complains about this, and especially for the function returning a reference, it seems like a bad idea. BUG=none ========== to ========== Fix binding of null pointer dereference to reference type A new Clang warning complains about this, and especially for the function returning a reference, it seems like a bad idea. BUG=none Committed: https://pdfium.googlesource.com/pdfium/+/06f457216f45093dc5dff12055733aa426c2... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://pdfium.googlesource.com/pdfium/+/06f457216f45093dc5dff12055733aa426c2... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
