media: Handle untrusted Mojo input with ReportBadMessage [chromium/src : main]
Vikram Pasupathy (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Dale Curtis
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis added 3 comments![]( "Open in Gerrit")
std::move(callback).Run(DecoderStatus::Codes::kFailed, false,No need to run the callback I think since it'll initiate teardown? Also does `this` have a ReportBadMessage method on it? That should be preferred over the mojo:: version if it exists.
mojo::ReportBadMessage("Unexpected stream_type");Ditto.
mojo::ReportBadMessage("The caller should not switch CDM");Ditto.
Related details
- Vikram Pasupathy
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vikram Pasupathy (Gerrit)
Vikram Pasupathy added 3 comments![]( "Open in Gerrit")
std::move(callback).Run(DecoderStatus::Codes::kFailed, false,No need to run the callback I think since it'll initiate teardown? Also does `this` have a ReportBadMessage method on it? That should be preferred over the mojo:: version if it exists.
Done for the cb.
`this` does not have the method, so keeping as `mojo::`.
mojo::ReportBadMessage("Unexpected stream_type");Vikram PasupathyDitto.
Done
mojo::ReportBadMessage("The caller should not switch CDM");Vikram PasupathyDitto.
Done
Related details
- Dale Curtis
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis added 1 comment![]( "Open in Gerrit")
CHECK(mojo::IsInMessageDispatch());ISTR ReportBadMessage only working if this check is true, otherwise you need to use `GetBadMessageCallback` and pass to where the message is handled. Are we in message handling for each of the cases you've added? Some I think yes, some it's unclear.
Related details
- Vikram Pasupathy
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vikram Pasupathy (Gerrit)
Vikram Pasupathy added 1 comment![]( "Open in Gerrit")
CHECK(mojo::IsInMessageDispatch());ISTR ReportBadMessage only working if this check is true, otherwise you need to use `GetBadMessageCallback` and pass to where the message is handled. Are we in message handling for each of the cases you've added? Some I think yes, some it's unclear.
I see, PTAL. Went around media/ to see where else to add this check.
Related details
- Dale Curtis
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis added 1 comment![]( "Open in Gerrit")
CHECK(mojo::IsInMessageDispatch());Vikram PasupathyISTR ReportBadMessage only working if this check is true, otherwise you need to use `GetBadMessageCallback` and pass to where the message is handled. Are we in message handling for each of the cases you've added? Some I think yes, some it's unclear.
I see, PTAL. Went around media/ to see where else to add this check.
The method internally has a DCHECK:
Do you think the CHECK adds much value? I was more suggesting that for cases which don't have test coverage of the bad message delivery it'll be important to audit since we'll end up with a low-high priority process crash otherwise.
Related details
- Vikram Pasupathy
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vikram Pasupathy (Gerrit)
Vikram Pasupathy added 1 comment![]( "Open in Gerrit")
CHECK(mojo::IsInMessageDispatch());Vikram PasupathyISTR ReportBadMessage only working if this check is true, otherwise you need to use `GetBadMessageCallback` and pass to where the message is handled. Are we in message handling for each of the cases you've added? Some I think yes, some it's unclear.
Dale CurtisI see, PTAL. Went around media/ to see where else to add this check.
The method internally has a DCHECK:
Do you think the CHECK adds much value? I was more suggesting that for cases which don't have test coverage of the bad message delivery it'll be important to audit since we'll end up with a low-high priority process crash otherwise.
I think CHECK is recommended regardless right? and I don't expect this CHECK to hit, it should be safe. I imagine the DCHECK was added in 2016, so noone got up to upgrading it to a CHECK.
Would you want NotFatalUntil? i.e https://chromium-review.googlesource.com/c/chromium/src/+/6360824
https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/checks.md
Related details
- Dale Curtis
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis added 1 comment![]( "Open in Gerrit")
CHECK(mojo::IsInMessageDispatch());Vikram PasupathyISTR ReportBadMessage only working if this check is true, otherwise you need to use `GetBadMessageCallback` and pass to where the message is handled. Are we in message handling for each of the cases you've added? Some I think yes, some it's unclear.
Dale CurtisI see, PTAL. Went around media/ to see where else to add this check.
Vikram PasupathyThe method internally has a DCHECK:
Do you think the CHECK adds much value? I was more suggesting that for cases which don't have test coverage of the bad message delivery it'll be important to audit since we'll end up with a low-high priority process crash otherwise.
I think CHECK is recommended regardless right? and I don't expect this CHECK to hit, it should be safe. I imagine the DCHECK was added in 2016, so noone got up to upgrading it to a CHECK.
Would you want NotFatalUntil? i.e https://chromium-review.googlesource.com/c/chromium/src/+/6360824
https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/checks.md
Sorry, I'm not that concerned about the CHECK/DCHECK, though upgrading the one in Mojo may be reasonable if we can delete all the ones elsewhere. I'm more stating that almost none of these BadMessage instances have test coverage, so we don't know that they're actually even in message dispatch without manually auditing. Hence, is that something you've already done?
Related details
- Vikram Pasupathy
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |