Skip to content

Multipart mime 3487 v38#11130

Closed
catenacyber wants to merge 5 commits intoOISF:masterfrom
catenacyber:multipart-mime-3487-v38
Closed

Multipart mime 3487 v38#11130
catenacyber wants to merge 5 commits intoOISF:masterfrom
catenacyber:multipart-mime-3487-v38

Conversation

@catenacyber
Copy link
Contributor

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

Describe changes:

  • convert HTTP to use new rust mime parser
  • convert SMTP to use new rust mime parser
  • json schema : add email.received array

Follows #11115 with rebase to get more SV test + adding a field in json schema from this test

SV_BRANCH=OISF/suricata-verify#1851

Copy link
Contributor

@jlucovsky jlucovsky left a comment

Choose a reason for hiding this comment

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

just a few nits -- copyright dates.

LGTM

@@ -0,0 +1,569 @@
/* Copyright (C) 2021 Open Information Security Foundation
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: date

@@ -1,4 +1,4 @@
/* Copyright (C) 2021 Open Information Security Foundation
/* Copyright (C) 2022 Open Information Security Foundation
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: date

@@ -0,0 +1,844 @@
/* Copyright (C) 2022 Open Information Security Foundation
Copy link
Contributor

Choose a reason for hiding this comment

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

date, here and everywhere

size_t new_val_len = strlen(scheme->val) + 3 + 1;
if (new_val_len > UINT16_MAX) {
size_t scheme_len = strlen(scheme->val);
if (scheme_len > UINT16_MAX - SCHEME_SUFFIX_LEN) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: even though - has higher precedence than >, I'd prefer paren's around UINT16_MAX - SCHEME_SUFFIX_LEN)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why so ?

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 20760

@codecov
Copy link

codecov bot commented May 24, 2024

Codecov Report

Attention: Patch coverage is 92.09139% with 90 lines in your changes are missing coverage. Please review.

Project coverage is 84.13%. Comparing base (b3eb1c4) to head (91be459).

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #11130      +/-   ##
==========================================
- Coverage   84.26%   84.13%   -0.14%     
==========================================
  Files         926      926              
  Lines      247387   246010    -1377     
==========================================
- Hits       208453   206970    -1483     
- Misses      38934    39040     +106     
Flag Coverage Δ
fuzzcorpus 64.01% <74.81%> (-0.18%) ⬇️
livemode 19.68% <2.53%> (+0.11%) ⬆️
pcap 46.26% <76.97%> (-0.21%) ⬇️
suricata-verify 62.79% <86.93%> (-0.32%) ⬇️
unittests 61.82% <57.64%> (-0.13%) ⬇️

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

@victorjulien victorjulien added this to the 8.0 milestone May 24, 2024
@victorjulien victorjulien self-requested a review May 24, 2024 14:16
Copy link
Member

@victorjulien victorjulien left a comment

Choose a reason for hiding this comment

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

Needs rebase due to a conflict.

Can you update the years while at it?

@catenacyber
Copy link
Contributor Author

Needs rebase due to a conflict.

Rebased in #11157

Can you update the years while at it?

Confused about this :
Jason told The year doesn't hold much value - but if a PR was first submitted in 2022 but not merged until 2024, it should keep its 2022 copyright date.
I still updated some years

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.

4 participants