Add implementation of MultipartFormData streaming encoder

[andreiltd]

I did a quick comparison of encoded FormData between SM and Chrome using following JS code:

Simple FormData

async function readStream(stream) {
 const reader = stream.getReader();
 const chunks = [];

 while (true) {
   const { done, value } = await reader.read();
   if (done) {
     break;
   }
   chunks.push(value);
 }

 let totalLen = 0;
 for (const chunk of chunks) {
   totalLen += chunk.length;
 }

 const result = new Uint8Array(totalLen);

 let offset = 0;
 for (const chunk of chunks) {
   result.set(chunk, offset);
   offset += chunk.length;
 }

 return result;
}

async function main() {
 const form = new FormData();
 form.append('field1', 'value1');
 form.append('field2', 'value2');

 const file = new File(['Hello World!'], 'dummy.txt', { type: 'foo' });
 form.append('file1', file);

 const req = new Request('https:example.com', {
   method: 'POST',
   body: form,
 });

 const type = req.headers.get('Content-Type');
 const boundary = type.split('boundary=')[1];

 // assert(type.startsWith('multipart/form-data; boundary='), "Content-Type is multipart/form-data");
 const data = await readStream(req.body);
 const dec = new TextDecoder();
 const bodyStr = dec.decode(data);

 console.log(bodyStr);
}

main();

The results are:

• Chrome:

------WebKitFormBoundaryhpShnP1JqrBTVTnC
Content-Disposition: form-data; name="field1"

value1
------WebKitFormBoundaryhpShnP1JqrBTVTnC
Content-Disposition: form-data; name="field2"

value2
------WebKitFormBoundaryhpShnP1JqrBTVTnC
Content-Disposition: form-data; name="file1"; filename="dummy.txt"
Content-Type: foo

Hello World!
------WebKitFormBoundaryhpShnP1JqrBTVTnC--

• SM:

----BoundaryjXo5N4HEAXWcKrw7
Content-Disposition: form-data; name="field1"

value1
----BoundaryjXo5N4HEAXWcKrw7
Content-Disposition: form-data; name="field2"

value2
----BoundaryjXo5N4HEAXWcKrw7
Content-Disposition: form-data; name="file1"; filename="dummy.txt"
Content-Type: foo

Hello World!
----BoundaryjXo5N4HEAXWcKrw7--

I also did some manual fuzzing to test the streaming logic by varying the buffer sizes the encoder writes into. Specifically, I tested these buffer sizes in bytes: 1, 2, 4, 8, 32, 1024, and 8192 by changing the size in the implementation. Though, having some unit test framework would be nice.

Relevant RFC: https://www.rfc-editor.org/rfc/rfc2046#section-5.1.1

[andreiltd]

It looks like the CI failure is unrelated to this PR (I've observed the same failure on main branch). This is a problematic test:

    const response = await fetch("https://http-me.glitch.me/meow?header=cat:é");
    strictEqual(response.headers.get('cat'), "é");

[tschneidereit]

@andreiltd, is this ready to review, or are you still expecting to make changes to it?

[andreiltd]

Yes, this is ready to review :)

[tschneidereit]

Either I'm missing something in looking at the diff, or there is some code missing—see the inline comment on form-data-encoder.h.

One thing that I can't fully evaluate, because it'll presumably be part of the implementation of encode_stream: IIUC, field names, will always be first have their newlines normalized to CRLF, and then have those escaped. It'd be good to fold those into one operation, if possible.

Otherwise, I left a few comments and suggestions. I'm a bit concerned about allocation and general failure handling, so it'd be good to go over those aspects in some detail.

[tschneidereit]

The implementation of these seems to be missing? (And in the case of query_length I also can't find any uses.)

[andreiltd]

Hmm... that's weird:

• create function is here: https://github.com/bytecodealliance/StarlingMonkey/pull/200/files#diff-1794d7449a6c94d4fb4e898ac1d44ebd87c84c0b52e2ddc2c9e1c522cfaae801R446
• encode_stream function is here: https://github.com/bytecodealliance/StarlingMonkey/pull/200/files#diff-1794d7449a6c94d4fb4e898ac1d44ebd87c84c0b52e2ddc2c9e1c522cfaae801R434

I totally forgot about query_length, I'm sorry about that! The reason for this is that the fetch spec doesn't say what to do with Content-Length in case of FormData.

Set length to unclear, see html/6424 for improving this.

See here: https://fetch.spec.whatwg.org/#bodyinit-unions. It technically should be possible to calculate an encoded size beforehand without doing an actual encoding, but it's unclear to me if this is needed or not.

[tschneidereit]

I think this needs a null check?

[andreiltd]

The function is overloaded but the version that doesn't take JSContetext is infallible. I can change this to separate function like escape_name and escape_name_val if we prefer explicit return value. Anyway, I've added the assertion for now.

[tschneidereit]

Does this need a null check?

[andreiltd]

This returns just a std::string.

[tschneidereit]

Is this an infallible operation? It seems like it should need to allocate memory, so I'm not sure I understand how that works. And ideally, if it does allocate, we should make that fallible—or at least assert that it didn't fail and abort execution.

[andreiltd]

Well, it technically can throw bad_alloc because it is internally calling std::string::reserve if needed - I guess this will abort excution.

https://cplusplus.com/reference/string/string/assign/

A bad_alloc exception is thrown if the function needs to allocate storage and fails.

[andreiltd]

Hi @tschneidereit, as always, thank you so much for a through review!

One thing that I can't fully evaluate, because it'll presumably be part of the implementation of encode_stream: IIUC, field names, will always be first have their newlines normalized to CRLF, and then have those escaped. It'd be good to fold those into one operation, if possible.

So the spec says this:

For each entry of entry list:

1. Replace every occurrence of U+000D (CR) not followed by U+000A (LF), and every occurrence of U+000A (LF) not preceded by U+000D (CR), in entry's name, by a string consisting of a U+000D (CR) and U+000A (LF).
2. If entry's value is not a File object, then replace every occurrence of U+000D (CR) not followed by U+000A (LF), and every occurrence of U+000A (LF) not preceded by U+000D (CR), in entry's value, by a string consisting of a U+000D (CR) and U+000A (LF).

It's really hard to follow the spec closely because the implementation is state-machine-based, and a lot of actions that the spec calls for correspond to different states in the implementation and cannot be done together. For example, processing both names and values: names are part of the entry-header state, whereas values are handled in the entry-body state.