buffer: fix lastIndexOf and indexOf in various edge cases by addaleax · Pull Request #6511 · nodejs/node

addaleax · 2016-05-02T02:01:18Z

Checklist

tests and code linting passes
a test and/or benchmark is included
the commit message follows commit guidelines

Affected core subsystem(s)

buffer

Description of change

Return -1 in Buffer.lastIndexOf if the needle is longer than the haystack. The previous check only tested the corresponding condition for forward searches.

Fixes: #6510

jasnell · 2016-05-02T02:21:11Z

You beat me to it. Had the fix ready to push lol.
Can you check to make sure this works with ucs2, utf8, and binary encode strings also?

addaleax · 2016-05-02T02:21:43Z

@jasnell … sorry. :-/ Sure.

addaleax · 2016-05-02T02:28:39Z

Updated it. CI: https://ci.nodejs.org/job/node-test-commit/3131/

jasnell · 2016-05-02T02:35:25Z

Having trouble getting to github from my laptop so I can't test your patch just yet. Can you give this one a try with your patch?

varb=Buffer.from([1,2]);b.lastIndexOf('€a',0,'ucs2')

addaleax · 2016-05-02T02:39:15Z

$./node>varb=Buffer.from([1,2]);undefined>b.lastIndexOf('€a',0,'ucs2')-1

And yep, without this patch the same code crashes Node.

jasnell · 2016-05-02T02:53:33Z

Ok. Might want to add a few tests with characters that definitely expand out to multi-bytes under utf8 also. Just to be extra safe.

jasnell · 2016-05-02T03:26:06Z

Looking good so far. @trevnorris ... PTAL

addaleax · 2016-05-02T03:27:15Z

Hmmmm, you’re right – but the other way around: Multi-byte characters with UTF-8 are fine, but ASCII characters with UTF-16 fail because needle_length underestimates the needle length. That one’s also happening for indexOf and must have been around for a really long time…

jasnell · 2016-05-02T03:32:52Z

Yep, I was just noticing the same thing. In v5.11.0 the following dies:

bash-3.2$ node -v
v5.11.0
bash-3.2$ node
> b = Buffer.from('abc')
<Buffer 61 62 63>
> b.indexOf('abc','ucs2')
Assertion failed: (0 <= index && index < length_), function operator[], file ../src/string_search.h, line 56.
Abort trap: 6
bash-3.2$

addaleax · 2016-05-02T03:34:09Z

Updated again, assuming StringBytes::Size is the right one for this (?).

jasnell · 2016-05-02T03:53:46Z

+1 ... I'm going to look at this a bit more in the morning to find more ways of breaking this. It definitely looks like v5.x is having some related issues also so we'll need to look at backporting. I haven't tested v4 yet but I suspect it's an issue there also.

bash-3.2$ node -v
v5.11.0
bash-3.2$ node
> b = Buffer.from('abc')
<Buffer 61 62 63>
> Buffer.from('ab','ucs2')
<Buffer 61 00 62 00>
> b.indexOf('ab','ucs2')
2

addaleax · 2016-05-02T03:59:38Z

Do that, please. That specific last one you posted would be fixed with this, too, but it seems to imply that there’s something more going wrong, doesn’t it? In any case, you have write perms to my repo if you want to add something here.

jasnell · 2016-05-02T04:03:45Z

Yeah, that's what I'm afraid of. Really appreciate you looking at this also

addaleax · 2016-05-02T06:04:15Z

@jasnell Heh, that last one you found is funny. Node v5.11.0:

Buffer.from("abc").indexOf("ab","ucs2")===2Buffer.from("abcd").indexOf("ab","ucs2")===-1Buffer.from("abcde").indexOf("ab","ucs2")===4Buffer.from("abcdef").indexOf("ab","ucs2")===-1Buffer.from("abcdefg").indexOf("ab","ucs2")===6

This seems to be because the haystack_length is divided by 2 before being passed to SearchString and result is multiplied with 2 after that, but that doesn’t cancel out when the string length isn’t even.

Updating again with a fix for that, too, and reordered the commits so that the ones that may be eligible for backporting come first in this PR.

jasnell · 2016-05-02T06:09:31Z

nice. well, at least it's consistently wrong :-) when I look at this more tomorrow I'm definitely going to test things out on v4 also. I think you're right that this has likely been a problem for quite some time.

jasnell · 2016-05-02T16:20:06Z

@addaleax ... ok, ran this through a few paces and couldn't find anywhere else it breaks down. The abort on that ascii/ucs2 case definitely happens in v4 also so we'll need to backport at least part of this to both v5 and v4.

Also can confirm that the following fails in v4:

> (new Buffer("abc")).indexOf("ab","ucs2")
2

jasnell · 2016-05-02T16:21:38Z

CI: https://ci.nodejs.org/job/node-test-pull-request/2462/

addaleax · 2016-05-02T16:26:30Z

The first two commits here cherry-pick cleanly to the v4.x and v5.x branches, so yep, sounds like a plan.

jasnell · 2016-05-02T16:37:09Z

Awesome. keep in mind tho that Buffer.from() does not exist in v4 so the test cases will need to be modified a bit there. Let's see what CI has to say and I definitely want @trevnorris to give it a look over when he gets a chance.

jasnell · 2016-05-02T16:37:38Z

nit: s/neares/nearest

Fix `buffer.indexOf` for the case that the haystack has odd length and the needle is not found in it. `StringSearch()` would return the length of the buffer in multiples of `sizeof(uint16_t)`, but checking that against `haystack_length` would not work if the latter one was odd. PR-URL: #6511 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

PR-URL: #6511 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

Fix `buffer.lastIndexOf()` for the case that the first character of the needle is contained in the haystack, but in a location that makes it impossible to be part of a full match. For example, when searching for 'abc' in 'abcdef', only the 'cdef' part needs to be searched for 'c', because earlier locations can be excluded by index calculations alone. Previously, such a search would result in an assertion failure. This applies only to Node.js v6, as `lastIndexOf` was added in it. PR-URL: #6511 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

Return -1 in `Buffer.lastIndexOf` if the needle is longer than the haystack. The previous check only tested the corresponding condition for forward searches. This applies only to Node.js v6, as `lastIndexOf` was added in it. Fixes: #6510 PR-URL: #6511 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

Use `StringBytes::Size` to determine the needle string length instead of assuming latin-1 or UTF-8. Previously, `Buffer.indexOf` could fail with an assertion failure when the needle's byte length, but not its character count, exceeded the haystack's byte length. PR-URL: #6511 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

Fix `buffer.indexOf` for the case that the haystack has odd length and the needle is not found in it. `StringSearch()` would return the length of the buffer in multiples of `sizeof(uint16_t)`, but checking that against `haystack_length` would not work if the latter one was odd. PR-URL: #6511 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

PR-URL: #6511 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

- **buffer**: fix lastIndexOf and indexOf in various edge cases (Anna Henningsen) [#6511](#6511) - **child_process**: use /system/bin/sh on android (Ben Noordhuis) [#6745](#6745) - **deps**: - upgrade npm to 3.8.9 (Rebecca Turner) [#6664](#6664) - upgrade to V8 5.0.71.47 (Ali Ijaz Sheikh) [#6572](#6572) - upgrade libuv to 1.9.1 (Saúl Ibarra Corretgé) [#6796](#6796) - Intl: ICU 57 bump (Steven R. Loomis) [#6088](#6088) - **repl**: - copying tabs shouldn't trigger completion (Eugene Obrezkov) [#5958](#5958) - exports `Recoverable` (Blake Embrey) [#3488](#3488) - **src**: add O_NOATIME constant (Rich Trott) [#6492](#6492) - **src,module**: add --preserve-symlinks command line flag (James M Snell) [#6537](#6537) - **util**: adhere to `noDeprecation` set at runtime (Anna Henningsen) [#6683](#6683) As of this release the 6.X line now includes 64-bit binaries for Linux on Power Systems running in big endian mode in addition to the existing 64-bit binaries for running in little endian mode. PR-URL: #6810

Use `StringBytes::Size` to determine the needle string length instead of assuming latin-1 or UTF-8. Previously, `Buffer.indexOf` could fail with an assertion failure when the needle's byte length, but not its character count, exceeded the haystack's byte length. PR-URL: #6511 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

Fix `buffer.indexOf` for the case that the haystack has odd length and the needle is not found in it. `StringSearch()` would return the length of the buffer in multiples of `sizeof(uint16_t)`, but checking that against `haystack_length` would not work if the latter one was odd. PR-URL: #6511 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

PR-URL: #6511 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

Notable changes: * **buffer**: * Buffer no longer errors if you call lastIndexOf with a search term longer than the buffer (Anna Henningsen) #6511 * deps: * update npm to 2.15.5 (Rebecca Turner) #6663 * http: * Invalid status codes can no longer be sent. Limited to 3 digit numbers between 100 - 999 (Brian White) #6291

keltheceo · 2016-05-20T14:45:20Z

👍 good work

Notable changes: * **buffer**: * Buffer no longer errors if you call lastIndexOf with a search term longer than the buffer (Anna Henningsen) #6511 * contextify: * Context objects are now properly garbage collected, this solves a problem some individuals were experiencing with extreme memory growth (Ali Ijaz Sheikh) #6871 * deps: * update npm to 2.15.5 (Rebecca Turner) #6663 * http: * Invalid status codes can no longer be sent. Limited to 3 digit numbers between 100 - 999 (Brian White) #6291

addaleax added the buffer Issues and PRs related to the buffer subsystem. label May 2, 2016

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label May 2, 2016

addaleax force-pushed the fix-buffer-lastindexof-needle-gt-haystack branch from fb4ec11 to 2d5f775Compare May 2, 2016 02:27

addaleax changed the title ~~buffer: fix lastIndexOf crash for overlong needle~~buffer: fix lastIndexOf and indexOf in various edge casesMay 2, 2016

addaleax force-pushed the fix-buffer-lastindexof-needle-gt-haystack branch from 26596a1 to dc4b946Compare May 2, 2016 06:03

addaleax force-pushed the fix-buffer-lastindexof-needle-gt-haystack branch from dc4b946 to 414877cCompare May 2, 2016 06:08

Fishrock123 assigned trevnorris May 2, 2016

addaleax force-pushed the fix-buffer-lastindexof-needle-gt-haystack branch from 414877c to a06edb1Compare May 2, 2016 14:53

jasnell reviewed May 2, 2016
View reviewed changes

Comment threadsrc/node_buffer.cc Outdated
jasnellMay 2, 2016
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: s/neares/nearest

MylesBorins pushed a commit that referenced this pull request May 16, 2016
src: fix FindFirstCharacter argument alignment
ae13fc7
PR-URL: #6511 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

evanlucas mentioned this pull request May 17, 2016
2016-05-17, Version 6.2.0 (Stable) #6810
Merged

evanlucas pushed a commit that referenced this pull request May 17, 2016
src: fix FindFirstCharacter argument alignment
27c17ce
PR-URL: #6511 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

MylesBorins pushed a commit that referenced this pull request May 18, 2016
src: fix FindFirstCharacter argument alignment
2fac15b
PR-URL: #6511 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

MylesBorins mentioned this pull request May 18, 2016
v4.4.5 proposal #6824
Merged

MylesBorins added the dont-land-on-v4.x label Jun 2, 2016

Uh oh!

Uh oh!

Conversation

addaleax commented May 2, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

jasnell commented May 2, 2016

Uh oh!

addaleax commented May 2, 2016

Uh oh!

addaleax commented May 2, 2016

Uh oh!

jasnell commented May 2, 2016

Uh oh!

addaleax commented May 2, 2016

Uh oh!

jasnell commented May 2, 2016

Uh oh!

jasnell commented May 2, 2016

Uh oh!

addaleax commented May 2, 2016

Uh oh!

jasnell commented May 2, 2016

Uh oh!

addaleax commented May 2, 2016

Uh oh!

jasnell commented May 2, 2016

Uh oh!

addaleax commented May 2, 2016

Uh oh!

jasnell commented May 2, 2016

Uh oh!

addaleax commented May 2, 2016

Uh oh!

jasnell commented May 2, 2016

Uh oh!

jasnell commented May 2, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented May 2, 2016

Uh oh!

addaleax commented May 2, 2016

Uh oh!

jasnell commented May 2, 2016

Uh oh!

jasnellMay 2, 2016

Choose a reason for hiding this comment

Uh oh!

keltheceo commented May 20, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jasnell commented May 2, 2016•
edited
Loading