jashkenas / underscore

JavaScript's utility _ belt
https://underscorejs.org
MIT License
27.33k stars 5.53k forks source link

isEqual not producing correct result when comparing array of Buffers in 1.11.0 #2875

Closed erossignon closed 4 years ago

erossignon commented 4 years ago
const _u = require("underscore");
const _l = require("lodash");

const a = [Buffer.from([1, 2, 3]), Buffer.from([1, 2, 3])];
const b = [Buffer.from([4, 5, 6]), Buffer.from([4, 5, 7])];

console.log(_u.isEqual(a, b)); // True => Wrong
console.log(_l.isEqual(a, b)); // False => Correct
jgonggrijp commented 4 years ago

@erossignon Thanks for reaching out. The expected behavior in 1.10 was by accident rather than by design, since we didn't support Buffer yet in that version, but I agree that the new behavior in 1.11 is wrong.

The issue also manifests with direct Buffer-to-Buffer comparisons:

_.isEqual(Buffer.from([1]), Buffer.from([2])) // true
_.isEqual(Buffer.from([1]), Buffer.from([1, 2])) // true

At first sight, it seems as if Buffer instances always compare equal in Node.js. Upon closer investigation, I found that the situation is a little bit more nuanced.

Under the hood, a Node.js Buffer is actually an Uint8Array with some quirks. In particular, Buffers created from small arrays are allocated on a common underlying ArrayBuffer as an optimization:

const a = Buffer.from([1]);
const b = Buffer.from([2]);
a.buffer === b.buffer // true
a.byteOffset === b.byteOffset // false

It so happens that our new support for typed arrays correctly identifies Node.js Buffer as such. _.isEqual then continues with a recursive comparison of the underlying buffers, but it doesn't take the offsets into account, so views of the same ArrayBuffer always compare equal even if they view different parts of the buffer.

I'll work on a fix.