Skip to content
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

Make "contents" check available for individual bits? #380

Open
ams-tschoening opened this issue Mar 15, 2018 · 7 comments
Open

Make "contents" check available for individual bits? #380

ams-tschoening opened this issue Mar 15, 2018 · 7 comments

Comments

@ams-tschoening
Copy link

I have one example, and only one currently, so might not be worth spending time at all, where a bit field is specified to always be 0. Because of that I thought of making that clear on runtime using contents, but that only works on bytes and because it's a bit field with additional bits set to unknown values, I can't provide a full byte to check against. Additionally, at least the Java runtime's ensureFixedContents reads data on it's own and therefore doesn't support reading individual bits.

My example is pretty simple:

- id:       is_res
  type:     b1
  #contents: [0x00]
- id:       is_prm
  type:     b1
- id:       fcb_or_acd_raw
  type:     b1
- id:       fcv_or_dfc_raw
  type:     b1
- id:       function_raw
  type:     b4

From my understanding we have at least two choices: One would be to define a different syntax for contents and bit-fields, e.g. contents-bits, so that it can be mapped to a different implementation which behaves the same like now, only using bit-reading logic instead of that for bytes. The benefit of the current approach is that an exception is thrown before storing any invalid data in the parser.

OTOH I thought it might be a good idea to simply remove the reading logic entirely from ensureFixedContents and instead change to a approach first reading the data and storing it into the parser and afterwards ensuring it's content by forwarding both the read and expected data to compare. Bytes and bits should map to byte[] in both cases. This doesn't make any difference regarding stream position etc. when the exception is thrown, but parser state is different, because it would now contain invalid data. The good thing is that one wouldn't need to find an additional syntax for contents and bits.

What do you think?

@arekbulski
Copy link
Member

It might be useful to make constants work with any type.

@ams-tschoening
Copy link
Author

I have one example, and only one currently, so might not be worth spending time at all, where a bit field is specified to always be 0.

Found another case and would simply like to document that one as well, because it spans multiple bits and might be of interest:

7.2.4.2 Configuration Field for Security Mode 0
The structure of the Configuration Field of Mode 0 is identical to Security Mode 5 (see Table 22). The M and N has to be set to 00h to indicate that no encryption is applied.[...]

https://oms-group.org/fileadmin/files/download4all/specification/Vol2/4.1.2/OMS-Spec_Vol2_Primary_v412.pdf

@GreyCat
Copy link
Member

GreyCat commented Apr 10, 2018

I believe something like contents-bits is the way to go here. We also have plans to have validations (i.e. as in #81), but that implementation has one important downside: it requires extra effort when doing serialization. Something like contents is good because it's very dumb: it's exactly these bytes/bits and nothing else, both on reading and on writing.

@Barracuda72
Copy link

Barracuda72 commented Apr 10, 2020

Oh, I wanted to create a new issue, even wrote some text, but just as I was ready to click "Submit new issue" Github showed me this one, and I missed it then searching... Github algorithms is definitely smarter than me.

Still, I'll put my text here, because I don't want to just throw it away. Let's say it counts as "vote for".

Suppose I'm reversing a format and there is a field I think is "reserved", i.e. always zero (or some other value), and I want to enforce this assumption. If this field has size in full bytes, I'd use contents and do the following:

meta:
  id: test1
  file-extension: t1
seq:
  - id: flag_one
    type: u1
  - id: flag_two
    type: u1
  - id: reserved
    contents: [ 0, 0 ]

But if I'm working with the bitfields, it seems I can't do the same, as contents treats it's elements either as bytes or strings (byte arrays).

meta:
  id: test2
  file-extension: t2
seq:
  - id: flag_one
    type: b1
  - id: flag_two
    type: b1
  - id: reserved
    #type: b6
    contents: [ ? ]

Of course, I can use instances and write something like:

meta:
  id: test2
  file-extension: t2
seq:
  - id: flag_one
    type: b1
  - id: flag_two
    type: b1
  - id: reserved
    type: b6
instances:
    reserved_is_valid:
        value: reserved == 0

But that doesn't feel quite right and only makes sense with more complicated cases than just 0 as I still will be required to check that field manually.

contents-bits would definitely be the way to go here:

meta:
  id: test2
  file-extension: t2
seq:
  - id: flag_one
    type: b1
  - id: flag_two
    type: b1
  - id: reserved
    contents-bits: [ 0b000000 ] # Or something like that

@generalmimon
Copy link
Member

generalmimon commented Apr 10, 2020

@Barracuda72 I think the valid key (#435) implemented in the current compiler can serve this purpose:

meta:
  id: valid_bits
seq:
  - id: flag_one
    type: b1
  - id: flag_two
    type: b1
  - id: reserved
    type: b6
    valid: '0b000000'

You can just copy/paste this to the devel Web IDE and check if it works. But please just make sure that you wrap the 0b* number in quotes (valid: '0b*' or "0b*"), because you never know if different YAML parsers (used for extracting a tree that KSC needs) can correctly handle this situation - they may interpret anything starting with 0b as 0 - that's not what you'd expect from valid: 0b100000. Unfortunately, a quick test shows that the YAML parser used by the Web IDE is indeed affected by this issue.

@Barracuda72
Copy link

@Barracuda72 I think the valid key (#435) implemented in the current compiler can serve this purpose:

Wow, thanks! That's exactly that I need! Didn't know it existed, as it's not in the docs (yet, I suppose).

@Barracuda72
Copy link

Barracuda72 commented Apr 11, 2020

Hmm, I think I found another issue (not sure it's a bug or just feature).

It seems that Kaitai treats one-bit bitfields differently from multi-bit ones. I actually knew this for a while, and even used it sometimes in 'if:' clauses, but it didn't become clear for me up until now.

The problem is, following works:

meta:
  id: test1
  file-extension: t1
seq:
  - id: flag_one
    type: b1
  - id: flag_two
    type: b1
  - id: reserved
    type: b2
    valid: '0b00'

but the following doesn't:

meta:
  id: test2
  file-extension: t2
seq:
  - id: flag_one
    type: b1
  - id: flag_two
    type: b1
  - id: reserved
    type: b1
    valid: '0b0'

Here I've got "TypeMismatchError: can't compare BitsType1 and Int1Type(true)".

It's as if "b1" is treated like "boolean", which is surprising, because none of other integer / bitfield types can't be implicitly used as boolean values. There should be a reason for this, I suspect, but as of now it doesn't look very logical to me.

Maybe I'm missing something?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants