Skip to content

Commit

Permalink
fix: make C FFI more robust with respect to malformed input (#5408)
Browse files Browse the repository at this point in the history
  • Loading branch information
Christopher M. Wolff committed Apr 12, 2023
1 parent f949299 commit 776751a
Show file tree
Hide file tree
Showing 3 changed files with 93 additions and 10 deletions.
83 changes: 77 additions & 6 deletions libflux/flux/src/cffi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use std::{

use anyhow::anyhow;
use fluxcore::{
ast,
ast::{self, BaseNode},
errors::SalvageResult,
formatter, merge_packages,
parser::Parser,
Expand Down Expand Up @@ -45,8 +45,9 @@ pub struct ErrorHandle {

impl From<Error> for Box<ErrorHandle> {
fn from(err: Error) -> Self {
let msg = err.to_string().replace('\0', "\\0");
Box::new(ErrorHandle {
message: CString::new(format!("{}", err)).unwrap(),
message: CString::new(msg).unwrap(),
err,
})
}
Expand Down Expand Up @@ -123,12 +124,36 @@ pub unsafe extern "C" fn flux_parse(
cfname: *const c_char,
csrc: *const c_char,
) -> Box<ast::Package> {
let fname = String::from_utf8(CStr::from_ptr(cfname).to_bytes().to_vec()).unwrap();
let src = String::from_utf8(CStr::from_ptr(csrc).to_bytes().to_vec()).unwrap();
let pkg = parse(fname, &src);
let pkg = catch_unwind(|| {
let fname = match String::from_utf8(CStr::from_ptr(cfname).to_bytes().to_vec()) {
Err(e) => return err_pkg(format!("flux file name: {}", e)),
Ok(s) => s,
};
let src = match String::from_utf8(CStr::from_ptr(csrc).to_bytes().to_vec()) {
Err(e) => return err_pkg(format!("flux source: {}", e)),
Ok(s) => s,
};
parse(fname, &src)
})
.unwrap_or_else(|panic_arg| match panic_arg.downcast_ref::<String>() {
Some(msg) => err_pkg(msg),
None => err_pkg("could not downcast message for caught panic"),
});
Box::new(pkg)
}

fn err_pkg<S: ToString>(msg: S) -> ast::Package {
ast::Package {
base: BaseNode {
errors: vec![msg.to_string()],
..BaseNode::default()
},
path: "".to_string(),
package: "".to_string(),
files: vec![],
}
}

/// Parse the contents of a string.
pub fn parse(fname: String, src: &str) -> ast::Package {
let mut p = Parser::new(src);
Expand Down Expand Up @@ -215,7 +240,7 @@ pub extern "C" fn flux_free_ast_pkg(_: Option<Box<ast::Package>>) {}
/// could occur.
#[no_mangle]
pub unsafe extern "C" fn flux_parse_json(
cstr: *mut c_char,
cstr: *const c_char,
out_pkg: *mut Option<Box<ast::Package>>,
) -> Option<Box<ErrorHandle>> {
catch_unwind(|| {
Expand Down Expand Up @@ -1095,4 +1120,50 @@ from(bucket: v.bucket)

assert_eq!(identifier.unwrap().name.package(), Some("universe"));
}

#[test]
fn parse_json_with_nul() {
let c_str = CString::new(
r#"{"type":"Package","package":"main","files":[{"body":[{"type":"\u0000"}]}]}"#,
)
.expect("no interior nuls");
let c_char_ptr = c_str.as_ptr();
let mut pkg: Option<Box<ast::Package>> = None;
let pkg_ptr: *mut Option<Box<ast::Package>> = &mut pkg;

// Safety: both pointers are valid
let err_hdl = unsafe { flux_parse_json(c_char_ptr, pkg_ptr) }.expect("some error");
let msg = err_hdl.message.to_string_lossy();
assert!(
err_hdl
.message
.to_string_lossy()
.contains("unknown variant `\\0`"),
"unexpected message: {msg}"
);
}

#[test]
fn parse_with_invalid_utf8() {
let cfname = CString::new("foo.flux").unwrap();
let cfname_ptr: *const c_char = cfname.as_ptr();
let v: Vec<c_char> = vec![-61, 0];
let csrc: *const c_char = &v[0];
// Safety: both pointers are valid
let pkg = unsafe { flux_parse(cfname_ptr, csrc) };
assert!(
pkg.base.errors[0].contains("flux source: incomplete utf-8"),
"did not get expected error in pkg: {:?}",
pkg
);

let (csrc, cfname_ptr) = (cfname_ptr, csrc);
// Safety: both pointers are valid
let pkg = unsafe { flux_parse(cfname_ptr, csrc) };
assert!(
pkg.base.errors[0].contains("flux file name: incomplete utf-8"),
"did not get expected error in pkg: {:?}",
pkg
);
}
}
2 changes: 1 addition & 1 deletion libflux/go/libflux/buildinfo.gen.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ var sourceHashes = map[string]string{
"libflux/flux/Cargo.toml": "308c541b31f6ef25094ed4a4c2fddaf38244b0632b93c1f44b2ee4b4209d479c",
"libflux/flux/FLUXDOC.md": "92e6dd8043bd87b4924e09aa28fb5346630aee1214de28ea2c8fc0687cad0785",
"libflux/flux/build.rs": "31dcb1e825555e56b4d959244c4ea630b1d32ccddc1f8615620e0c23552d914f",
"libflux/flux/src/cffi.rs": "d490e117f0e46386dee9fa35cc19209a07d1d57d7db352ab216b4d6fea1536a0",
"libflux/flux/src/cffi.rs": "d6cc56867bc475805c71cc9bc7ccd3cedf71b9cdf936973d5e30116245500802",
"libflux/flux/src/lib.rs": "af2f62a02ec08ee476121e035c6587e31c1b6d5d4912ccace1e2e9ae019ad9c1",
"libflux/flux/templates/base.html": "a818747b9621828bb96b94291c60922db54052bbe35d5e354f8e589d2a4ebd02",
"libflux/flux/templates/home.html": "f9927514dd42ca7271b4817ad1ca33ec79c03a77a783581b4dcafabd246ebf3f",
Expand Down
18 changes: 15 additions & 3 deletions libflux/go/libflux/parser_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import (
"context"
"encoding/json"
"errors"
"fmt"
"strings"
"testing"

"github.com/google/go-cmp/cmp"
Expand All @@ -26,11 +26,10 @@ from(bucket: "telegraf")
t.Fatal(err)
}

jsonBuf, err := ast.MarshalJSON()
_, err := ast.MarshalJSON()
if err != nil {
panic(err)
}
fmt.Printf("json has %v bytes:\n%v\n", len(jsonBuf), string(jsonBuf))

ast.Free()
}
Expand Down Expand Up @@ -252,6 +251,19 @@ re !~ /foo/
}
}

func TestParseJSON_NullByte(t *testing.T) {
bytes := []byte(`{"type":"Package","package":"main","files":[{"body":[{"type":"\u0000"}]}]}`)
_, err := libflux.ParseJSON(bytes)
if err == nil {
t.Fatal("expected error")
}
want := "unknown variant `\\0`, expected one of"
got := err.Error()
if !strings.Contains(got, want) {
t.Fatalf("expected error message to contain\n%q\nbut it contained\n%q", want, got)
}
}

func mustIndent(t *testing.T, bs []byte) []byte {
t.Helper()
var bb bytes.Buffer
Expand Down

0 comments on commit 776751a

Please sign in to comment.