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

output/email: reset custom fields during init #9544

Closed
wants to merge 1 commit into from

Conversation

inashivb
Copy link
Member

@inashivb inashivb commented Oct 3, 2023

Redmine ticket: None yet. Need somebody to confirm if the analysis is correct and this is a bug. This purely came from "works on my machine" case of OISF/suricata-verify#1405 after I spent way too much time figuring the difference in behaviors and finally tried to check the code.

I believe (and tested) that the alternate solution for this problem is:

diff --git a/src/output-json-smtp.c b/src/output-json-smtp.c
index cc3003907..f7674687c 100644
--- a/src/output-json-smtp.c
+++ b/src/output-json-smtp.c
@@ -122,7 +122,7 @@ static OutputInitResult OutputSmtpLogInitSub(ConfNode *conf, OutputCtx *parent_c
     OutputInitResult result = { NULL, false };
     OutputJsonCtx *ojc = parent_ctx->data;
 
-    OutputJsonEmailCtx *email_ctx = SCMalloc(sizeof(OutputJsonEmailCtx));
+    OutputJsonEmailCtx *email_ctx = SCCalloc(1, sizeof(OutputJsonEmailCtx));
     if (unlikely(email_ctx == NULL))
         return result;

I'm not sure which one is more ideal.

email_ctx->fields only get populated when smtp.custom setting is on.
The fn EveEmailLogJSONCustom is called when either
1. smtp.extended setting is on or,
2. email_ctx->fields is populated which means smtp.custom setting is on

In case neither of these are set in suricata.yaml, no call should
ideally be made to the fn EveEmailLogJSONCustom.
However, it turns out that email_ctx->fields is unset and then set only
after the smtp config was found. This leads to email_ctx->fields
sometimes contain value even when no config was given to the smtp
section and can lead to unexpected output.
@codecov
Copy link

codecov bot commented Oct 3, 2023

Codecov Report

Merging #9544 (a3b2743) into master (bb15a8f) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9544      +/-   ##
==========================================
- Coverage   82.19%   82.18%   -0.02%     
==========================================
  Files         968      968              
  Lines      274216   274216              
==========================================
- Hits       225405   225374      -31     
- Misses      48811    48842      +31     
Flag Coverage Δ
fuzzcorpus 64.07% <100.00%> (+<0.01%) ⬆️
suricata-verify 60.90% <100.00%> (-0.03%) ⬇️
unittests 62.88% <0.00%> (-0.01%) ⬇️

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

@inashivb inashivb marked this pull request as ready for review October 4, 2023 02:20
@victorjulien
Copy link
Member

When 8 opens we will merge #9388. I think I would prefer the same fix here, as you suggest.

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.

To be clear, I would to see a new PR with the SCCalloc approach.

@inashivb inashivb closed this Oct 4, 2023
@inashivb inashivb deleted the smtp-email-logging/v1 branch October 4, 2023 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants