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

Wireshark pedantry fixes #128

Merged
merged 1 commit into from
Jun 20, 2024

Conversation

Boolean263
Copy link
Contributor

Changes that hopefully bring the Wireshark plugin closer to the standard against which core Wireshark dissectors are held. Tested on an in-tree Linux build of Wireshark 4.2's master branch.

Change return type of timediff() from int to long to prevent compiler warning about shortening a long into an int. (Such warnings are fatal for core dissectors.)

Tweak printf-style functions which use the return value of timediff() to use the PRId64 format specifier, due to the previous change.

Add include of <wireshark.h> (see which for its use). Remove redundant and unused header includes.

Replace "forbidden" API call (as defined by Wireshark's tools/checkAPIs.pl) isascii() with g_ascii_isalnum(), and replace isspace() with g_ascii_isspace().

Replace uses of get_value_ptr(field)->value.uinteger with fvalue_get_uinteger(get_value_ptr(field)) for consistency with other places in this plugin, and to insulate against recent Wireshark changes which have removed the value.uinteger member in favour of value.uinteger64.

With these changes, the plugin compiles without warnings, and passes all of the checks made by Wireshark's tools/check_dissector.py except for tools/checkfiltername.pl which doesn't like that the field names all start with ja4.ja4 ; this seems acceptable given JA4's naming conventions.

Changes that hopefully bring the Wireshark plugin closer to the standard
against which core Wireshark dissectors are held. Tested on an in-tree
Linux build of Wireshark 4.2's `master` branch.

Change return type of `timediff()` from `int` to `long` to prevent
compiler warning about shortening a `long` into an `int`. (Such warnings
are fatal for core dissectors.)

Tweak printf-style functions which use the return value of `timediff()`
to use the `PRId64` format specifier, due to the previous change.

Add include of `<wireshark.h>` (see which for its use). Remove redundant
and unused header includes.

Replace "forbidden" API call (as defined by Wireshark's
`tools/checkAPIs.pl`) `isascii()` with `g_ascii_isalnum()`, and replace
`isspace()` with `g_ascii_isspace()`.

Replace uses of `get_value_ptr(field)->value.uinteger` with
`fvalue_get_uinteger(get_value_ptr(field))` for consistency with other
places in this plugin, and to insulate against recent Wireshark changes
which have removed the `value.uinteger` member in favour of
`value.uinteger64`.

With these changes, the plugin compiles without warnings, and passes all
of the checks made by Wireshark's `tools/check_dissector.py` except for
`tools/checkfiltername.pl` which doesn't like that the field names all
start with `ja4.ja4` ; this seems acceptable given JA4's naming
conventions.
Copy link
Collaborator

@noeltimothy noeltimothy 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 these changes. I tested them and they actually fix some wrong timestamps in the JA4T retransmit time calculation as well.
Can be merged into main.

@igr001-galactica igr001-galactica merged commit a02175f into FoxIO-LLC:main Jun 20, 2024
4 checks passed
Boolean263 added a commit to Boolean263/ja4 that referenced this pull request Dec 10, 2024
Fix a crash when fetching `ssh.direction` caused by using the wrong
function in FoxIO-LLC#128. `fvalue_get_uinteger()` doesn't support the
`FT_BOOLEAN` field, but `fvalue_get_uinteger64()` does.

Fix a crash in `locate_tree()` when the first child of the passed-in
tree is non-null but has a null `finfo` member.

These fixes allow the plugin to pass the test suite on the current main
branch (`master`) of Wireshark when built as an in-tree plugin.
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.

3 participants