-
Notifications
You must be signed in to change notification settings - Fork 74
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Check if index is negative before incrementing #75
Conversation
This one is related to 101arrowz#71 :) After the `setTiemout` issues was sorted I found antoher one. In the hMap function there is a moment where the program may try to access a negative index. Although in Node.js and browser that does not raise an error, in QML it does raise a TypeError. Good thing is after this fix it all works over there. I was not able to run the complete fflate test suite in my machine, so hopefully this works fine.
Co-authored-by: Manu MA <manu.valladolid@gmail.com>
Also, if negative index access is not allowed, I'm assuming out-of-bounds index access in general isn't allowed, but |
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 For the first line you mentioned, in case 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 :) |
The line numbers actually got messed up because my link wasn't tied to a specific commit, so after updating (() => {
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. |
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 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 With this I am fine with dropping this change altogether, I'll just remove the |
@101arrowz if the access to negatve number is fairly common, this might be even a perf improvemnt, no? |
This one is related to #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 aTypeError
. 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: