1187157 - new FormData(HTMLFormElement).getAll(…) or .get(…) crashes Firefox if the key passed to get/getAll belongs to an HTMLInputElement of type="file" (mozilla::dom::GetOrCreateDOMReflectorHelper<T>::GetOrCreate)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Sebastian Simon]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:39.0) Gecko/20100101 Firefox/39.0
Build ID: 20150629114917

Steps to reproduce:

<form id="a">
  <input name="b" type="file"/>
</form>
<script>
  new FormData(document.getElementById('a')).getAll('b');
</script>


Actual results:

This crashes Firefox. The same applies to .get('b');

https://crash-stats.mozilla.com/report/index/1e6276b9-2e8c-44d6-950e-41c5f2150724


Expected results:

It should have returned the value of the file input (or an Array), according to the spec and it should not have crashed.

Comment 2

[reysquared]

This specifically seems to occur when no file has been specified for the form input yet.

Comment 3

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Confirmed to occur with the current Nightly.

Comment 4

[karibou]

Firefox 64 bits 43.0.1 Windows

Works fine even if file input is empty.

But crashes with:
- form embedded in a jQuery dialog
- AND file input is empty.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Can't reproduce on nightly, aurora or beta, but can on release.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Would be great to get un-regression range to figure out what has fixed this.

https://crash-stats.mozilla.com/report/index/dc4ae136-de60-4798-b215-d4ee72160104 is what I got, and
looks like a null+offset crash deep in bindings code (it is accessing null wrapper cache which then checks for some flags offset from null)

Comment 7

[Boris Zbarsky [:bzbarsky]]

So on trunk this is throwing an uncatchable exception when I do get('b').

That's happening because the mType of the union is eUninitialized when we land in OwningFileOrUSVString::ToJSVal, which causes it to return false without setting an exception on the JSContext.

This has been the ToJSVal behavior for the eUninitialized case for a good long while, so if something changed it changed in the way the union is set up in the formdata/form code.

But the behavior is clearly bogus, right?  The value should be empty string, per https://html.spec.whatwg.org/multipage/forms.html#constructing-form-data-set step 7, and there should be no exception.

Comment 8

[Boris Zbarsky [:bzbarsky]]

So what happens here is that we end up in http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFormData.cpp#180 with null aFile, called from HTMLInputElement::SubmitNamesValues in the mType == NS_FORM_INPUT_FILE case when files.IsEmpty().

This calls SetNameFilePair, which does:

67     aData->name = aName;
68     if (aFile) {
69       aData->value.SetAsFile() = aFile;
70     }

and otherwise leaves aData->value uninitialized.  That null-check was basically added in bug 1127703; see <http://hg.mozilla.org/mozilla-central/rev/512b90a0f35f>.  My apologies for not realizing the implications of it not being present before.  :(

So on trunk the right fix is to add an else clause setting aData->value to an empty string.  On branches the right fix is to add a null check and set valueIsFile to false if aFile is null, while setting aData->stringValue to empty string....

Andrea, do you have time to write this up, with tests?

Comment 9

[Boris Zbarsky [:bzbarsky]]

And in particular, the uncatchable exception behavior here is pretty bad too, and we really should fix it asap.

Comment 10

[Andrea Marchesini [:baku]]

Attachment: crash.patch

I'll submit a patch for older branches once this patch is reviewed.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/23974693ffc5

Comment 12

[Andrea Marchesini [:baku]]

Attachment: m-b/m-a

Approval Request Comment
[Feature/regressing bug #]: bug 1085293 - WebIDL supporting iterable<>
[User impact if declined]: a crash
[Describe test coverage new/current, TreeHerder]: a test is included
[Risks and why]: none
[String/UUID change made/needed]: none

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/43f10b226ee1 for failing test_formSubmission.html on Mulet Linux x64 opt

Backout pus: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=43f10b226ee1
Failing push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=23974693ffc5
https://treeherder.mozilla.org/logviewer.html#?job_id=19370515&repo=mozilla-inbound

16:16:41     INFO -  1168 INFO TEST-UNEXPECTED-FAIL | dom/html/test/test_formSubmission.html | Correct name in send form using XHR and FormData - got "form-data; name=\"file_2\"", expected "form-data; name=\"file_2\"; filename=\"\""
16:16:41     INFO -      SimpleTest.is@SimpleTest/SimpleTest.js:267:5
16:16:41     INFO -      checkMPSubmission@dom/html/test/test_formSubmission.html:627:7
16:16:41     INFO -      runTest@dom/html/test/test_formSubmission.html:786:3
16:16:41     INFO -      runTest/xhr.onload@dom/html/test/test_formSubmission.html:782:29
16:16:41     INFO -  Not taking screenshot here: see the one that was previously logged
16:16:41     INFO -  1169 INFO TEST-UNEXPECTED-FAIL | dom/html/test/test_formSubmission.html | Correct content type in send form using XHR and FormData - got undefined, expected "application/octet-stream"
16:16:41     INFO -      SimpleTest.is@SimpleTest/SimpleTest.js:267:5
16:16:41     INFO -      checkMPSubmission@dom/html/test/test_formSubmission.html:631:7
16:16:41     INFO -      runTest@dom/html/test/test_formSubmission.html:786:3
16:16:41     INFO -      runTest/xhr.onload@dom/html/test/test_formSubmission.html:782:29
16:16:41     INFO -  Not taking screenshot here: see the one that was previously logged
16:16:41     INFO -  1170 INFO TEST-UNEXPECTED-FAIL | dom/html/test/test_formSubmission.html | Wrong number of headers in send form using XHR and FormData - got 1, expected 2
16:16:41     INFO -      SimpleTest.is@SimpleTest/SimpleTest.js:267:5
16:16:41     INFO -      checkMPSubmission@dom/html/test/test_formSubmission.html:634:7
16:16:41     INFO -      runTest@dom/html/test/test_formSubmission.html:786:3
16:16:41     INFO -      runTest/xhr.onload@dom/html/test/test_formSubmission.html:782:29
16:16:41     INFO -  1171 INFO TEST-PASS | dom/html/test/test_formSubmission.html | Correct value in send form using XHR and FormData
16:16:41     INFO -  1172 INFO TEST-PASS | dom/html/test/test_formSubmission.html | Correct name in send form using XHR and FormData
16:16:41     INFO -  1173 INFO TEST-PASS | dom/html/test/test_formSubmission.html | Correct content type in send form using XHR and FormData
16:16:41     INFO -  1174 INFO TEST-PASS | dom/html/test/test_formSubmission.html | Wrong number of headers in send form using XHR and FormData
16:16:41     INFO -  1175 INFO TEST-PASS | dom/html/test/test_formSubmission.html | Correct value in send form using XHR and FormData
16:16:41     INFO -  1176 INFO TEST-PASS | dom/html/test/test_formSubmission.html | Correct name in send form using XHR and FormData
16:16:41     INFO -  1177 INFO TEST-PASS | dom/html/test/test_formSubmission.html | Correct content type in send form using XHR and FormData
16:16:41     INFO -  1178 INFO TEST-PASS | dom/html/test/test_formSubmission.html | Wrong number of headers in send form using XHR and FormData
16:16:41     INFO -  1179 INFO TEST-PASS | dom/html/test/test_formSubmission.html | Correct value in send form using XHR and FormData
16:16:41     INFO -  1180 INFO TEST-PASS | dom/html/test/test_formSubmission.html | Correct name in send form using XHR and FormData
16:16:41     INFO -  1181 INFO TEST-PASS | dom/html/test/test_formSubmission.html | Correct content type in send form using XHR and FormData
16:16:41     INFO -  1182 INFO TEST-PASS | dom/html/test/test_formSubmission.html | Wrong number of headers in send form using XHR and FormData
16:16:41     INFO -  1183 INFO TEST-PASS | dom/html/test/test_formSubmission.html | Correct value in send form using XHR and FormData
16:16:41     INFO -  Not taking screenshot here: see the one that was previously logged
16:16:41     INFO -  1184 INFO TEST-UNEXPECTED-FAIL | dom/html/test/test_formSubmission.html | Correct name in send form using XHR and FormData - got "form-data; name=\"file_5\"", expected "form-data; name=\"file_5\"; filename=\"\""
16:16:41     INFO -      SimpleTest.is@SimpleTest/SimpleTest.js:267:5
16:16:41     INFO -      checkMPSubmission@dom/html/test/test_formSubmission.html:627:7
16:16:41     INFO -      runTest@dom/html/test/test_formSubmission.html:786:3
16:16:41     INFO -      runTest/xhr.onload@dom/html/test/test_formSubmission.html:782:29
16:16:41     INFO -  Not taking screenshot here: see the one that was previously logged
16:16:41     INFO -  1185 INFO TEST-UNEXPECTED-FAIL | dom/html/test/test_formSubmission.html | Correct content type in send form using XHR and FormData - got undefined, expected "application/octet-stream"
16:16:41     INFO -      SimpleTest.is@SimpleTest/SimpleTest.js:267:5
16:16:41     INFO -      checkMPSubmission@dom/html/test/test_formSubmission.html:631:7
16:16:41     INFO -      runTest@dom/html/test/test_formSubmission.html:786:3
16:16:41     INFO -      runTest/xhr.onload@dom/html/test/test_formSubmission.html:782:29
16:16:41     INFO -  Not taking screenshot here: see the one that was previously logged
16:16:41     INFO -  1186 INFO TEST-UNEXPECTED-FAIL | dom/html/test/test_formSubmission.html | Wrong number of headers in send form using XHR and FormData - got 1, expected 2
16:16:41     INFO -      SimpleTest.is@SimpleTest/SimpleTest.js:267:5
16:16:41     INFO -      checkMPSubmission@dom/html/test/test_formSubmission.html:634:7
16:16:41     INFO -      runTest@dom/html/test/test_formSubmission.html:786:3
16:16:41     INFO -      runTest/xhr.onload@dom/html/test/test_formSubmission.html:782:29
16:16:41     INFO -  1187 INFO TEST-PASS | dom/html/test/test_formSubmission.html | Correct value in send form using XHR and FormData

Comment 14

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8704762 [details] [diff] [review]
patch

So how did this test use to work?  Was it just happening to work because we reused a union from a previous iteration or something?

Comment 16

[Andrea Marchesini [:baku]]

Attachment: test

A separate patch for m-i/m-a/m-b.

bz, I'll answer to your question soon.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f149d988f78e
https://hg.mozilla.org/integration/mozilla-inbound/rev/524b2e90d33f

Comment 18

[Andrea Marchesini [:baku]]

The reason why before the test was working is because of this:

https://mxr.mozilla.org/mozilla-central/source/dom/html/nsFormSubmission.cpp#451

where we were forcing this contentType in case aFile was null as the spec says: "If there are no selected files, then append an entry to the form data set with the name as the name, the empty string as the value, and application/octet-stream as the type."

https://mxr.mozilla.org/mozilla-central/source/dom/html/nsFormSubmission.cpp#498

With the current changes, we don't support null files anymore, because we replace them with an empty string.
So, the current solution is not the right one.

I also think that the spec is not clear enough, because we don't have any setter for name+string+type. What should we return from a |get(that name)|? an empty string? or a blob with no content and a type?
If we want the latter, probably a good solution should be to create a new blob with no content and type and store it in the formData directly.

Comment 19

[Andrea Marchesini [:baku]]

https://github.com/whatwg/html/issues/476

Comment 20

[Anne (:annevk)]

My recommendation is that we use an empty Blob for this case, similar to what we transmit to the server.

Comment 21

[Andrea Marchesini [:baku]]

I'm filing a follow up for this next step.

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8704676 [details] [diff] [review]
m-b/m-a

Please renominate once test failures are addressed.

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f149d988f78e
https://hg.mozilla.org/mozilla-central/rev/524b2e90d33f

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late to fix this for 43.
Tracking for 45+ since this sounds like either a regression or a crash we really don't want to have.
I don't see a lot of crashes with this signature currently though.

Comment 25

[Boris Zbarsky [:bzbarsky]]

We should probably track for 44 too, yes?

And we presumably need branch approval requests here?

Comment 26

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Bz, I am not inclined to take any fixes in Fx44 now unless they are critical (recent) regressions, sec and stability fixes. While this fixes a crash, but to me the likelihood of getting into this situation is low. Please let me know if that is not the case. Thanks.!

Comment 27

[Boris Zbarsky [:bzbarsky]]

I can live with this, I guess; this is not in widespread use yet and the crash is at least a safe null-deref...

On the other hand, it also seems like we could create a pretty minimally targeted crash fix for 44, if that were desired.

Comment 28

[Andrea Marchesini [:baku]]

Comment on attachment 8704605 [details] [diff] [review]
crash.patch

Approval Request Comment
[Feature/regressing bug #]: bug 1085293 - WebIDL supporting iterable<>
[User impact if declined]: a crash
[Describe test coverage new/current, TreeHerder]: a test is included
[Risks and why]: none
[String/UUID change made/needed]: none

Comment 29

[Liz Henry (:lizzard) (relman/hg->git project)]

Thanks bz, looks like this missed 44 but we can probably uplift this at least to 45 (even if it ends up landing after merge day)

Comment 30

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8704605 [details] [diff] [review]
crash.patch

Taking it in 45 before the merge.

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fbf8189d6951
https://hg.mozilla.org/releases/mozilla-aurora/rev/826352bbda0a