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

detect/files: append data on closing even with FILE_NOSTORE for file.data keyword usage #12713

Closed
wants to merge 1 commit into from
Closed

detect/files: append data on closing even with FILE_NOSTORE for file.data keyword usage #12713

wants to merge 1 commit into from

Conversation

catenacyber
Copy link
Contributor

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

Describe changes:

  • detect/files: append data on closing even with FILE_NOSTORE, useful on HTTP1 multipart small files where we close the file with the whole data

SV_BRANCH=OISF/suricata-verify#2333

@catenacyber
files: append data on closing even with FILE_NOSTORE
16815eb 
Ticket: 7577

When HTTP1 post multipart handles a small file, it will call
HTPFileClose with some data
This data needs to be appended to the streaming buffer for usage
by file.data keyword even if we do not end up storing the file
@catenacyber
Copy link
Contributor Author

Also, side-note :

We do not increment local_file_id++ when we have if (buffer == NULL) continue; which makes us return a NULL inspection buffer for all the remaining files

@victorjulien
Copy link
Member

Also, side-note :

We do not increment local_file_id++ when we have if (buffer == NULL) continue; which makes us return a NULL inspection buffer for all the remaining files

Sounds like a bug as well?

@codecov Codecov
Copy link

codecov bot commented Mar 4, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 80.72%. Comparing base (93bd193) to head (16815eb).
Report is 23 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #12713      +/-   ##
==========================================
+ Coverage   80.71%   80.72%   +0.01%     
==========================================
  Files         936      936              
  Lines      259393   259392       -1     
==========================================
+ Hits       209368   209398      +30     
+ Misses      50025    49994      -31
Flag Coverage Δ
fuzzcorpus 56.97% <33.33%> (-0.01%) ⬇️
livemode 19.42% <0.00%> (+<0.01%) ⬆️
pcap 44.18% <33.33%> (-0.01%) ⬇️
suricata-verify 63.52% <33.33%> (+<0.01%) ⬆️
unittests 58.21% <33.33%> (+<0.01%) ⬆️

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

🚀 New features to boost your workflow:
  • ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 24962

@catenacyber
Copy link
Contributor Author

Sounds like a bug as well?

https://redmine.openinfosecfoundation.org/issues/7579

Indeed, but hard to craft a test triggering the bug

@victorjulien victorjulien added this to the 8.0 milestone Mar 5, 2025
@victorjulien victorjulien mentioned this pull request Mar 5, 2025
Merged
@victorjulien
Copy link
Member

Merged in #12717, thanks!

@catenacyber catenacyber mentioned this pull request Mar 6, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@victorjulien victorjulien victorjulien approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
8.0
Development

Successfully merging this pull request may close these issues.
3 participants
@catenacyber @victorjulien @suricata-qa