search

kaitai-io / kaitai_struct_javascript_runtime

Kaitai Struct: runtime for JavaScript
Apache License 2.0
37 stars 22 forks source link

Fix processZlib on Node #12

Closed cherue closed 4 years ago

cherue commented 4 years ago

Use the provided (sub)buffer (with its byteOffset and byteLength) instead of the underlying buffer.

See #3

GreyCat commented 4 years ago

Thanks for this! Any chance we can create a test in tests repo that will demonstrate the problem, so we'll have understanding of what exactly this fixes?

cherue commented 4 years ago

Yeah I'll write a test for it. What needs to be tested is process: zlib surrounded by some other data. Let's call it zlib_surrounded?

cherue commented 4 years ago

This can be merged now and then #3 can be closed.

GreyCat commented 4 years ago

CI run with test went through, everything (sort of) works, except for JavaScript — as expected.

Next step: merging the fix.

GreyCat commented 4 years ago

Ok, this PR brough breakage of both zlib-related tests: see https://ci.kaitai.io/

  Not a string or buffer
TypeError: Not a string or buffer
    at zlibBufferSync (zlib.js:235:11)
    at Object.exports.inflateSync (zlib.js:163:10)
    at Function.KaitaiStream.processZlib (/home/travis/build/kaitai-io/ci_targets/runtime/javascript/KaitaiStream.js:607:31)
    at ZlibSurrounded._read (/home/travis/build/kaitai-io/ci_targets/compiled/javascript/ZlibSurrounded.js:23:35)
    at new ZlibSurrounded (/home/travis/build/kaitai-io/ci_targets/compiled/javascript/ZlibSurrounded.js:18:10)
    at /home/travis/build/kaitai-io/ci_targets/tests/helpers/javascript/testHelper.js:12:17
    at FSReqWrap.readFileAfterClose [as oncomplete] (fs.js:380:3)

Can I ask you to take a look?

cherue commented 4 years ago

All zlib tests are now failing on nodejs4 and nodejs7.

Node's zlib.inflateSync documentation's history section shows that support for Uint8Array argument was added in Node v8.0.0.

I'll fix that and make another PR against the current runtime? Or should the fix be reverted first?

GreyCat commented 4 years ago

If you can do it relatively quickly — please just do a fix. If that will take more time, I can revert this PR.