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.

Sign up for GitHub

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

buffer: make byteLength work with Buffer correctly #4738

Closed
wants to merge 1 commit into from
Closed

buffer: make byteLength work with Buffer correctly #4738

wants to merge 1 commit into from

Conversation

JacksonTian
Copy link
Contributor

Make the byteLength work correctly when input is Buffer.

e.g:

// The incomplete unicode string
Buffer.byteLength(new Buffer([0xe4, 0xb8, 0xad, 0xe6, 0x96]))

The old output: 9
The new output: 5

@mscdex mscdex added the buffer Issues and PRs related to the buffer subsystem. label Jan 18, 2016
bnoordhuis
bnoordhuis reviewed Jan 18, 2016
View reviewed changes
lib/buffer.js
@@ -262,6 +262,10 @@ function base64ByteLength(str, bytes) {


function byteLength(string, encoding) {
if (string instanceof Buffer) {
return string.length;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you drop the braces? It's out of style with the surrounding code.

@bnoordhuis
Copy link
Member

LGTM with a style nit. CI: https://ci.nodejs.org/job/node-test-pull-request/1287/

@JacksonTian
buffer: make byteLength work with Buffer correctly
d4f3dbc 
Make the byteLength work correctly when input is Buffer.

e.g:

```js
// The incomplete unicode string
Buffer.byteLength(new Buffer([0xe4, 0xb8, 0xad, 0xe6, 0x96]))
```
The old output: 9
The new output: 5
@JacksonTian
Copy link
Contributor Author

Fixed the style. Thanks @bnoordhuis

@cjihrig
Copy link
Contributor

cjihrig commented Jan 18, 2016

LGTM. Those same tests were failing on other CI runs last night, so I'm going to say they are unrelated.

@mscdex
Copy link
Contributor

mscdex commented Jan 18, 2016

LGTM

1 similar comment
@jasnell
Copy link
Member

jasnell commented Jan 18, 2016

LGTM

jasnell pushed a commit that referenced this pull request Jan 18, 2016
@JacksonTian @jasnell
buffer: make byteLength work with Buffer correctly
8d0ca10 
Make the byteLength work correctly when input is Buffer.

e.g:

```js
// The incomplete unicode string
Buffer.byteLength(new Buffer([0xe4, 0xb8, 0xad, 0xe6, 0x96]))
```
The old output: 9
The new output: 5

PR-URL: #4738
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jasnell
Copy link
Member

jasnell commented Jan 18, 2016

Landed in 8d0ca10

@jasnell jasnell closed this Jan 18, 2016
@evanlucas evanlucas mentioned this pull request Jan 18, 2016
Merged
evanlucas pushed a commit that referenced this pull request Jan 18, 2016
@JacksonTian @evanlucas
buffer: make byteLength work with Buffer correctly
f221a43 
Make the byteLength work correctly when input is Buffer.

e.g:

```js
// The incomplete unicode string
Buffer.byteLength(new Buffer([0xe4, 0xb8, 0xad, 0xe6, 0x96]))
```
The old output: 9
The new output: 5

PR-URL: #4738
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@STRML
Copy link
Contributor

STRML commented Jan 19, 2016

Thanks for this and what fortuitous timing; we just squashed a bug yesterday that was causing corrupted writes to Redis (trailing garbage data) because we were using Buffer.byteLength(buffer) when we should have been using buffer.length. Thankfully the data never hit clients but it certainly was scary enough. Thanks for this fix!

@JacksonTian JacksonTian deleted the byteLength branch January 20, 2016 03:27
MylesBorins pushed a commit that referenced this pull request Jan 28, 2016
@JacksonTian
buffer: make byteLength work with Buffer correctly
f7f0b97 
Make the byteLength work correctly when input is Buffer.

e.g:

```js
// The incomplete unicode string
Buffer.byteLength(new Buffer([0xe4, 0xb8, 0xad, 0xe6, 0x96]))
```
The old output: 9
The new output: 5

PR-URL: #4738
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 11, 2016
@JacksonTian
buffer: make byteLength work with Buffer correctly
fa62321 
Make the byteLength work correctly when input is Buffer.

e.g:

```js
// The incomplete unicode string
Buffer.byteLength(new Buffer([0xe4, 0xb8, 0xad, 0xe6, 0x96]))
```
The old output: 9
The new output: 5

PR-URL: #4738
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 11, 2016
@JacksonTian
buffer: make byteLength work with Buffer correctly
d8fbefc 
Make the byteLength work correctly when input is Buffer.

e.g:

```js
// The incomplete unicode string
Buffer.byteLength(new Buffer([0xe4, 0xb8, 0xad, 0xe6, 0x96]))
```
The old output: 9
The new output: 5

PR-URL: nodejs#4738
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 11, 2016
Merged
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 13, 2016
@JacksonTian @MylesBorins
buffer: make byteLength work with Buffer correctly
c711d51 
Make the byteLength work correctly when input is Buffer.

e.g:

```js
// The incomplete unicode string
Buffer.byteLength(new Buffer([0xe4, 0xb8, 0xad, 0xe6, 0x96]))
```
The old output: 9
The new output: 5

PR-URL: nodejs#4738
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 13, 2016
@MylesBorins
2016-02-12, Version 4.3.1 'Argon' (LTS)
5dd0c2f 
Notable changes:

* buffer: make byteLength work with Buffer correctly (Jackson Tian)
  - nodejs#4738
* debugger: guard against call from non-node context (Ben Noordhuis)
  - nodejs#4328
* node_contextify: do not incept debug context (Myles Borins)
  - nodejs#4815
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 15, 2016
@JacksonTian
buffer: make byteLength work with Buffer correctly
748d2b4 
Make the byteLength work correctly when input is Buffer.

e.g:

```js
// The incomplete unicode string
Buffer.byteLength(new Buffer([0xe4, 0xb8, 0xad, 0xe6, 0x96]))
```
The old output: 9
The new output: 5

PR-URL: nodejs#4738
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 16, 2016
2016-02-16, Version 4.3.1 'Argon' (LTS)
ef37a46 
Notable changes:

* buffer: make byteLength work with Buffer correctly (Jackson Tian)
  - nodejs#4738
* debugger: guard against call from non-node context (Ben Noordhuis)
  - nodejs#4328
* node_contextify: do not incept debug context (Myles Borins)
  - nodejs#4819
* deps: update to http-parser 2.5.2 (James Snell)
  - nodejs#5238
MylesBorins pushed a commit that referenced this pull request Feb 17, 2016
2016-02-16, Version 4.3.1 'Argon' (LTS)
c1b3d78 
Notable changes:

* buffer: make byteLength work with Buffer correctly (Jackson Tian)
  - #4738
* debugger: guard against call from non-node context (Ben Noordhuis)
  - #4328
* node_contextify: do not incept debug context (Myles Borins)
  - #4819
* deps: update to http-parser 2.5.2 (James Snell)
  - #5238

PR-URL: #5200 (comment)
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
@JacksonTian
buffer: make byteLength work with Buffer correctly
7553cdb 
Make the byteLength work correctly when input is Buffer.

e.g:

```js
// The incomplete unicode string
Buffer.byteLength(new Buffer([0xe4, 0xb8, 0xad, 0xe6, 0x96]))
```
The old output: 9
The new output: 5

PR-URL: nodejs#4738
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
shugigo added a commit to shugigo/subtext that referenced this pull request Jun 10, 2016
@shugigo
Node 4.3.1 or higer is required for proper multipart parse
5e1db35 
Until Node commit nodejs/node#4738 Buffer.byteLength function did not work correctly. (This commit was entered in Node 4.3.1 https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V4.md#4.3.1)
On hapijs/pez project, the commit hapijs/pez@9f0042b#diff-6d186b954a58d5bb740f73d84fe39073R162 was depending on Buffer.byteLength to work correctly.
This resulted in a misbehaving multipart parsing when the payload maxBytes is close to the uploaded binary file (Inside the multipart form post payload).
Meaning that Subtext library parsing a multipart payload with a binary file that its content-length is smaller than the maxBytes parameter but is close enough than the inner hapijs/pez libary will count mistakenly for a bigger buffer size and will return an error "Maximum size exceeded" (Subtext will return "Invalid multipart payload format").

I wrote a little program that will help you understand the described scenario.
Notice: 
1. You need to put a binary file inside the project for the program to work.
2. In my example the file was 28MB and the limit I entered was 30MB.

Package.json:
{
  "name": "subtext_example",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "author": "",
  "license": "ISC",
  "dependencies": {
    "hapi": "^13.4.1",
    "restler": "^3.4.0",
    "subtext": "^4.0.4"
  }
}


Index.json
'use strict';

const Hapi = require('hapi');
const Subtext = require('subtext');
const Restler = require('restler');
const Fs = require('fs');
const Path = require('path');

const server = new Hapi.Server();
server.connection({ port: 3000 });

var demoFilePath = Path.join(__dirname,"binary_file.exe");
var demoFileMimeType = "application/x-msdownload";
var postUrl = "http://localhost:3000/test_subtext";
var payloadMaxBytes = (30 * 1024 * 1024);

server.route({
    method: 'POST',
    path: '/test_subtext',
    config : {
        payload : {
            output: 'stream',
            parse : false,
            timeout : false,
            maxBytes : (1024 * 1024 * 1024)
        }
    },
    handler: function (request, reply) {
        var subtextParseOptions = {
                output: 'file',
                parse : true,
                timeout : false,
                maxBytes : payloadMaxBytes
            };

        const onParsed = (err, parsed) => {

            request.mime = parsed.mime;
            request.payload = parsed.payload || null;

            if (!err) {
                return reply();
            }

            const failAction = request.route.settings.payload.failAction;         // failAction: 'error', 'log', 'ignore'
            if (failAction !== 'ignore') {
                request._log(['payload', 'error'], err);
            }

            if (failAction === 'error') {
                return reply(err);
            }

            return reply();
        };

        request._isPayloadPending = true;
        Subtext.parse(request.raw.req, request._tap(), subtextParseOptions, (err, parsed) => {

            if (!err ||
                !request._isPayloadPending) {

                request._isPayloadPending = false;
                return onParsed(err, parsed);
            }

            // Flush out any pending request payload not consumed due to errors

            const stream = request.raw.req;

            const read = () => {

                stream.read();
            };

            const end = () => {

                stream.removeListener('readable', read);
                stream.removeListener('error', end);
                stream.removeListener('end', end);

                request._isPayloadPending = false;
                return onParsed(err, parsed);
            };

            stream.on('readable', read);
            stream.once('error', end);
            stream.once('end', end);
        });
    }
});

server.start((err) => {
    Fs.stat(demoFilePath, function(err, stats) {
        Restler.post(postUrl, {
            multipart: true,
            data: {
                "firstKey": "firstValue",
                "filename": Restler.file(demoFilePath, null, stats.size, null, demoFileMimeType)
            }
        }).on("complete", function(data) {
            console.log('Completed: ' + JSON.stringify(data));
        });
    });
});
@shugigo shugigo mentioned this pull request Jun 10, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
buffer Issues and PRs related to the buffer subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@JacksonTian @bnoordhuis @cjihrig @mscdex @jasnell @STRML @MylesBorins