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

decode: fix offset for DCE layer #9251

Closed
wants to merge 1 commit into from

Conversation

catenacyber
Copy link
Contributor

Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/3637

Describe changes:

  • decode: fix offset for DCE layer

Jason, it looks like your commit 95015a3 was right but got broken in between

Rebase #9153 now that 7 is released to get attention for 7.0.1

OISF/suricata-verify#1289

SV_BRANCH=pr/1289

@victorjulien
Copy link
Member

Can you find the commit that broke things? Would like to understand if it was broken accidentally or that it was attempt to fix something else.

@victorjulien
Copy link
Member

Btw, my ~/.gitconfig has:

[alias]
    fixes = log -1 --pretty='tformat:Fixes: %h (\"%s\")' --abbrev=12

Then git fixes <hash> creates a nice line to add to a commit.

@catenacyber
Copy link
Contributor Author

Commit 136d351 broke it

-        case ETHERNET_TYPE_DCE:
-            if (unlikely(len < ETHERNET_DCE_HEADER_LEN)) {
-                ENGINE_SET_INVALID_EVENT(p, DCE_PKT_TOO_SMALL);
-            } else {
-                DecodeEthernet(tv, dtv, p, pkt + ETHERNET_DCE_HEADER_LEN,
-                    len - ETHERNET_DCE_HEADER_LEN);
-            }
-            break;
+        case ETHERNET_TYPE_DCE:
+            if (unlikely(len < ETHERNET_DCE_HEADER_LEN)) {
+                ENGINE_SET_INVALID_EVENT(p, DCE_PKT_TOO_SMALL);
+            } else {
+                DecodeEthernet(tv, dtv, p, data, len);
+            }
+            break;

base ETHERNET_DCE_HEADER_LEN is different than ETHERNET_HEADER_LEN

@catenacyber
Copy link
Contributor Author

I force-pushed the reworded commit.
Is it good ?

@codecov
Copy link

codecov bot commented Jul 19, 2023

Codecov Report

Merging #9251 (9ad4ade) into master (9a33c53) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9251      +/-   ##
==========================================
- Coverage   82.42%   82.42%   -0.01%     
==========================================
  Files         968      968              
  Lines      273952   273952              
==========================================
- Hits       225807   225795      -12     
- Misses      48145    48157      +12     
Flag Coverage Δ
fuzzcorpus 64.69% <100.00%> (-0.01%) ⬇️
suricata-verify 60.80% <100.00%> (-0.01%) ⬇️
unittests 62.93% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@victorjulien
Copy link
Member

I force-pushed the reworded commit. Is it good ?

git log --grep 'Fixes:' shows examples of what the format is if you'd use my alias above

@catenacyber
Copy link
Contributor Author

I force-pushed the reworded commit. Is it good ?

git log --grep 'Fixes:' shows examples of what the format is if you'd use my alias above

Roger, we got that wrong for commit 789353b @inashivb ;-)

(we used completes instead of Fixes)

See #9261

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

Successfully merging this pull request may close these issues.

2 participants