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

Jump to bottom

bodyParser ignores maxBodySize and buffers whole file in memory for file uploads #956

Open
romandecker opened this issue Nov 23, 2015 · 5 comments
Open

bodyParser ignores maxBodySize and buffers whole file in memory for file uploads #956

romandecker opened this issue Nov 23, 2015 · 5 comments
Labels
Bug Critical Good First Issue Help Wanted

Comments

@romandecker
Copy link

@romandecker romandecker commented Nov 23, 2015

The bodyParser middleware seems to buffer the whole file in memory for file uploads.

Check the following minimum example:

'use strict';

var os = require( 'os' );
var restify = require( 'restify' );

var server = restify.createServer( {} );

server.use( restify.bodyParser( {
  // this should supposedly limit maximum upload size to 10 KiB
  maxBodySize: 10 * 1024,
  mapParms: true,
  mapFiles: true,
  keepExtensions: true,
  uploadDir: os.tmpdir()
} ) );

server.get( '/', function( req, res, next ) {
  res.json( { message: 'hello' } );
  next();
} );

server.post( '/upload', function( req, res, next ) {
  console.log( 'Received files:', req.files );
  res.send( 200 );
  next();
} );

server.listen( 8000 );

Then, upload a really big file to it:

# bigfile.dat has a couple of GiB
curl -F "file=@/path/to/bigfile.dat" localhost:8000/upload

This will send the process into accumulating tons of memory until it eventually runs out of memory and crashes with:

FATAL ERROR: JS Allocation failed - process out of memory

Am I making a mistake in setting up the middleware or is this a bug?
@micahr
Copy link
Member

@micahr micahr commented Nov 23, 2015

Looks like there are two pieces in play here, the maxBodySize doesn't limit the upload, rather it limits the body size regardless of how much data is uploaded. The second piece is running out of memory due to Node reading in the file.

Restify could abort the request and return a HTTP 413 status code in that case. @yunong or @DonutEspresso what are your thoughts?

@yunong
Copy link
Member

@yunong yunong commented Nov 23, 2015

Makes sense that this is a bug. It should respect the body size.

@yunong yunong added the Bug label Nov 23, 2015
@rkarami
Copy link

@rkarami rkarami commented Dec 31, 2017

so what happens? how should we limit uploaded file size?

@retrohacker
Copy link
Member

@retrohacker retrohacker commented Feb 8, 2018

Hey @rkarami! It doesn't look like there currently is a way to effectively limit the upload size at the front door. If you are interested in taking a pass at a PR I would be happy to offer help!

@richardkiene
Copy link
Contributor

@richardkiene richardkiene commented Aug 31, 2018

@micahr, @retrohacker, and @yunong FWIW I have only been able to reproduce this on Restify versions <= 2.x.x. It seems like the current code for the body reader reads in chunks until it is done or when a given chunk exceeds maxBodySize, which seems effective but it's possible I'm missing something.

I tested with different Node versions (0.10.48, 4.8.5, 6.14.3) and different Restify versions (2.0.0, 3.0.0, 4.0.0, 4.3.3). I used the exact code listed in this issue and an 8GB file uploaded from a separate machine to the test application. Only restify 2.0.0 hit ENOMEM and ENOSPC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Critical Good First Issue Help Wanted
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@micahr @romandecker @richardkiene @retrohacker @yunong @hekike @rkarami