node-formidable / formidable

The most used, flexible, fast and streaming parser for multipart form data. Supports uploading to serverless environments, AWS S3, Azure, GCP or the filesystem. Used in production.
MIT License
7.04k stars 682 forks source link

MultipartParser does not call callback in `_flush` if `state == STATE.END` #878

Closed stevenwdv closed 1 year ago

stevenwdv commented 2 years ago

Support plan

Context

What are you trying to achieve or the steps to reproduce?

I'm trying to read a MIME buffer and consume partData buffers.

import {Readable} from 'node:stream';
import formidable from 'formidable';

const mime = `--_
Content-Disposition: form-data; name="myname"

myval
--_--
`.replaceAll('\n', '\r\n');
const parser = new formidable.MultipartParser();
parser.initWithBoundary('_');
for await (const {name, buffer, start, end} of Readable.from(mime).pipe(parser))
    console.log(name, buffer ? buffer.subarray(start, end).toString() : '');
console.log('DONE');

CommonJS alternative:

const {Readable} = require('node:stream');
const {pipeline} = require('node:stream/promises');
const {MultipartParser} = require('formidable');

const mime = `--_
Content-Disposition: form-data; name="myname"

myval
--_--
`.replaceAll('\n', '\r\n');
const parser = new MultipartParser();
parser.initWithBoundary('_');
pipeline(Readable.from(mime), parser).then(() => {
    let obj;
    while ((obj = parser.read())) {
        const {name, buffer, start, end} = obj;
        console.log(name, buffer ? buffer.subarray(start, end).toString() : '');
    }
    console.log('DONE');
});

What was the result you got?

partBegin 
headerField Content-Disposition     
headerValue form-data; name="myname"
headerEnd
headersEnd
partData myval
partEnd
end

<exit code 13>

CommonJS:

<no output>

<exit code 0>

Note how 'DONE' is missing. (And for the CJS variant the .then callback is never called.) Exit code 13 means Node.js detected a deadlock and exited: https://stackoverflow.com/q/64602114. This is caused by MultipartParser not calling the done() callback in _flush if this.state === STATE.END. See Multipart.js:78.

What result did you expect?

partBegin
headerField Content-Disposition     
headerValue form-data; name="myname"
headerEnd
headersEnd
partData myval
partEnd
end
DONE

<exit code 0>

Fix

Add else done(); to MultipartParser#_flush.

stevenwdv commented 2 years ago

Hmm, actually, even with the fix it still hangs on invalid input like '--_\r\n--_--\r\n'. Not sure how to handle that.

GrosSacASac commented 2 years ago

Thanks I'll have a look

GrosSacASac commented 1 year ago

Use try catch for the failing example

import {Readable} from 'node:stream';
import {MultipartParser} from '../src/index.js';

const mime = `--_\r\n--_--\r\n`;
const parser = new MultipartParser();
parser.initWithBoundary('_');
try {
    for await (const {name, buffer, start, end} of Readable.from(mime).pipe(parser)) {
        console.log(name, buffer ? buffer.subarray(start, end).toString() : '');
    }

} catch (e) {
    console.error('error');
    console.error(e);

}
console.log('DONE');
GrosSacASac commented 1 year ago

Also good to know that Streams are now valid async iterables. First time I see for await of loop used like that.