Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
924843002:
Fix for possible crashes due to pointer value being null (Closed)
|
Created:
5 years, 10 months ago by Sunil Ratnu Modified:
5 years, 10 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, plundblad+watch_chromium.org, aboxhall+watch_chromium.org, nasko+codewatch_chromium.org, jam, yuzo+watch_chromium.org, darin-cc_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix for possible crashes due to pointer value being null
While looking through the code, found some places where we are using pointers
even though their values can be null. This patch adds null checks so as not to
have any crashes due to pointer value being null.
Committed: https://crrev.com/66592ebd9bd467b756155f72b8bee1324d95f9d8
Cr-Commit-Position: refs/heads/master@{#317001}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address review comments #
Total comments: 2
Patch Set 3 : Addressing review comments #
Messages
Total messages: 28 (13 generated)
sunil.ratnu@samsung.com changed reviewers: + aboxhall@chromium.org, jln@chromium.org
aboxhall@chromium.org: Please review changes in content/browser/accessibility/ jln@chromium.org: Please review changes in content/common/
sunil.ratnu@samsung.com changed reviewers: + l.gombos@samsung.com
dmazzoni@chromium.org changed reviewers: + dmazzoni@chromium.org
lgtm for content/browser/accessibility
Sorry, I only review sandboxing related things in content/common. Please ask a content/ reviewer.
creis@chromium.org changed reviewers: + japhet@chromium.org
creis@chromium.org changed reviewers: + creis@chromium.org
You should probably drop the webcursor.cc change and improve the fix to render_frame_impl.cc. Suggestions below. https://codereview.chromium.org/924843002/diff/1/content/common/cursors/webcu... File content/common/cursors/webcursor.cc (right): https://codereview.chromium.org/924843002/diff/1/content/common/cursors/webcu... content/common/cursors/webcursor.cc:146: return false; This is probably the wrong thing to do. It's probably legitimate to have an empty custom_data_, in which case we would write 0 bytes and move on successfully. There's no reason to abort early here with a return false. Are you sure there's even a bug here? Writing 0 bytes may mean that we don't touch |data| at all, so it might not matter that it's NULL. (In fact, it looks like WriteData may put that 0 into the format, so it matters that we make that call.) https://codereview.chromium.org/924843002/diff/1/content/renderer/render_fram... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/924843002/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:3776: params.is_post = true; Good catch. I don't think we should skip this line based on the entry being null, though. It's still important to mark params.is_post = true. Looking at ExtractPostId, we should probably set params.post_id to -1 if there's no entry. In fact, we should probably change ExtractPostId to take in the entry and let it handle the null case, since that's what it used to do with WebHistoryItem before @japhet's change in https://codereview.chromium.org/248013003.
New patchsets have been uploaded after l-g-t-m from dmazzoni@chromium.org
Patchset #2 (id:20001) has been deleted
Thanks for the review Charlie. I've made the changes suggested by you. Please have a look. https://codereview.chromium.org/924843002/diff/1/content/common/cursors/webcu... File content/common/cursors/webcursor.cc (right): https://codereview.chromium.org/924843002/diff/1/content/common/cursors/webcu... content/common/cursors/webcursor.cc:146: return false; Removed this change as we do not need to abort early. https://codereview.chromium.org/924843002/diff/1/content/renderer/render_fram... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/924843002/diff/1/content/renderer/render_fram... content/renderer/render_frame_impl.cc:3776: params.is_post = true; On 2015/02/17 21:26:59, Charlie Reis wrote: > Good catch. I don't think we should skip this line based on the entry being > null, though. It's still important to mark params.is_post = true. > > Looking at ExtractPostId, we should probably set params.post_id to -1 if there's > no entry. In fact, we should probably change ExtractPostId to take in the entry > and let it handle the null case, since that's what it used to do with > WebHistoryItem before @japhet's change in > https://codereview.chromium.org/248013003. Done.
Thanks. LGTM with one change. https://codereview.chromium.org/924843002/diff/40001/content/renderer/render_... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/924843002/diff/40001/content/renderer/render_... content/renderer/render_frame_impl.cc:235: if (item.httpBody().isNull()) Please change this to: if (item.isNull() || item.httpBody().isNull())
New patchsets have been uploaded after l-g-t-m from creis@chromium.org
https://codereview.chromium.org/924843002/diff/40001/content/renderer/render_... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/924843002/diff/40001/content/renderer/render_... content/renderer/render_frame_impl.cc:235: if (item.httpBody().isNull()) On 2015/02/19 00:51:37, Charlie Reis wrote: > Please change this to: > if (item.isNull() || item.httpBody().isNull()) Done.
The CQ bit was checked by sunil.ratnu@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/924843002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sunil.ratnu@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/924843002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sunil.ratnu@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/924843002/60001
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/66592ebd9bd467b756155f72b8bee1324d95f9d8 Cr-Commit-Position: refs/heads/master@{#317001} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
