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.

Sign up for GitHub

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

Jump to bottom

read_bytes_term() should raise EOFError instead of Exception #41

Open
overfl0 opened this issue Feb 26, 2020 · 2 comments · May be fixed by #80
Open

read_bytes_term() should raise EOFError instead of Exception #41

overfl0 opened this issue Feb 26, 2020 · 2 comments · May be fixed by #80
Labels
enhancement

Comments

@overfl0
Copy link

overfl0 commented Feb 26, 2020

read_bytes_term() should raise EOFError instead of Exception to be consistent with read_bytes().

Btw. it's funny that found this issue 1h later than the person that has reported #40

Even if #40 is implemented using a custom class, I'd suggest having both read_bytes_term() and read_bytes() raise the same type of error.

My use case is that I'm trying to detect when the packet that I'm parsing has been truncated so it would be helpful to be able to just do except EOFError (or except kaitaistruct.EOFError) instead of having to parse the exception message.
armijnhemel added a commit to armijnhemel/binaryanalysis-ng that referenced this issue Apr 13, 2022
@armijnhemel
romfs: catch generic exception, as kaitai struct uses generic exceptions
8b69824 
in some cases instead of custom exceptions, see kaitai-io/kaitai_struct_python_runtime#41
@generalmimon
Copy link
Member

generalmimon commented Sep 8, 2024 •
edited
Loading

I agree that if the terminator is required and doesn't occur before EOF, the file appears to be truncated and therefore an EOFError-compatible error should be thrown. I just wonder if it would make sense to create a subclass of EOFError specifically for the use in read_bytes_term(), because it feels a bit like a special case to me. Usually it's quite obvious whether an EOFError will be thrown, because it just depends on how many bytes are requested (e.g. read_u4le requests 4 bytes) vs. how many bytes are actually available in the stream.

With read_bytes_term(), it's different - you don't know in advance how many bytes it will request, because that depends on when (or if) the terminator byte occurs. Therefore, I think the error message should continue to reflect that this is a "end of stream reached, but no terminator %d found" error, not really a "requested {} bytes, but only {} bytes available" error.

Another reason why I believe this is a slightly different error than a bare EOFError is that the error thrown by read_bytes_term() can actually be suppressed using eos-error: false (https://doc.kaitai.io/user_guide.html#eos-error). This is not possible with normal EOFErrors. So if a user receives a more specific error than EOFError in this case, they can more easily realize that they actually want the eos-error: false behavior.

In PHP, I added an exception class NoTerminatorFoundError and I think @GreyCat liked it (kaitai-io/kaitai_struct_php_runtime#6 (comment)), so perhaps it's not a bad idea.

Does anybody (@GreyCat, @armijnhemel, @dgelessus) have an opinion on this?

@armijnhemel
Copy link

Does anybody (@GreyCat, @armijnhemel, @dgelessus) have an opinion on this?

Personally I am agnostic to this, but it might be good to keep things consistent across the different languages. Also, if there is more information available (which seems to be the case here), then there might be users that would like to know. A subclass of EOFError would then be the right choice.

generalmimon added a commit that referenced this issue Sep 15, 2024
@generalmimon
Adopt a system of exceptions derived from KaitaiStructError
2a63b4f 
* Resolves
  #40
* Resolves
  #41

As explained in
#40,
this makes it easy to handle to all errors caused by invalid input data
by using `kaitaistruct.KaitaiStructError` in a `try..except` statement.
Three new exception types were added: `InvalidArgumentError`,
`EndOfStreamError` and `NoTerminatorFoundError`. All changes to raised
exceptions in this commit should be backward compatible, as we are only
moving to subclasses of previously raised exceptions.
`NoTerminatorFoundError` is a subclass of `EndOfStreamError` to address
the suggestion in
#41.

Note that the `process_rotate_left` method could only raise
`NotImplementedError` if someone called it manually (because
KSC-generated parsers hardcode `group_size` to `1`, see
https://github.com/kaitai-io/kaitai_struct_compiler/blob/c23ec2ca88d84042edba76f70c1f003d062b7585/shared/src/main/scala/io/kaitai/struct/languages/PythonCompiler.scala#L211),
so it makes no sense to raise an exception derived `KaitaiStructError`
(it's a programmer error, not a user input error). Most of our runtime
libraries in other languages don't even have this `group_size`
parameter, and if they do (C#, Java, Ruby), they also throw the
equivalent of `NotImplementedError` (except the JavaScript runtime,
which throws a plain string, which is _possible_ in JS but considered
bad practice, so we should fix this).
@generalmimon generalmimon linked a pull request Sep 15, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Adopt a system of exceptions derived from KaitaiStructError kaitai-io/kaitai_struct_python_runtime
3 participants
@overfl0 @armijnhemel @generalmimon