Feat/packet inspector query tools#30
Open
dipeshwalia wants to merge 5 commits into
Open
Conversation
Avoid duplicate decode-as default reapplication across module traversal and keep PREF_DECODE_AS_RANGE updates merged with protocol defaults to prevent runaway memory growth on Node 20.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis PR adds three new data-extraction APIs to Wiregasm: ChangesExtract Fields, List Present Fields & Protocol Hierarchy
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/wiregasm/lib.cpp`:
- Around line 1097-1099: When wtap_seek_read(cfile->provider.wth,
fdata->file_off, &rec, &rec_buf, &err, &err_info) returns false, the loop
currently breaks without setting response.error causing callers to get
incomplete data; modify the failure path in the block around wtap_seek_read (and
the analogous blocks that call wtap_seek_read at the other two locations) to
populate response.error with the available error information (use err and/or
err_info contents or formatted message) and return/exit early instead of
silently breaking so the caller receives the read failure details.
- Around line 1281-1289: The protocol node counts are being incremented multiple
times per single frame because every FT_PROTOCOL hfinfo match increments
protocol->frames and protocol->bytes; fix this by preventing duplicate
increments within the same frame — e.g., add a per-protocol last-seen frame
marker (a field like ProtocolNode::last_seen_frame_id or last_frame_index) and
update it when you first account for that protocol in the current frame; in the
block where you get ProtocolNode* protocol via wg_find_or_create_protocol_node,
only increment protocol->frames and protocol->bytes if
protocol->last_seen_frame_id != current_frame_id (then set last_seen_frame_id =
current_frame_id), or alternatively maintain a per-frame set of counted protocol
pointers and skip duplicates, ensuring the logic uses the same current frame
identifier used elsewhere in this code path.
- Around line 1025-1044: FieldValue.raw is never set, so populate it with the
original string representation: when finfo->value is present call
fvalue_to_string_repr as you do, capture the returned char* into a temporary
std::string (e.g. std::string repr(display)), assign field_value.value = repr
(or move), and also assign field_value.raw = repr before calling wmem_free(NULL,
display); and in the label fallback branch set field_value.raw =
field_value.value (or the label string) so raw is filled for every pushed
FieldValue; update references to field_value, fvalue_to_string_repr, and
values->push_back accordingly.
In `@src/index.ts`:
- Around line 271-274: The code currently constructs a RegExp from untrusted
query (const regex = new RegExp(query)) and tests field.field/field.name, which
fails for inputs like (?i)http and is vulnerable to ReDoS; replace the regex
approach with a case-insensitive substring match: normalize the query to a
string and lower-case it (e.g., const q = String(query).toLowerCase()) and
filter using field.field.toLowerCase().includes(q) ||
field.name.toLowerCase().includes(q) so no RegExp is compiled and matching is
safe and case-insensitive.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fb7fba74-67c0-4e9c-9342-508a2a30bfec
📒 Files selected for processing (8)
lib/wiregasm/bindings.cpplib/wiregasm/lib.cpplib/wiregasm/lib.hlib/wiregasm/wiregasm.cpplib/wiregasm/wiregasm.hsrc/index.test.tssrc/index.tssrc/types.ts
Comment on lines
+1025
to
+1044
| FieldValue field_value; | ||
| field_value.field = field_it->second; | ||
|
|
||
| if (finfo->value) { | ||
| char *display = fvalue_to_string_repr(NULL, finfo->value, FTREPR_DISPLAY, finfo->hfinfo->display); | ||
| if (display) { | ||
| field_value.value = display; | ||
| wmem_free(NULL, display); | ||
| } | ||
| } | ||
|
|
||
| if (field_value.value.empty()) { | ||
| char label_str[ITEM_LABEL_LENGTH]; | ||
| label_str[0] = '\0'; | ||
| proto_item_fill_label(finfo, label_str); | ||
| field_value.value = string(label_str); | ||
| } | ||
|
|
||
| values->push_back(field_value); | ||
| } |
There was a problem hiding this comment.
FieldValue.raw is never populated.
Line 1025–1044 fills field/value only. raw remains empty for all results, which makes the exposed response shape misleading.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/wiregasm/lib.cpp` around lines 1025 - 1044, FieldValue.raw is never set,
so populate it with the original string representation: when finfo->value is
present call fvalue_to_string_repr as you do, capture the returned char* into a
temporary std::string (e.g. std::string repr(display)), assign field_value.value
= repr (or move), and also assign field_value.raw = repr before calling
wmem_free(NULL, display); and in the label fallback branch set field_value.raw =
field_value.value (or the label string) so raw is filled for every pushed
FieldValue; update references to field_value, fvalue_to_string_repr, and
values->push_back accordingly.
Comment on lines
+1097
to
+1099
| if (!wtap_seek_read(cfile->provider.wth, fdata->file_off, &rec, &rec_buf, &err, &err_info)) { | ||
| break; | ||
| } |
There was a problem hiding this comment.
Surface read failures instead of returning partial success silently.
On Line 1097, Line 1213, and Line 1319, read failures break the loop but never populate response.error, so callers can receive incomplete data with an empty error string.
Suggested fix
- if (!wtap_seek_read(cfile->provider.wth, fdata->file_off, &rec, &rec_buf, &err, &err_info)) {
- break;
- }
+ if (!wtap_seek_read(cfile->provider.wth, fdata->file_off, &rec, &rec_buf, &err, &err_info)) {
+ response.error = err_info ? string(err_info) : "Failed to read packet data";
+ break;
+ }Also applies to: 1213-1215, 1319-1321
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/wiregasm/lib.cpp` around lines 1097 - 1099, When
wtap_seek_read(cfile->provider.wth, fdata->file_off, &rec, &rec_buf, &err,
&err_info) returns false, the loop currently breaks without setting
response.error causing callers to get incomplete data; modify the failure path
in the block around wtap_seek_read (and the analogous blocks that call
wtap_seek_read at the other two locations) to populate response.error with the
available error information (use err and/or err_info contents or formatted
message) and return/exit early instead of silently breaking so the caller
receives the read failure details.
Avoid compiling untrusted present-field queries as regex by using case-insensitive substring matching with (?i) compatibility, and prevent protocol frames/bytes over-counting when a protocol appears multiple times in a single frame.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
lib/wiregasm/lib.cpp (1)
1272-1300:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPer-frame protocol de-duplication still resets across recursion branches.
counted_protocolsis recreated on every recursive call. If the same protocol is reached twice under the same parent via different non-protocol subtrees in one frame,frames/bytescan still be incremented twice. Carry the same set through same-level recursion and only start a fresh one when descending into a child protocol'schildren.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/wiregasm/lib.cpp` around lines 1272 - 1300, The counted_protocols set is recreated per call causing duplicate counting across different branches at the same hierarchy level; modify wg_collect_protocol_hierarchy to accept and propagate a reference (or pointer) to a counted_protocols set for the current sibling-level so that when iterating siblings you reuse the same set, and only create/pass a fresh set when recursing into a protocol node's children (i.e., when calling wg_collect_protocol_hierarchy for protocol->children); update all call sites accordingly so counting (protocol->frames and protocol->bytes) is guarded by insertion into this shared per-level set.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/wiregasm/lib.cpp`:
- Around line 1127-1142: The current loop breaks when response.rows reaches
limit which leaves metadata (matched and response.total_rows) reflecting only
the scanned prefix; instead, continue scanning all packets after hitting limit
but stop appending rows: when response.rows.size() >= limit set
response.truncated = true and continue processing (calling wtap_rec_reset(&rec)
and epan_dissect_reset(&edt) each iteration) so matched and any counters are
still updated for every record, and only suppress pushing additional rows into
response.rows; adjust the logic around the block that currently breaks to skip
push but not break so response.total_rows and matched reflect the full scan
while preserving limit behavior.
- Around line 1172-1183: The occurrences counter is only incremented when the
field wasn't already seen in the current frame because the increment is guarded
by frame_fields->insert(...). Change the logic in the block handling field
sightings (around frame_fields, present_fields, field_name and finfo->hfinfo in
lib.cpp) so that you always increment PresentField.occurrences whenever the
field is encountered: if present_fields does not contain field_name, create a
PresentField and set occurrences = 1; otherwise increment it
(it->second.occurrences++). Keep the existing frame_fields insert to suppress
per-frame duplicates for whatever purpose it serves, but do not gate the
occurrences increment on that insert.
---
Duplicate comments:
In `@lib/wiregasm/lib.cpp`:
- Around line 1272-1300: The counted_protocols set is recreated per call causing
duplicate counting across different branches at the same hierarchy level; modify
wg_collect_protocol_hierarchy to accept and propagate a reference (or pointer)
to a counted_protocols set for the current sibling-level so that when iterating
siblings you reuse the same set, and only create/pass a fresh set when recursing
into a protocol node's children (i.e., when calling
wg_collect_protocol_hierarchy for protocol->children); update all call sites
accordingly so counting (protocol->frames and protocol->bytes) is guarded by
insertion into this shared per-level set.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b16539c8-05d7-4fc9-b992-c31de6ef0625
📒 Files selected for processing (2)
lib/wiregasm/lib.cppsrc/index.ts
Comment on lines
+1127
to
+1142
| if (!row.values.empty()) { | ||
| response.rows.push_back(row); | ||
| if (limit > 0 && response.rows.size() >= limit) { | ||
| response.truncated = true; | ||
| wtap_rec_reset(&rec); | ||
| epan_dissect_reset(&edt); | ||
| break; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| wtap_rec_reset(&rec); | ||
| epan_dissect_reset(&edt); | ||
| } | ||
|
|
||
| response.total_rows = response.rows.size(); |
There was a problem hiding this comment.
Keep scanning after hitting limit so the metadata stays accurate.
Breaking here makes matched and total_rows describe only the scanned prefix, not the full result set. With truncated = true, callers will still expect the untruncated counts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/wiregasm/lib.cpp` around lines 1127 - 1142, The current loop breaks when
response.rows reaches limit which leaves metadata (matched and
response.total_rows) reflecting only the scanned prefix; instead, continue
scanning all packets after hitting limit but stop appending rows: when
response.rows.size() >= limit set response.truncated = true and continue
processing (calling wtap_rec_reset(&rec) and epan_dissect_reset(&edt) each
iteration) so matched and any counters are still updated for every record, and
only suppress pushing additional rows into response.rows; adjust the logic
around the block that currently breaks to skip push but not break so
response.total_rows and matched reflect the full scan while preserving limit
behavior.
Comment on lines
+1172
to
+1183
| if (frame_fields->insert(field_name).second) { | ||
| auto it = present_fields->find(field_name); | ||
| if (it == present_fields->end()) { | ||
| PresentField present; | ||
| present.field = field_name; | ||
| present.name = finfo->hfinfo->name ? finfo->hfinfo->name : ""; | ||
| present.type = static_cast<int>(finfo->hfinfo->type); | ||
| present.occurrences = 1; | ||
| (*present_fields)[field_name] = present; | ||
| } else { | ||
| it->second.occurrences++; | ||
| } |
There was a problem hiding this comment.
occurrences is currently counting frames, not occurrences.
frame_fields->insert(...) suppresses repeated instances inside the same packet, so fields that appear multiple times in one frame still add only 1. That makes the exported occurrences value inaccurate.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/wiregasm/lib.cpp` around lines 1172 - 1183, The occurrences counter is
only incremented when the field wasn't already seen in the current frame because
the increment is guarded by frame_fields->insert(...). Change the logic in the
block handling field sightings (around frame_fields, present_fields, field_name
and finfo->hfinfo in lib.cpp) so that you always increment
PresentField.occurrences whenever the field is encountered: if present_fields
does not contain field_name, create a PresentField and set occurrences = 1;
otherwise increment it (it->second.occurrences++). Keep the existing
frame_fields insert to suppress per-frame duplicates for whatever purpose it
serves, but do not gate the occurrences increment on that insert.
Upstream zlib.net re-published the 1.2.11 tarball with a different hash, breaking the glib subproject download. Update the expected source_hash and add a source_fallback_url pointing to the GitHub release mirror.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Tests