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

replace unsafe new Buffer() by Buffer.alloc() when decode and encode #106

Closed
wants to merge 4 commits into from

Conversation

sodawy
Copy link

@sodawy sodawy commented Jan 6, 2018

  1. session is sensitive data. new Buffer() was deprecated and unsafe(expose the session to public buffer memory). Buffer.alloc will never use buffer_pools, a little slower but safe.
  2. add test: when session contains multibyte character should still work.
  3. update debug to ^2.6.9, fix: remove ReDoS regexp

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling a60955a on sodawy:master into 5e41b47 on koajs:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling a60955a on sodawy:master into 5e41b47 on koajs:master.

@sodawy sodawy changed the title replace unsafe new Buffer() by Buffer.alloc() when decode&encode replace unsafe new Buffer() by Buffer.alloc() when decode and encode Jan 6, 2018
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling a228f9b on sodawy:master into 5e41b47 on koajs:master.

@coveralls
Copy link

coveralls commented Jan 6, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 7627347 on sodawy:master into 5e41b47 on koajs:master.

@Runrioter
Copy link
Contributor

Buffer.from(...) is better.

@sodawy
Copy link
Author

sodawy commented Jan 6, 2018

@Runrioter When the buffer size is less than 4KB in default conf, Buffer.from(...) Buffer.allocUnsafe(...) new Buffer(...) , they will reuse the same buffer pool, while Buffer.alloc(...) will always alloc new Buffer without share.

Session data is sensitive. If someone or other middlewares reuse the buffer pool without fill, the session data may be dumped.

That is why new Buffer(...) is dangerous.

https://nodejs.org/dist/latest-v8.x/docs/api/buffer.html#buffer_buffer_from_buffer_alloc_and_buffer_allocunsafe

@Runrioter
Copy link
Contributor

Runrioter commented Jan 6, 2018

@sodawy Firstly, your understanding of buffer is right.
But

  1. new version method in Buffer mostly has default fill and be expected to be used.
  2. since node v8.0.0, new Buffer(size) will return zero-filled memory by default.
  3. if some package use buffer without filling, it maybe be unsafe and need to be fixed.
  4. In fact, here both performance and memory usage are more important.

Finally, if there are bad software running in your server, Buffer.alloc doesn't help you

@sodawy
Copy link
Author

sodawy commented Jan 6, 2018

@Runrioter

  1. Session data in public memory is unsafe, this is the most important i think.

  2. I think the performance between Buffer.alloc and Buffer.from is acceptable.

In node benchmark, Buffer.alloc is faster than Buffer.from
https://github.com/nodejs/node/blob/master/benchmark/buffers/buffer-creation.js
I run in my mac with nodeV8.9.1:

buffers/buffer-creation.js n=1024 len=100 type="fast-alloc-fill": 922.0554291784343
buffers/buffer-creation.js n=1024 len=100 type="fast-allocUnsafe": 8,343.75447081424

Another test:

const TIMES_K = 1024
const STR = 'eyJ2aWV3cyI6MSwiX2V4cGlyZSI6MTUxNTI1OTQwNzMxNSwiX21heEFnZSI6ODY0MDAwMDB9';


function byFrom(times, string) {
    const start = Date.now();
    for(let i = 0; i < times; i++){
        Buffer.from(string, 'base64')
    }
    return (Date.now() - start)/times
}

function byAlloc(times, string) {
    const start = Date.now();
    for(let i = 0; i < times; i++){
        Buffer.alloc(Buffer.byteLength(string, 'base64'), string, 'base64')
    }
    return (Date.now() - start)/times
}

console.log(`every alloc operation by Buffer.from: ${byFrom(TIMES_K, STR)}ms`); //
console.log(`every alloc operation by Buffer.alloc: ${byAlloc(TIMES_K, STR)}ms`);

/*
output
every alloc operation by Buffer.from: 0.00390625ms
every alloc operation by Buffer.alloc: 0.005859375ms

every buffer alloc, Buffer.alloc is shower than Buffer.from 0.002ms
*/

@Runrioter
Copy link
Contributor

Runrioter commented Jan 6, 2018

@sodawy oh sorry, not only performance, memory usage is also important when we meet high concurrence.

@sodawy
Copy link
Author

sodawy commented Jan 6, 2018

@Runrioter Haha~ Never mind, I keep my opinion.

@kroleg
Copy link
Contributor

kroleg commented Dec 18, 2018

@dead-horse new Buffer() calls were replaced in #159, so this PR is obsolete

@sodawy
Copy link
Author

sodawy commented Dec 25, 2018

@dead-horse new Buffer() calls were replaced in #159, so this PR is obsolete

@kroleg thanks~

@sodawy sodawy closed this Dec 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants