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

rust: don't suppress static_mut_refs globally - v2 #12673

Closed
wants to merge 4 commits into from

Conversation

jasonish
Copy link
Member

As its use is now a default compiler warning, and eventually will become an
error, remove the global suppression of it. Instead suppress as needed, so its
more intentional.

  • rust: remove allow of static mutables
  • smtp: use rwlock instead of global static mut
  • smb: use atomics instead of static mutables

For reference:
https://doc.rust-lang.org/nightly/edition-guide/rust-2024/static-mut-references.html,

Ticket: https://redmine.openinfosecfoundation.org/issues/7417

Other than some debug output on initialization we don't take mutable
references to these static mut's.

    = note: for more information, see <https://doc.rust-lang.org/nightly/edition-guide/rust-2024/static-mut-references.html>
    = note: mutable references to mutable statics are dangerous; it's undefined behavior if any other pointer to the static is used or if any other reference is created for the static while the mutable reference lives

Ticket: OISF#7417
It doesn't appear to be needed. The vec being cleared is only set once
per run, so never needs to be cleared.

Removes one point where we have to supress the static_mut_refs compiler
warning.

Ticket: OISF#7417
Allows us to get rid of the global supression.

Ticket: OISF#7417
As references to static mutables are highly discouraged, remove the
global suppressing of the compiler warning. Each use case can be
suppressed as needed.

Ticket: OISF#7417
Copy link

codecov bot commented Feb 25, 2025

Codecov Report

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

Project coverage is 80.77%. Comparing base (3bc2a14) to head (e17c930).
Report is 10 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #12673   +/-   ##
=======================================
  Coverage   80.77%   80.77%           
=======================================
  Files         932      932           
  Lines      259517   259521    +4     
=======================================
+ Hits       209629   209633    +4     
  Misses      49888    49888           
Flag Coverage Δ
fuzzcorpus 57.00% <0.00%> (+0.01%) ⬆️
livemode 19.39% <0.00%> (+0.01%) ⬆️
pcap 44.19% <0.00%> (+0.03%) ⬆️
suricata-verify 63.51% <100.00%> (+<0.01%) ⬆️
unittests 58.32% <0.00%> (-0.01%) ⬇️

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

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 24895

// Suppress static_mut_refs for this debug code. Safe
// otherwise.
#[allow(unused_lints, static_mut_refs)]
{
Copy link
Member

Choose a reason for hiding this comment

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

would it make sense to contain this stuff in a helper func that can be inlined?

e.g. something like (pseudocode)

/* SMB_CFG_MAX_READ_SIZE is set at Suricata startup and is static after that... */
#[always_inline]
#[allow(static_mut_refs)]
fn get_max_read_size(...) -> u32 {
    SMB_CFG_MAX_READ_SIZE
}

Copy link
Member

Choose a reason for hiding this comment

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

that way we can keep the clutter out of the main logic. Esp when addition indents are added things are getting ugly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, and in turn, wrapping the get in a function allows the compiler to reason about it better and you can remove the allow for static_mut_refs.

@jasonish
Copy link
Member Author

Replaced by #12680

@jasonish jasonish closed this Feb 26, 2025
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.

3 participants