Out of Memory Error in parse()

[donmahallem]

The following script result in an out of memory error where the image header suggest a far larger file that in the data provided. The runtime snapshot below should provide some additional information. I haven't had time to dive deep into the code but I would suggest an early break if the offset in the parse method clearly exceeds the provided data length. The offset in this case is 726 when the Memory runs out and only 21 bytes are provided.

const jpeg = require("jpeg-js");
const buf = new Buffer("/9j/wSHEb88b4GDuAxgXZDQ5YzQx", "base64");
jpeg.decode(buf);

Same is valid for encoded buffer:

/9j/wK1WotT/4YD//3/a7H322Q==

[patrickhulce]

Possibly fixed by https://github.com/eugeneware/jpeg-js/pull/54 ?

[donmahallem]

Will try to manually patch that PR and will report back later. But imho this is an indirect fix(if it works) as I said as it does look like the parser does read far more data than there is in the file. If one manages to stay below 64megapixel it will pass this test and still reads "no data"

[patrickhulce]

It would indeed be indirect since #54 protects against OOM whether a buffer of that length has been provided or not while this fix would be more targeted to data provided in a buffer. That'd be welcome if you're so inclined :)

[donmahallem]

Checked and it trips the Exception of #54. It's up to you if you close this PR. Personally I would still probably add another safeguard for the behavior described above as staying just below the limit could yield pretty intensive resource allocation on servers with a very small payload. Valid pictures that large would atleast be limiting themself by their "size" alone.

[patrickhulce]

yield pretty intensive resource allocation on servers with a very small payload

FWIW, I would strongly recommend to avoid using this library on any server for processing user input. Anywhere that node is an option, you'd want a non-blocking C++ implementation of JPEG decoding (like sharp)

But yes that improvement would be welcome either way, I'll leave this issue open. 👍

[donmahallem]

I totally agree. I was originally was tinkering with jimp as I see it being used in tutorials etc. for web apps. This was just an error I traced back to this module.

PS: I did find several TypeErrors inside this module too which raise if asked to parse invalid input. Like the SOS block being the first fileMarker and decodeScan crashing due to frame being undefined. Would you like me to report those too (and corresponding PRs if I can figure it out myself?)?

[patrickhulce]

Would you like me to report those too

Thanks for the offer @donmahallem! IMO, we shouldn't really bother hunting down the individual failure modes. At best we're providing a slightly more specific error message, but a master try-catch and rethrow as jpeg-js failed to decode invalid JPEG data would essentially provide the same level of actionable detail for most of our users. That would be a pretty easy PR if you agree and think it'd help :)

[patrickhulce]

54 is our strategy to address these sorts of issues moving forward, the memory limit is now customizable 🎉