string_decoder: RangeError introduced by 6.2.1

[gagern]

• Version: 6.2.1
• Platform: Linux 4.6.1-gentoo x86_64 AuthenticAMD GNU/Linux
• Subsystem: string_decoder

The update from 6.2.0 to 6.2.1 broke tests (on Travis and elsewhere) for CindyJS:

_stream_readable.js:65
  // not happen before the first write call.

RangeError: out of range index
    at RangeError (native)
    at StringDecoder.fillLast (string_decoder.js:94:9)
    at StringDecoder.write (string_decoder.js:73:14)
    at readableAddChunk (_stream_readable.js:160:31)
    at ReadStream.Readable.push (_stream_readable.js:130:10)
    at onread (fs.js:1774:12)
    at FSReqWrap.wrapper [as oncomplete] (fs.js:675:17)

The fact that the quoted line is a comment is highly confusing. It makes it hard to get a sound idea as to what's actually going on here. So I bisected the problem to 79ad172 for PR #6777.

So far I haven't managed to come up with a small reproducing example for this. The issue is perfectly reproducible in CindyJS/CindyJS@c685ce4, though. Just run node make forbidden in the top-level directory of a git clone (not a mere tarball download):

nvm install v6.2.1
git clone https://github.com/CindyJS/CindyJS.git
cd CindyJS
git checkout c685ce4
node make forbidden

It will check groups of files against forbidden regular expressions. The high-level task specification is in make/build.js, with the actual command implementation in make/commands.js. If I only operate on files matching globs, I don't see this error message, so the code running git ls-files is probably involved somehow. But even when I extracted those parts into a smaller separate script, I couldn't reproduce the issue, so I'm not sure what other components play a role here.

Note that there will be an error message about an attempt to re-evaluate the fs module, which is due to the outdated version of graceful-fs used by unzip. That is unrelated; uncommenting the require("unzip") line makes it go away while still reproducing the issue. I'll probably switch to something like yauzl soon.

[gagern]

I've got a reproducing example now:

'use strict';
const StringDecoder = require('string_decoder').StringDecoder;
const url = 'https://raw.githubusercontent.com/CindyJS/CindyJS/0d04d273c3/ref/img/RostS5.png';
let chunks = [];
require('https').get(url, res => {
  if (res.statusCode !== 200) throw Error(res.statusMessage);
  res.on('data', d => chunks.push(d));
  res.on('end', haveImg);
});
function haveImg() {
  const buf = Buffer.concat(chunks);
  if (buf.length !== 181909) throw Error("Unexpected size: " + buf.length);
  const decoder = new StringDecoder('utf8');
  decoder.write(buf.slice(0, 65536));
  decoder.write(buf.slice(65536, 131072));
  decoder.write(buf.slice(131072, 181909));
  decoder.end();
}

Yes, I'm feeding a PNG file to an UTF-8 decoder, so I don't expect the result to make much sense. Mostly replacement characters. I'd not expect a throw, though. The chunking appears to be relevant here; feeding the whole file to the decoder in one piece will not throw.

[gagern]

And a smaller reproducing example:

'use strict';
const decoder = new (require('string_decoder').StringDecoder)('utf8');
decoder.write(Buffer.from('c9b5a9', 'hex'));
decoder.write(Buffer.from('41', 'hex'));

[mscdex]

Yes, I see it now. I will have a PR to fix this soon.

I am not sure why the exception message is pointing to the wrong text though (and even the wrong source file).

[gagern]

@mscdex My first guess was that Q.longStackSupport=true might be involved somehow, but that wasn't the case. And the reproducing example didn't reproduce that aspect of the problem either. Feels like another bug, probably unrelated to what we have here. Tracking the source of that location information through v8 internal code is no fun, though…