Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
stevenwdv opened this issue Aug 8, 2022 · 4 comments · Fixed by #945
Closed

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

stevenwdv opened this issue Aug 8, 2022 · 4 comments · Fixed by #945
Labels

Comments

@stevenwdv
Copy link

stevenwdv commented Aug 8, 2022

Support plan

  • Which support plan is this issue covered by? (Community, Sponsor, Enterprise): Community
  • Currently blocking your project/work? (yes/no): no
  • Affecting a production system? (yes/no): no

Context

  • Node.js version:
  • Release Line of Formidable (Legacy, Current, Next): Current
  • Formidable exact version: 2.0.5
  • Environment (node, browser, native, OS): node
  • Used with (popular names of modules): -

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 stevenwdv added the bug label Aug 8, 2022
@stevenwdv
Copy link
Author

stevenwdv commented Aug 8, 2022

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

@GrosSacASac
Copy link
Contributor

Thanks I'll have a look

@GrosSacASac
Copy link
Contributor

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants