New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 891990 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 5
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

mojo::UnwrapPlatformHandle() may crash when the input handle is invalid

Project Member Reported by hashimoto@chromium.org, Oct 4

Issue description

mojo::edk::PassWrappedInternalPlatformHandle() was replaced with mojo::UnwrapPlatformHandle().
(e.g. https://chromium-review.googlesource.com/c/chromium/src/+/1100041/7/chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc)

These two functions are almost identical, but the former just returns an error when an invalid handle is given as an argument, but the other may crash because the dispatcher is null if the input handle is invalid.
(https://chromium.googlesource.com/chromium/src/+/270f4a68bd597964b95feb413bb5085286cb0adc/mojo/core/core.cc#1020)

IMO we should change UnwrapPlatformHandle() to perform null check to avoid this crash.
 
Status: Started (was: Assigned)
Made a CL https://chromium-review.googlesource.com/c/chromium/src/+/1260782
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 4

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/317fd980dd9504d3fe321b53469fee79e3276fed

commit 317fd980dd9504d3fe321b53469fee79e3276fed
Author: Ryo Hashimoto <hashimoto@chromium.org>
Date: Thu Oct 04 05:04:22 2018

Mojo: Check if dispatcher is null in Core::UnwrapPlatformHandle()

The same check is done in other functions in this .cc file.
Do the same thing for UnwrapPlatformHandle().

BUG= 891990 
TEST=mojo_unittests

Change-Id: I05fe4bfd5edd8ec3fc67aeb9f11879c74fd71dd4
Reviewed-on: https://chromium-review.googlesource.com/c/1260782
Reviewed-by: Ken Rockot <rockot@chromium.org>
Commit-Queue: Ryo Hashimoto <hashimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596510}
[modify] https://crrev.com/317fd980dd9504d3fe321b53469fee79e3276fed/mojo/core/core.cc

Labels: Merge-Request-70 Merge-Request-69
Requesting merge to M70 and M69 (if it's not too late).
This patch fixes a crash bug.
Project Member

Comment 4 by sheriffbot@chromium.org, Oct 4

Labels: -Merge-Request-70 Merge-Review-70 Hotlist-Merge-Review
This bug requires manual review: We are only 11 days from stable.
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Review-70 Merge-Approved-70
Approved for 70. I think we're too late for 69.
Project Member

Comment 6 by bugdroid1@chromium.org, Oct 5

Labels: -merge-approved-70 merge-merged-3538
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/077c30ba522b89a3ab11194794e838e0339b300b

commit 077c30ba522b89a3ab11194794e838e0339b300b
Author: Ryo Hashimoto <hashimoto@chromium.org>
Date: Fri Oct 05 01:00:06 2018

Mojo: Check if dispatcher is null in Core::UnwrapPlatformHandle()

The same check is done in other functions in this .cc file.
Do the same thing for UnwrapPlatformHandle().

BUG= 891990 
TEST=mojo_unittests

Change-Id: I05fe4bfd5edd8ec3fc67aeb9f11879c74fd71dd4
Reviewed-on: https://chromium-review.googlesource.com/c/1260782
Reviewed-by: Ken Rockot <rockot@chromium.org>
Commit-Queue: Ryo Hashimoto <hashimoto@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#596510}(cherry picked from commit 317fd980dd9504d3fe321b53469fee79e3276fed)
Reviewed-on: https://chromium-review.googlesource.com/c/1263538
Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#868}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
[modify] https://crrev.com/077c30ba522b89a3ab11194794e838e0339b300b/mojo/core/core.cc

Status: Fixed (was: Started)
Labels: Merge-Merged-70-3538
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/077c30ba522b89a3ab11194794e838e0339b300b

Commit: 077c30ba522b89a3ab11194794e838e0339b300b
Author: hashimoto@chromium.org
Commiter: hashimoto@chromium.org
Date: 2018-10-05 01:00:06 +0000 UTC

Mojo: Check if dispatcher is null in Core::UnwrapPlatformHandle()

The same check is done in other functions in this .cc file.
Do the same thing for UnwrapPlatformHandle().

BUG= 891990 
TEST=mojo_unittests

Change-Id: I05fe4bfd5edd8ec3fc67aeb9f11879c74fd71dd4
Reviewed-on: https://chromium-review.googlesource.com/c/1260782
Reviewed-by: Ken Rockot <rockot@chromium.org>
Commit-Queue: Ryo Hashimoto <hashimoto@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#596510}(cherry picked from commit 317fd980dd9504d3fe321b53469fee79e3276fed)
Reviewed-on: https://chromium-review.googlesource.com/c/1263538
Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#868}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}

Sign in to add a comment