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

[draft] Example of catching panics in Rust #8757

Closed
wants to merge 3 commits into from
Closed

[draft] Example of catching panics in Rust #8757

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d904590
rust/dns: example of catching rust panics
jasonish Apr 24, 2023
66bd9c1
jsonbuilder: rustfmt
jasonish May 7, 2023
9cbae4e
(wip) rust/jsonbuilder: make "new" fallible
jasonish May 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 55 additions & 32 deletions rust/src/dns/dns.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ use std;
use std::collections::HashMap;
use std::collections::VecDeque;
use std::ffi::CString;
use std::panic::catch_unwind;

use crate::applayer::*;
use crate::core::{self, *};
Expand Down Expand Up @@ -769,8 +770,10 @@ pub unsafe extern "C" fn rs_dns_parse_request(
flow: *const core::Flow, state: *mut std::os::raw::c_void, _pstate: *mut std::os::raw::c_void,
stream_slice: StreamSlice, _data: *const std::os::raw::c_void,
) -> AppLayerResult {
let state = cast_pointer!(state, DNSState);
state.parse_request_udp(flow, stream_slice);
let _ = catch_unwind(|| {
let state = cast_pointer!(state, DNSState);
state.parse_request_udp(flow, stream_slice);
});
Comment on lines +773 to +776
Copy link
Member

Choose a reason for hiding this comment

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

I'm trying to understand why we're using it here. I'm reading Don't build your programs to unwind under normal circumstances. Ideally, you should only panic for programming errors or extreme problems. at https://doc.rust-lang.org/nomicon/unwinding.html where I also see You must absolutely catch any panics at the FFI boundary! so I kind of see why we want to do this. But,

  1. What points of panic/undefined behavior can happen on the enclosed code?
  2. How do we know that the panic caused here for whatever reason would be implemented with unwinding? Or, do we not care about it as it is not incurring any runtime cost in either case?

Thank you for this! :)

Copy link
Member

Choose a reason for hiding this comment

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

For reference https://redmine.openinfosecfoundation.org/issues/3333

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm trying to understand why we're using it here. I'm reading Don't build your programs to unwind under normal circumstances. Ideally, you should only panic for programming errors or extreme problems. at https://doc.rust-lang.org/nomicon/unwinding.html where I also see You must absolutely catch any panics at the FFI boundary! so I kind of see why we want to do this. But,

  1. What points of panic/undefined behavior can happen on the enclosed code?

Simple ones that we can catch would be programming errors like walking off the end of an array. In C this might create a segfault, but is a catchable error in Rust. These warnings are more to prevent people from using unwinding as a flow control mechanism, or treating them like exceptions when coming from C++, Python, etc.

I also disagree about the must on FFI boundaries. Currently, we crash if the Rust code panics at the FFI boundary, and that's a reasonable outcome.

  1. How do we know that the panic caused here for whatever reason would be implemented with unwinding? Or, do we not care about it as it is not incurring any runtime cost in either case?

We don't care. If the panic is unwindable we'll catch it and not crash. If it's not catchable, perhaps a deep-rooted segfault, or an illegal instruction the program should abort and crash.

All the Rust web frameworks like Axum, Rocket, Warp, implement such unwinding in their request handlers, so one bad request handler won't take down the server, this is a very similar approach.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for the explanation!
Do I get it right that if there is actually an error that can be unwinded, it'll affect the performance negatively?
Do we log it in stats somehow to indicate what's causing the possible slowdown?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do I get it right that if there is actually an error that can be unwinded, it'll affect the performance negatively?

Yes, it should a little as it has to do some resetting of data. Plus that line that gets log that we can't control is slow. All prints to the console are slow though.

The alternative is crashing.

AppLayerResult::ok()
}

Expand All @@ -779,8 +782,10 @@ pub unsafe extern "C" fn rs_dns_parse_response(
flow: *const core::Flow, state: *mut std::os::raw::c_void, _pstate: *mut std::os::raw::c_void,
stream_slice: StreamSlice, _data: *const std::os::raw::c_void,
) -> AppLayerResult {
let state = cast_pointer!(state, DNSState);
state.parse_response_udp(flow, stream_slice);
let _ = catch_unwind(|| {
let state = cast_pointer!(state, DNSState);
state.parse_response_udp(flow, stream_slice);
});
AppLayerResult::ok()
}

Expand All @@ -790,27 +795,40 @@ pub unsafe extern "C" fn rs_dns_parse_request_tcp(
flow: *const core::Flow, state: *mut std::os::raw::c_void, _pstate: *mut std::os::raw::c_void,
stream_slice: StreamSlice, _data: *const std::os::raw::c_void,
) -> AppLayerResult {
let state = cast_pointer!(state, DNSState);
if stream_slice.is_gap() {
state.request_gap(stream_slice.gap_size());
} else if !stream_slice.is_empty() {
return state.parse_request_tcp(flow, stream_slice);
match catch_unwind(|| {
let state = cast_pointer!(state, DNSState);
if stream_slice.is_gap() {
state.request_gap(stream_slice.gap_size());
} else if !stream_slice.is_empty() {
return state.parse_request_tcp(flow, stream_slice);
}
return AppLayerResult::ok();
}) {
Ok(result) => result,
Result::Err(_) => AppLayerResult::err(),
}
AppLayerResult::ok()
}

#[no_mangle]
pub unsafe extern "C" fn rs_dns_parse_response_tcp(
flow: *const core::Flow, state: *mut std::os::raw::c_void, _pstate: *mut std::os::raw::c_void,
stream_slice: StreamSlice, _data: *const std::os::raw::c_void,
) -> AppLayerResult {
let state = cast_pointer!(state, DNSState);
if stream_slice.is_gap() {
state.response_gap(stream_slice.gap_size());
} else if !stream_slice.is_empty() {
return state.parse_response_tcp(flow, stream_slice);
match catch_unwind(|| {
let state = cast_pointer!(state, DNSState);
if stream_slice.is_gap() {
state.response_gap(stream_slice.gap_size());
} else if !stream_slice.is_empty() {
return state.parse_response_tcp(flow, stream_slice);
}
return AppLayerResult::ok();
}) {
Ok(result) => result,
Result::Err(_) => {
println!("caught error!");
return AppLayerResult::err();
}
}
AppLayerResult::ok()
}

#[no_mangle]
Expand Down Expand Up @@ -938,24 +956,29 @@ pub unsafe extern "C" fn rs_dns_probe(
pub unsafe extern "C" fn rs_dns_probe_tcp(
_flow: *const core::Flow, direction: u8, input: *const u8, len: u32, rdir: *mut u8,
) -> AppProto {
if len == 0 || len < std::mem::size_of::<DNSHeader>() as u32 + 2 {
return core::ALPROTO_UNKNOWN;
}
let slice: &[u8] = std::slice::from_raw_parts(input as *mut u8, len as usize);
//is_incomplete is checked by caller
let (is_dns, is_request, _) = probe_tcp(slice);
if is_dns {
let dir = if is_request {
Direction::ToServer
} else {
Direction::ToClient
};
if (direction & DIR_BOTH) != dir.into() {
*rdir = dir as u8;
match catch_unwind(|| {
if len == 0 || len < std::mem::size_of::<DNSHeader>() as u32 + 2 {
return core::ALPROTO_UNKNOWN;
}
return ALPROTO_DNS;
let slice: &[u8] = std::slice::from_raw_parts(input as *mut u8, len as usize);
//is_incomplete is checked by caller
let (is_dns, is_request, _) = probe_tcp(slice);
if is_dns {
let dir = if is_request {
Direction::ToServer
} else {
Direction::ToClient
};
if (direction & DIR_BOTH) != dir.into() {
*rdir = dir as u8;
}
return ALPROTO_DNS;
}
return 0;
}) {
Ok(proto) => proto,
Result::Err(_) => ALPROTO_UNKNOWN,
}
return 0;
}

#[no_mangle]
Expand Down
20 changes: 10 additions & 10 deletions rust/src/dns/log.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,7 +399,7 @@ pub fn dns_print_addr(addr: &Vec<u8>) -> std::string::String {

/// Log SOA section fields.
fn dns_log_soa(soa: &DNSRDataSOA) -> Result<JsonBuilder, JsonError> {
let mut js = JsonBuilder::new_object();
let mut js = JsonBuilder::try_new_object()?;

js.set_string_from_bytes("mname", &soa.mname)?;
js.set_string_from_bytes("rname", &soa.rname)?;
Expand All @@ -415,7 +415,7 @@ fn dns_log_soa(soa: &DNSRDataSOA) -> Result<JsonBuilder, JsonError> {

/// Log SSHFP section fields.
fn dns_log_sshfp(sshfp: &DNSRDataSSHFP) -> Result<JsonBuilder, JsonError> {
let mut js = JsonBuilder::new_object();
let mut js = JsonBuilder::try_new_object()?;

let mut hex = Vec::new();
for byte in &sshfp.fingerprint {
Expand All @@ -432,7 +432,7 @@ fn dns_log_sshfp(sshfp: &DNSRDataSSHFP) -> Result<JsonBuilder, JsonError> {

/// Log SRV section fields.
fn dns_log_srv(srv: &DNSRDataSRV) -> Result<JsonBuilder, JsonError> {
let mut js = JsonBuilder::new_object();
let mut js = JsonBuilder::try_new_object()?;

js.set_uint("priority", srv.priority as u64)?;
js.set_uint("weight", srv.weight as u64)?;
Expand All @@ -444,7 +444,7 @@ fn dns_log_srv(srv: &DNSRDataSRV) -> Result<JsonBuilder, JsonError> {
}

fn dns_log_json_answer_detail(answer: &DNSAnswerEntry) -> Result<JsonBuilder, JsonError> {
let mut jsa = JsonBuilder::new_object();
let mut jsa = JsonBuilder::try_new_object()?;

jsa.set_string_from_bytes("rrname", &answer.name)?;
jsa.set_string("rrtype", &dns_rrtype_string(answer.rrtype))?;
Expand Down Expand Up @@ -516,7 +516,7 @@ fn dns_log_json_answer(
js.set_string("rcode", &dns_rcode_string(header.flags))?;

if !response.answers.is_empty() {
let mut js_answers = JsonBuilder::new_array();
let mut js_answers = JsonBuilder::try_new_array()?;

// For grouped answers we use a HashMap keyed by the rrtype.
let mut answer_types = HashMap::new();
Expand All @@ -527,7 +527,7 @@ fn dns_log_json_answer(
match &answer.data {
DNSRData::A(addr) | DNSRData::AAAA(addr) => {
if !answer_types.contains_key(&type_string) {
answer_types.insert(type_string.to_string(), JsonBuilder::new_array());
answer_types.insert(type_string.to_string(), JsonBuilder::try_new_array()?);
}
if let Some(a) = answer_types.get_mut(&type_string) {
a.append_string(&dns_print_addr(addr))?;
Expand All @@ -540,31 +540,31 @@ fn dns_log_json_answer(
| DNSRData::NULL(bytes)
| DNSRData::PTR(bytes) => {
if !answer_types.contains_key(&type_string) {
answer_types.insert(type_string.to_string(), JsonBuilder::new_array());
answer_types.insert(type_string.to_string(), JsonBuilder::try_new_array()?);
}
if let Some(a) = answer_types.get_mut(&type_string) {
a.append_string_from_bytes(bytes)?;
}
}
DNSRData::SOA(soa) => {
if !answer_types.contains_key(&type_string) {
answer_types.insert(type_string.to_string(), JsonBuilder::new_array());
answer_types.insert(type_string.to_string(), JsonBuilder::try_new_array()?);
}
if let Some(a) = answer_types.get_mut(&type_string) {
a.append_object(&dns_log_soa(soa)?)?;
}
}
DNSRData::SSHFP(sshfp) => {
if !answer_types.contains_key(&type_string) {
answer_types.insert(type_string.to_string(), JsonBuilder::new_array());
answer_types.insert(type_string.to_string(), JsonBuilder::try_new_array()?);
}
if let Some(a) = answer_types.get_mut(&type_string) {
a.append_object(&dns_log_sshfp(sshfp)?)?;
}
}
DNSRData::SRV(srv) => {
if !answer_types.contains_key(&type_string) {
answer_types.insert(type_string.to_string(), JsonBuilder::new_array());
answer_types.insert(type_string.to_string(), JsonBuilder::try_new_array()?);
}
if let Some(a) = answer_types.get_mut(&type_string) {
a.append_object(&dns_log_srv(srv)?)?;
Expand Down
Loading