Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2277133003:
Fixed a crash in Chrome caused by duplicate certificates (Closed)
|
Created:
4 years, 3 months ago by Ivan Šandrk Modified:
4 years, 3 months ago CC:
chromium-reviews, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFixed a crash in Chrome caused by duplicate certificates
In some cases the certificates_ array would be empty thus selecting the first
row (which is non-existing) would crash Chrome.
BUG=630319
Committed: https://crrev.com/691e5d283a74fcc59967008d17f6068d071952fe
Cr-Commit-Position: refs/heads/master@{#415352}
Patch Set 1 #Patch Set 2 : Added comments, changed the fix #
Total comments: 9
Patch Set 3 : Nits #
Total comments: 8
Patch Set 4 : Nits #Messages
Total messages: 31 (21 generated)
The CQ bit was checked by isandrk@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...
Description was changed from ========== Fixed a crash in Chrome caused by duplicate certificates In some cases the certificates_ array would be empty thus selecting the first row (which is non-existing) would crash Chrome. BUG=630319 ========== to ========== Fixed a crash in Chrome caused by duplicate certificates In some cases the certificates_ array would be empty thus selecting the first row (which is non-existing) would crash Chrome. BUG=630319 ==========
isandrk@chromium.org changed reviewers: + emaxx@chromium.org
Emaxx ptal
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by isandrk@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 checked by isandrk@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
lgtm with nits. And you also need the owners review for this change. https://codereview.chromium.org/2277133003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/certificate_selector.cc (right): https://codereview.chromium.org/2277133003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:133: // TODO(isandrk): See crbug.com/641440. A certificate that was previously nit: I think it makes more sense to move this crbug reference into the very end of comment. https://codereview.chromium.org/2277133003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:134: // provided by *both* the TPM and an extension will get incorrectly nit: s/TPM/platform/ (this is a more generic and correct description) https://codereview.chromium.org/2277133003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:134: // provided by *both* the TPM and an extension will get incorrectly nit: I don't think that the text in comments should be indented. (And I don't see any examples of doing this in the existing Chrome code base.) https://codereview.chromium.org/2277133003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:137: // certificate selection dialog will be shown which will cause a crash nit: I would suggest rephrasing this sentence in a little bit more careful fashion: "..., hence the certificates_ array will be empty. But displaying a dialog with an empty list won't make much sense for the user, and also there are some CHECKs in the code that will fail when the list is empty. That's why an early exit is performed here." (I'm assuming that the comment will be moved into the Show method.) https://codereview.chromium.org/2277133003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:139: // crash). nit: Generally, why not move this text and merge with the comment in the Show method? The actual change is inside the Show method. And, if you want, you may just refer to the code in the constructor, e.g.: "... will get incorrectly filtered out in the constructor ..."
https://codereview.chromium.org/2277133003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/certificate_selector.cc (right): https://codereview.chromium.org/2277133003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:133: // TODO(isandrk): See crbug.com/641440. A certificate that was previously On 2016/08/29 14:16:40, emaxx wrote: > nit: I think it makes more sense to move this crbug reference into the very end > of comment. Done. https://codereview.chromium.org/2277133003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:134: // provided by *both* the TPM and an extension will get incorrectly On 2016/08/29 14:16:39, emaxx wrote: > nit: I don't think that the text in comments should be indented. (And I don't > see any examples of doing this in the existing Chrome code base.) Done. https://codereview.chromium.org/2277133003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:137: // certificate selection dialog will be shown which will cause a crash On 2016/08/29 14:16:40, emaxx wrote: > nit: I would suggest rephrasing this sentence in a little bit more careful > fashion: > "..., hence the certificates_ array will be empty. But displaying a dialog with > an empty list won't make much sense for the user, and also there are some CHECKs > in the code that will fail when the list is empty. That's why an early exit is > performed here." > > (I'm assuming that the comment will be moved into the Show method.) Done. https://codereview.chromium.org/2277133003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:139: // crash). On 2016/08/29 14:16:40, emaxx wrote: > nit: Generally, why not move this text and merge with the comment in the Show > method? The actual change is inside the Show method. > And, if you want, you may just refer to the code in the constructor, e.g.: > "... will get incorrectly filtered out in the constructor ..." Done.
The CQ bit was checked by isandrk@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...
isandrk@chromium.org changed reviewers: + msw@chromium.org
Hi Mike, PTAL. Thanks! Ivan
https://codereview.chromium.org/2277133003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/certificate_selector.cc (right): https://codereview.chromium.org/2277133003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:181: // TODO(isandrk): A certificate that was previously provided by *both* the What exactly is the TODO here? Will you try to fix the incorrect filtering? If that is the actual fix we want, can we avoid this change, or just add a DCHECK(!certificates_.empty()) here instead? https://codereview.chromium.org/2277133003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:184: // the certificates_ array will be empty. Displaying a dialog with an empty nit: |certificates_| https://codereview.chromium.org/2277133003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:187: // is performed here. See crbug.com/641440 for more details. nit: fix indent. https://codereview.chromium.org/2277133003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:189: GetWidget()->Close(); How is this dialog triggered? If the user clicks a button to open this manually, what happens in that case? It seems odd to just no-op if the user clicks something and expects to see a dialog. Should it show a message instead?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2277133003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/certificate_selector.cc (right): https://codereview.chromium.org/2277133003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:181: // TODO(isandrk): A certificate that was previously provided by *both* the On 2016/08/29 17:08:29, msw wrote: > What exactly is the TODO here? Will you try to fix the incorrect filtering? If > that is the actual fix we want, can we avoid this change, or just add a > DCHECK(!certificates_.empty()) here instead? So this is just a quick fix for the crash, the actual bug is tracked in crbug.com/641440 and will be fixed at a later time. https://codereview.chromium.org/2277133003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:184: // the certificates_ array will be empty. Displaying a dialog with an empty On 2016/08/29 17:08:29, msw wrote: > nit: |certificates_| Done. https://codereview.chromium.org/2277133003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:187: // is performed here. See crbug.com/641440 for more details. On 2016/08/29 17:08:29, msw wrote: > nit: fix indent. Done. https://codereview.chromium.org/2277133003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/certificate_selector.cc:189: GetWidget()->Close(); On 2016/08/29 17:08:28, msw wrote: > How is this dialog triggered? If the user clicks a button to open this manually, > what happens in that case? It seems odd to just no-op if the user clicks > something and expects to see a dialog. Should it show a message instead? Chrome network stack requests the dialog to be open during TLS handshake, this is not directly triggered by user action. So this behaviour should be fine.
lgtm as a temporary workaround, please clean this up with the real fix.
The CQ bit was checked by isandrk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from emaxx@chromium.org Link to the patchset: https://codereview.chromium.org/2277133003/#ps60001 (title: "Nits")
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 ========== Fixed a crash in Chrome caused by duplicate certificates In some cases the certificates_ array would be empty thus selecting the first row (which is non-existing) would crash Chrome. BUG=630319 ========== to ========== Fixed a crash in Chrome caused by duplicate certificates In some cases the certificates_ array would be empty thus selecting the first row (which is non-existing) would crash Chrome. BUG=630319 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Fixed a crash in Chrome caused by duplicate certificates In some cases the certificates_ array would be empty thus selecting the first row (which is non-existing) would crash Chrome. BUG=630319 ========== to ========== Fixed a crash in Chrome caused by duplicate certificates In some cases the certificates_ array would be empty thus selecting the first row (which is non-existing) would crash Chrome. BUG=630319 Committed: https://crrev.com/691e5d283a74fcc59967008d17f6068d071952fe Cr-Commit-Position: refs/heads/master@{#415352} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/691e5d283a74fcc59967008d17f6068d071952fe Cr-Commit-Position: refs/heads/master@{#415352} |
