1569594 - Crash [@ mozilla::net::nsHttpHandler::AsyncOnChannelRedirect]

Status: RESOLVED WORKSFORME

(Core :: Networking: HTTP, defect, P2)

Description

[Christian Holler (:decoder)]

The attached testcase crashes on mozilla-central revision 20190729-50df4b75c9b6 (build with (buildFlags not available)).

For detailed crash information, see attachment.

To reproduce the issue, perform the following steps:

1. Download the attached testcase, save as "test.bin".
   2a. Build with --enable-fuzzing (requires Clang and ASan, also build gtests using ./mach gtest dontruntests).
   2b. Alternatively you can download builds from TC using python -mfuzzfetch -a --fuzzing --tests gtest (see https://github.com/MozillaSecurity/fuzzfetch).
2. Run MOZ_RUN_GTEST=1 LIBFUZZER=1 FUZZER=NetworkHttp objdir/dist/bin/firefox test.bin

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Junior [inactive]]

Hello :decoder,
Did the crash happen on the non-debug build?
Looks like we have an assertion which is not triggered.

https://searchfox.org/mozilla-central/rev/1eb05019f47069172ba81a6c108a584a409a24ea/netwerk/protocol/http/nsHttpHandler.cpp#833-836

Comment 4

[Frederik Braun [:freddy]]

I can verify this crashes the parent process on a non-debug nightly build.
https://crash-stats.mozilla.org/report/index/4291f978-d810-4c32-8dc5-782040190801

Comment 5

[Christian Holler (:decoder)]

(In reply to Junior [:junior] from comment #3)

Hello :decoder,
Did the crash happen on the non-debug build?
Looks like we have an assertion which is not triggered.

https://searchfox.org/mozilla-central/rev/1eb05019f47069172ba81a6c108a584a409a24ea/netwerk/protocol/http/nsHttpHandler.cpp#833-836

Correct, this fuzzing target is currently not running on debug builds. Builds are non-debug optimized ASan.

Comment 6

[Junior [inactive]]

Cool. That makes senses.
Thanks, :decoder and :freddyb.

Well, the crashed object is only for telemetry.
I can do a null-check and leave the assertion since we're expecting we have one url.

Comment 7

[Christian Holler (:decoder)]

I can make a patch for this that just null-checks newURI in nsHttpHandler::AsyncOnChannelRedirect. However, I would like to confirm that it is expected in this case that this function gets a channel that has no URI associated. If that isn't case, then there might be a bug deeper in the code somewhere.

:junior, can you answer this? If not, feel free to forward the needinfo to e.g. :dragana.

Comment 8

[Junior [inactive]]

I would say it's not expected, but dragana might have some insight.

Comment 9

[Honza Bambas (:mayhemer)]

We crash here: https://hg.mozilla.org/mozilla-central/file/50df4b75c9b6/netwerk/protocol/http/nsHttpHandler.cpp#l836

nsresult nsHttpHandler::AsyncOnChannelRedirect(
    nsIChannel* oldChan, nsIChannel* newChan, uint32_t flags,
    nsIEventTarget* mainThreadEventTarget) {
  MOZ_ASSERT(NS_IsMainThread() && (oldChan && newChan));

  nsCOMPtr<nsIURI> newURI;
  newChan->GetURI(getter_AddRefs(newURI));
  MOZ_ASSERT(newURI);

  nsAutoCString scheme;

  newURI->GetScheme(scheme);  <----

that is definitely very unexpected.

Comment 10

[Christian Holler (:decoder)]

Attachment: log-663df481fcbf.txt

I cannot reproduce this in local builds but it reproduces cleanly on TC builds. Here is the log, it looks like we do a redirect to a "view-source:" URI and then the new channel doesn't have a URI so newURI remains empty.

Since the newURI is only used to gather Telemetry on the redirect scheme, I can fix this crash by null-checking newURI and only send Telemetry if we actually have a new URI.

Honza, does that sound like the correct action here or do we expect the new channel to have the URI in any case, even for this weird "view-source:" redirect?

Comment 11

[Honza Bambas (:mayhemer)]

P2 as this is reproducible and show a weakness. I don't understand why this is not deterministic. In the log I can see we don't correctly process the response in the transaction (logan doesn't see it, there are just sparse nsHttpTransaction::ParseLine calls). If the new channel doesn't have a URI (a totally unexpected thing) it means it has been ill-constructed and we are not properly checking all error results somewhere.

Comment 12

[Honza Bambas (:mayhemer)]

(In reply to Christian Holler (:decoder) from comment #10)

Honza, does that sound like the correct action here or do we expect the new channel to have the URI in any case, even for this weird "view-source:" redirect?

Every channel must have a URI set.

Comment 13

[Honza Bambas (:mayhemer)]

Releasing back to the triage pool for now.

Comment 14

[Valentin Gosu [:valentin]]

I think we no longer crash on this one, right?
We might still hit the assertion though.