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

Improve TzIf format detection #237

Merged
merged 6 commits into from
Feb 2, 2022
Merged

Improve TzIf format detection #237

merged 6 commits into from
Feb 2, 2022

Conversation

n-vr
Copy link
Contributor

@n-vr n-vr commented Jan 24, 2022

Added the four-octet ASCII sequence to identify TzIf files: 0x54 0x5A 0x69 0x66
Source

This pull request fixes #214

@coveralls
Copy link

coveralls commented Jan 24, 2022

Coverage Status

Coverage remained the same at 96.407% when pulling 64aff2c on n-vr:improve-tzif-detection into 272584c on gabriel-vasile:master.

@n-vr n-vr marked this pull request as ready for review January 24, 2022 11:13
Copy link
Owner

@gabriel-vasile gabriel-vasile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, thank you for taking your time to contribute.

The problem I tried to describe in #214 is that the detection is too loose. Any simple text file that happens to start with TZif is detected as application/tzif (see the commit I added on this PR, it shows the problem.)

https://tools.ietf.org/id/draft-murchison-tzdist-tzif-00.html#rfc.section.3 describes in detail the format of the tzif file. For example, fifth byte, version, must be 0x00, 0x32 or 0x33, charcnt bytes must not be 0, etc.
These are the checks that we can do for tzif files to make the detection more accurate.

@n-vr
Copy link
Contributor Author

n-vr commented Jan 31, 2022

Hello @gabriel-vasile,

Thanks for creating a test case. I did not fully understand the problem, and I think I do now. I created some additional checks:

  • File length at least 44 bytes (length of full header)
  • Version MUST be 0x00, 0x32 or 0x33
  • typecnt MUST not be zero

Additional header checks:
- Header length
- Version
- typecnt MUST not be zero
@n-vr
Copy link
Contributor Author

n-vr commented Jan 31, 2022

Sorry for overwriting twice, my signature was not working properly because of a new system install.

Copy link
Owner

@gabriel-vasile gabriel-vasile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, code looks better but there are still some issues to fix. I left some suggestions.

You can commit how many times you want, there is not problem.

internal/magic/binary.go Outdated Show resolved Hide resolved
mimetype_test.go Outdated Show resolved Hide resolved
n-vr and others added 3 commits February 1, 2022 08:54
Co-authored-by: Gabriel Vasile <[email protected]>

Co-authored-by: Gabriel Vasile <[email protected]>
Code is easier to read and faster to run.

Co-authored-by: Gabriel Vasile <[email protected]>
Test case was used to show the problem
when detecting a TzIf file.
The test is no longer needed.
Copy link
Owner

@gabriel-vasile gabriel-vasile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution!

@gabriel-vasile gabriel-vasile merged commit 98e3221 into gabriel-vasile:master Feb 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Time zone info format detection could be more accurate
3 participants