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: remove transmute; make fn's unsafe - v8 #6299

Closed
wants to merge 2 commits into from

Conversation

jasonish
Copy link
Member

Previous PR: #6280

The first commits remove all usages of transmute. Every usage of
it in our code can be replaced by a cast, or Box::into_raw and
Box::from_raw.

Next, mark all functions that dereference a raw pointer as unsafe,
this means most extern "C" functions that take a pointer from C.

Reference: https://rust-lang.github.io/rust-clippy/master/index.html#not_unsafe_ptr_arg_deref

And some discussion in PR #4666.

All cases of our transmute can be replaced with more idiomatic
solutions and do no require the power of transmute.

When returning an object to C for life-time management, use
Box::into_raw to convert the boxed object to pointer and use
Box::from_raw to convert back.

For cases where we're just returning a pointer to Rust managed
data, use a cast.
Based on the Rust clippy lint that recommends that any public
function that dereferences a raw pointer, mark all FFI functions
that reference raw pointers with build_slice and cast_pointer
as unsafe.

This commits starts by removing the unsafe wrapper inside
the build_slice and cast_pointer macros then marks all
functions that use these macros as unsafe.

Then fix all not_unsafe_ptr_arg_deref warnings from clippy.

Fixes clippy lint:
https://rust-lang.github.io/rust-clippy/master/index.html#not_unsafe_ptr_arg_deref
@jasonish jasonish requested a review from victorjulien as a code owner August 18, 2021 19:12
@jasonish jasonish changed the title (rfc) rust: remove transmute; make fn's unsafe - v8 rust: remove transmute; make fn's unsafe - v8 Aug 18, 2021
@codecov
Copy link

codecov bot commented Aug 18, 2021

Codecov Report

Merging #6299 (ef271b4) into master (9d24a53) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #6299   +/-   ##
=======================================
  Coverage   76.94%   76.94%           
=======================================
  Files         611      611           
  Lines      185941   185941           
=======================================
+ Hits       143064   143075   +11     
+ Misses      42877    42866   -11     
Flag Coverage Δ
fuzzcorpus 52.80% <ø> (+0.06%) ⬆️
suricata-verify 51.13% <ø> (-0.04%) ⬇️
unittests 63.12% <ø> (ø)

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 3844

@jasonish
Copy link
Member Author

Continued at #6303 which cleans up more clippy lints.

@jasonish jasonish closed this Aug 20, 2021
@jasonish jasonish deleted the rust/unsafe-ffi/v8 branch August 23, 2021 18:18
catenacyber added a commit to catenacyber/suricata that referenced this pull request Sep 18, 2023
catenacyber added a commit to catenacyber/suricata that referenced this pull request Sep 18, 2023
catenacyber added a commit to catenacyber/suricata that referenced this pull request Oct 5, 2023
catenacyber added a commit to catenacyber/suricata that referenced this pull request Oct 10, 2023
catenacyber added a commit to catenacyber/suricata that referenced this pull request Oct 17, 2023
catenacyber added a commit to catenacyber/suricata that referenced this pull request Oct 17, 2023
expecially sets transactions to complete when we get a response
without having seen the request.

Ticket: OISF#6299
catenacyber added a commit to catenacyber/suricata that referenced this pull request Jan 5, 2024
catenacyber added a commit to catenacyber/suricata that referenced this pull request Jan 5, 2024
expecially sets transactions to complete when we get a response
without having seen the request.

Ticket: OISF#6299
catenacyber added a commit to catenacyber/suricata that referenced this pull request Jan 9, 2024
Ticket: OISF#6299

Simply because it is faster (just linear).
catenacyber added a commit to catenacyber/suricata that referenced this pull request Jan 9, 2024
Especially sets transactions to complete when we get a response
without having seen the request, so that the transactions
end up getting cleaned (instead of living/leaking in the state).

Also try to set the event on the relevant transaction, instead
of creating a new transaction just for the purpose of having
the event.

Ticket: OISF#6299
catenacyber added a commit to catenacyber/suricata that referenced this pull request Jan 9, 2024
Ticket: OISF#6299

Simply because it is faster (just linear).
catenacyber added a commit to catenacyber/suricata that referenced this pull request Jan 9, 2024
Especially sets transactions to complete when we get a response
without having seen the request, so that the transactions
end up getting cleaned (instead of living/leaking in the state).

Also try to set the event on the relevant transaction, instead
of creating a new transaction just for the purpose of having
the event.

Ticket: OISF#6299
catenacyber added a commit to catenacyber/suricata that referenced this pull request Jan 9, 2024
Especially sets transactions to complete when we get a response
without having seen the request, so that the transactions
end up getting cleaned (instead of living/leaking in the state).

Also try to set the event on the relevant transaction, instead
of creating a new transaction just for the purpose of having
the event.

Ticket: OISF#6299
catenacyber added a commit to catenacyber/suricata that referenced this pull request Jan 14, 2024
Ticket: OISF#6299

Simply because it is faster (just linear).
catenacyber added a commit to catenacyber/suricata that referenced this pull request Jan 14, 2024
Especially sets transactions to complete when we get a response
without having seen the request, so that the transactions
end up getting cleaned (instead of living/leaking in the state).

Also try to set the event on the relevant transaction, instead
of creating a new transaction just for the purpose of having
the event.

Ticket: OISF#6299
catenacyber added a commit to catenacyber/suricata that referenced this pull request Jan 14, 2024
Ticket: OISF#6299

Simply because it is faster (just linear).

This is for merging match_array into tx_candidates
catenacyber added a commit to catenacyber/suricata that referenced this pull request Jan 14, 2024
Especially sets transactions to complete when we get a response
without having seen the request, so that the transactions
end up getting cleaned (instead of living/leaking in the state).

Also try to set the event on the relevant transaction, instead
of creating a new transaction just for the purpose of having
the event.

Ticket: OISF#6299
catenacyber added a commit to catenacyber/suricata that referenced this pull request Jan 15, 2024
Ticket: OISF#6299

Simply because it is faster (just linear).

This is for merging match_array into tx_candidates
catenacyber added a commit to catenacyber/suricata that referenced this pull request Jan 15, 2024
Especially sets transactions to complete when we get a response
without having seen the request, so that the transactions
end up getting cleaned (instead of living/leaking in the state).

Also try to set the event on the relevant transaction, instead
of creating a new transaction just for the purpose of having
the event.

Ticket: OISF#6299
catenacyber added a commit to catenacyber/suricata that referenced this pull request Jan 15, 2024
Ticket: OISF#6299

Simply because it is faster (just linear).

This is for merging match_array into tx_candidates
catenacyber added a commit to catenacyber/suricata that referenced this pull request Jan 15, 2024
Especially sets transactions to complete when we get a response
without having seen the request, so that the transactions
end up getting cleaned (instead of living/leaking in the state).

Also try to set the event on the relevant transaction, instead
of creating a new transaction just for the purpose of having
the event.

Ticket: OISF#6299
victorjulien pushed a commit to victorjulien/suricata that referenced this pull request Jan 30, 2024
Ticket: OISF#6299

Simply because it is faster (just linear).

This is for merging match_array into tx_candidates
victorjulien pushed a commit to victorjulien/suricata that referenced this pull request Jan 30, 2024
Especially sets transactions to complete when we get a response
without having seen the request, so that the transactions
end up getting cleaned (instead of living/leaking in the state).

Also try to set the event on the relevant transaction, instead
of creating a new transaction just for the purpose of having
the event.

Ticket: OISF#6299
victorjulien pushed a commit to victorjulien/suricata that referenced this pull request Jan 30, 2024
Ticket: OISF#6299

Simply because it is faster (just linear).

This is for merging match_array into tx_candidates

(cherry picked from commit 5bb8800)
victorjulien pushed a commit to victorjulien/suricata that referenced this pull request Jan 30, 2024
Especially sets transactions to complete when we get a response
without having seen the request, so that the transactions
end up getting cleaned (instead of living/leaking in the state).

Also try to set the event on the relevant transaction, instead
of creating a new transaction just for the purpose of having
the event.

Ticket: OISF#6299
(cherry picked from commit 89936b6)
ct0br0 pushed a commit to ct0br0/suricata that referenced this pull request Jan 30, 2024
Especially sets transactions to complete when we get a response
without having seen the request, so that the transactions
end up getting cleaned (instead of living/leaking in the state).

Also try to set the event on the relevant transaction, instead
of creating a new transaction just for the purpose of having
the event.

Ticket: OISF#6299
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