Bug 108931 – Crash when loading MHTML including malformed Content-Type

• Status: NEW
• Component: WebCore Misc.

[KwangYong Choi]

MHTMLParser does not set main resource when it has malformed Content-Type. So, m_archive->mainResource() can be NULL.

[KwangYong Choi]

Created attachment 186601 [details]
Patch

[KwangYong Choi]

Created attachment 186602 [details]
Patch

Add missing files.

[WebKit Review Bot]

Comment on attachment 186602 [details]
Patch

Attachment 186602 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16371745

New failing tests:
mhtml/page_with_malformed_content_type.mht

[KwangYong Choi]

May I add empty pixel expectation for this test? In this test, I can not make testRunner.dumpAsText() call.

[KwangYong Choi]

Created attachment 186620 [details]
Patch

Add pixel expectation.

[Adam Barth]

Comment on attachment 186620 [details]
Patch

Perhaps we can use a ref test?

[KwangYong Choi]

(In reply to comment #6)
> (From update of attachment 186620 [details])
> Perhaps we can use a ref test?

Thank you for your idea. May I use an empty html file for ref?

[KwangYong Choi]

Created attachment 186786 [details]
Patch

[KwangYong Choi]

Created attachment 186795 [details]
Patch

[Build Bot]

Comment on attachment 186795 [details]
Patch

Attachment 186795 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16396124

[KwangYong Choi]

Created attachment 186972 [details]
Patch

Re-upload patch again because of win-ews false alarm.

[KwangYong Choi]

(In reply to comment #11)
> Created an attachment (id=186972) [details]
> Patch
> 
> Re-upload patch again because of win-ews false alarm.

cr-android-ews :(

[KwangYong Choi]

Created attachment 187262 [details]
Patch

[KwangYong Choi]

Review?

[Bem Jones-Bey]

Is this patch still needed?

[Alexey Proskuryakov]

Seems like it's needed for MHTML support, but I'm not sure if any ports enable MHTML. Perhaps the whole feature should be removed?

ENABLE_MHTML is never seen in sources, but it's in Tools/Scripts/webkitperl/FeatureList.pm:

define => "ENABLE_MHTML", default => (isGtk() || isEfl()), value => \$mhtmlSupport },

What is this file used for? Does this mean that Gtk and Efl enable MHTML?

[Bem Jones-Bey]

(In reply to comment #16)
> Seems like it's needed for MHTML support, but I'm not sure if any ports enable MHTML. Perhaps the whole feature should be removed?
> 
> ENABLE_MHTML is never seen in sources, but it's in Tools/Scripts/webkitperl/FeatureList.pm:
> 
> define => "ENABLE_MHTML", default => (isGtk() || isEfl()), value => \$mhtmlSupport },
> 
> What is this file used for? Does this mean that Gtk and Efl enable MHTML?

I'm not sure what that file is used for, but it is in some cmake build files as being turned on for EFL and Gtk:

Source/cmake/OptionsEfl.cmake
80:WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_MHTML ON)

Source/cmake/OptionsGTK.cmake
52:WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_MHTML ON)

But it doesn't answer if they're actually using it in practice.

[Chris Dumez]

(In reply to comment #17)
> (In reply to comment #16)
> > Seems like it's needed for MHTML support, but I'm not sure if any ports enable MHTML. Perhaps the whole feature should be removed?
> > 
> > ENABLE_MHTML is never seen in sources, but it's in Tools/Scripts/webkitperl/FeatureList.pm:
> > 
> > define => "ENABLE_MHTML", default => (isGtk() || isEfl()), value => \$mhtmlSupport },
> > 
> > What is this file used for? Does this mean that Gtk and Efl enable MHTML?
> 
> I'm not sure what that file is used for, but it is in some cmake build files as being turned on for EFL and Gtk:
> 
> Source/cmake/OptionsEfl.cmake
> 80:WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_MHTML ON)
> 
> Source/cmake/OptionsGTK.cmake
> 52:WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_MHTML ON)
> 
> But it doesn't answer if they're actually using it in practice.

Gyuyoung should confirm but last I checked we were using MHTML. FYI, I believe I fixed this crash (and another one iirc) on Blink side:
https://src.chromium.org/viewvc/blink?view=rev&revision=164584

[Chris Dumez]

Comment on attachment 187262 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=187262&action=review

> Source/WebCore/loader/DocumentLoader.cpp:498
>          return false;

If you clear m_archive here before returning, then you no longer need the fix in DocumentLoader::commitData(). See corresponding Blink fix:
https://src.chromium.org/viewvc/blink?revision=164584&view=revision

[Csaba Osztrogonác]

Created attachment 253191 [details]
Patch

[Darin Adler]

Comment on attachment 253191 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=253191&action=review

I am OK with this change, but I think we can do better.

> Source/WebCore/loader/DocumentLoader.cpp:970
> +    if (!m_archive || !m_archive->mainResource()) {

I suggest we make this fix inside ArchiveFactory::create. That function should never return an archive without a main resource. Further, the Archive class should be changed to have a reference for the main resource instead of a pointer. We don’t need to be able to create archives without main resources.