search

kaitai-io / kaitai_struct_java_runtime

Kaitai Struct: runtime for Java
MIT License
42 stars 18 forks source link

Problem with the function readBytesTerm in "ByteBufferKaitaiStream.java" #35

Closed ranitg closed 3 years ago

ranitg commented 3 years ago

Hi,

The line: int c = bb.get(); in the function readBytesTerm in "ByteBufferKaitaiStream.java"

This should read into an unsigned int variable.

In the kaitai web IDE the code works as: terminator: 0xfe

but in this project: terminator: -2 (the byte is read as -2 (int) instead of 254 (unsigned int))

This is an inconsistency.

Thanks!

generalmimon commented 3 years ago

Good catch! I came to the conclusion that the signature of method readBytesTerm:

https://github.com/kaitai-io/kaitai_struct_java_runtime/blob/e1168b06391394fd6ddefcb5af9a62f2d516f1ed/src/main/java/io/kaitai/struct/KaitaiStream.java#L281

should be changed to accept byte in place of the term parameter (not int). It just makes more sense and conveys the intent better - terminator isn't allowed to be anything else than a byte value. In addition, methods for applying the terminator and pad-right properties on already parsed byte arrays (which arise when you use size or size-eos: true) also use the byte type to accept byte values:

https://github.com/kaitai-io/kaitai_struct_java_runtime/blob/e1168b06391394fd6ddefcb5af9a62f2d516f1ed/src/main/java/io/kaitai/struct/KaitaiStream.java#L301

https://github.com/kaitai-io/kaitai_struct_java_runtime/blob/e1168b06391394fd6ddefcb5af9a62f2d516f1ed/src/main/java/io/kaitai/struct/KaitaiStream.java#L308

On the other hand, the processXor method for a single value uses int for the byte key (which is especially strange because the processXor for a multi-byte key is using byte[]):

https://github.com/kaitai-io/kaitai_struct_java_runtime/blob/e1168b06391394fd6ddefcb5af9a62f2d516f1ed/src/main/java/io/kaitai/struct/KaitaiStream.java#L349

https://github.com/kaitai-io/kaitai_struct_java_runtime/blob/e1168b06391394fd6ddefcb5af9a62f2d516f1ed/src/main/java/io/kaitai/struct/KaitaiStream.java#L365

So I think it's reasonable to normalize everything to use byte everywhere.

There is a question of breaking backward compatibility, since we'll need to adjust the compiler to inject the (byte) 254 type castings (and the generated Java parsers intended for older runtime versions (without the narrowing type conversion) will trigger a compile-time error), but I believe it's not much of an issue - anyone can just grab the latest compiler and update the parser in minutes.