Check if index is negative before incrementing

[brizental]

This one is related to https://github.com/101arrowz/fflate/issues/71 :) After the setTimeout issue was sorted I found another minor one.

In the hMap function there is a moment where the program may try to access a negative index. Although in Node.js and browsers that does not raise an error, in QML it does raise a TypeError. Good thing is: after this fix it all works over there too.

I was not able to run the complete fflate test suite in my machine, so hopefully this works fine. The actual behaviour of the code does not change here, I just changed the code path a little bit to avoid attempting to access a negative index.

AH! And for reference, this is the error I got while trying to run yarn test on my machine:

 FAIL  compression  "image"
    Cannot find module '/Users/beatrizmachado/Projects/fflate/lib/index.cjs'. Please verify that the package.json has a valid "main" entry

[101arrowz]

hMap is used a lot and is a significant part of the time spent in both compression and decompression. Adding a conditional could screw with the JIT a bit, so I'll look for another solution before merging.

Also, if negative index access is not allowed, I'm assuming out-of-bounds index access in general isn't allowed, but fflate does that a lot as well. You should check thoroughly to ensure that this is the only problem that occurs in QML; I think that this line and this line are also potentially problematic.

[101arrowz]

I checked around and it seems there was no better solution to avoid negative index access, so for the time being I'll merge. I won't publish a new version until you confirm that the other lines I mentioned present no issue in QML.

[brizental]

I checked around and it seems there was no better solution to avoid negative index access, so for the time being I'll merge. I won't publish a new version until you confirm that the other lines I mentioned present no issue in QML.

I do not think we are hitting issues with neither line. I performed manual testing and all seems to be working with the latest master.

For the first line you mentioned, in case mb is negative we will get a RangeError from JavaScript. That is not exclusive to QML though, that error is thrown in browsers and Node.js too. I don't think that line is an issue in this case. I am sure if that bug was present it would have manifested before :)

For the second line, do you mean that index could be negative as well? I did not hit any issues in my manual testing. I will file a bug to validate that gzipping is working as expected in the wild, once the version with gzipping is released. If I see there are many errors, I will come back here to let you know.

For now, I think it is safe to release a new version with the current changes.

Thanks a lot for giving this issues such attention :)

[101arrowz]

The line numbers actually got messed up because my link wasn't tied to a specific commit, so after updating master they shifted. The first line is the main one I'm concerned about: on the first iteration fflate will necessarily access le[-1] and l[-1]. I don't have access to the V4 JavaScript engine (which Qt uses AFAIK), so I'd like you to confirm that out-of-bounds index access and setting don't matter. I'll publish a new version as soon as I can, but in the meantime try out these test cases and let me know the output (i.e. which ones error):

(() => {
  var buf = new Uint8Array(10);
  var testInd = 0;
  var test = function(fn) {
    testInd++;
    try { fn(); }
    catch (err) { console.log('Test ' + testInd + ' failed'); }
  }
  test(function() { buf[-1] = 1; });
  test(function() { buf[10] = 1; });
  test(function() { buf[NaN] = 1; });
  test(function() { buf[0] = NaN; if (buf[0] !== 0) throw ''; });
  test(function() { buf[-1]++; });
  test(function() { ++buf[-1]; });
  test(function() { var x = undefined; var y = x + 1; });
  test(function() { var x = undefined; ++x; });
})();

These results will help determine if there are any possible further issues.

[brizental]

Hm, plot twist here. Actually all the test cases pass. I am a bit embarrassed, but my initial hypothesis that QML threw an error if the index was negative is incorrect.

Still, we got a mystery here. See, not even this code:

   (() => {
      var mb = 8;
      var l = new Uint16Array(mb);
      var cd = new Uint8Array(273);
      let i = 0;
      for (; i < cd.length; ++i)
        ++l[cd[i] - 1];
    })();

Throws an error by itself.

This line does throw an error everytime without the conditional. The error being Type Error undefined, not the most helpful error message either.

Honestly I have no idea what is going on there. The conditional does fixes it, so I appreciate keeping it. However I understand if you prefer to remove it :)

[UPDATE]

Of course, 10 seconds after sending this message I realized what might be the problem and it really was it. Webpack adds a "use strict" statement at the top of my compiled lib. That makes the code throw when it finds a negative index. Removing it makes everything work again.

With this I am fine with dropping this change altogether, I'll just remove the "use strict" from my own lib. Thanks for support, again :)

[manucorporat]

@101arrowz if the access to negatve number is fairly common, this might be even a perf improvemnt, no? https://v8.dev/blog/elements-kinds#avoid-reading-beyond-the-length-of-the-array