From d9b9d103bf5c4610927e1fe4ad32012ea5100710 Mon Sep 17 00:00:00 2001 From: Nick Vatamaniuc Date: Tue, 30 Jun 2026 17:22:52 -0400 Subject: [PATCH] Fix out-of-bounds heap write in string encoder Previously if we had a run of unescaped bytes copid as a single memcpy after reserving space with enc_ensure(run), enc_ensure() could report success without actually growing the buffer so the memcpy would have overwritten memory. There is a good chance it fell around the memory allocate heap metadata so the crash might have shown up the allocator calls in OTP. This would be pretty hard to hit, it would be something like long strings starting with lots escapes (to push e->i high enough) then also followed by long runs of non-escapes. That explains why property tests never caught this. The fix is to check the buffer size if he have a buffer already before returning. If it's too small, return otherwise release and allocate one of the right size. To ensure we catch these kind of errors in the future, enhance property tests by switching to PropEr (code borrowed from Apache CouchDB). For belt-and-suspenders and also added some determistic tests as well for the issue. Fix https://github.com/davisp/jiffy/issues/303 --- .github/workflows/ci.yml | 28 +-- .gitignore | 3 - Makefile | 11 +- c_src/encoder.c | 10 +- rebar.config | 9 + rebar.config.script | 29 +-- test/install_eqc_mini.sh | 40 ---- test/jiffy_11_property_tests.erl | 249 ++++++++++++-------- test/jiffy_23_enc_buffer_boundary_tests.erl | 60 +++++ test/jiffy_prop.hrl | 22 ++ 10 files changed, 267 insertions(+), 194 deletions(-) delete mode 100755 test/install_eqc_mini.sh create mode 100644 test/jiffy_23_enc_buffer_boundary_tests.erl create mode 100644 test/jiffy_prop.hrl diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index db347ddb..5a0ebfa2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -30,30 +30,20 @@ jobs: run: | make REBAR=${{matrix.rebar}} check - # Keep this no more than 2 releases away from EQC-mini beam files. Currently - # they are on OTP 25 so we can use erlang up to version 27 technically. - # - eqc-mini-25: - name: eqc-mini-25 - runs-on: ubuntu-latest - container: - image: erlang:25 - steps: - - uses: actions/checkout@v6 - - name: rebar3 make check-with-eqc - run: | - make REBAR=rebar3 check-with-eqc - - eqc-mini-27: - name: eqc-mini-27 + prop-tests: + name: proper-${{matrix.otp_version}} runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + otp_version: [27, 28] container: - image: erlang:27 + image: erlang:${{matrix.otp_version}} steps: - uses: actions/checkout@v6 - - name: rebar3 make check-with-eqc + - name: rebar3 make check-with-proper run: | - make REBAR=rebar3 check-with-eqc + make REBAR=rebar3 check-with-proper lcov-coverage: name: lcov-coverage diff --git a/.gitignore b/.gitignore index 10d69c0b..e9197cd1 100644 --- a/.gitignore +++ b/.gitignore @@ -15,13 +15,10 @@ erln8.config hexer.config rebar.lock TEST-*.xml -/eqc/ /bench/ coverage.info coverage-jiffy.info coverage-html/ -.eqc-info -current_counterexample.eqc *.gcda *.gcno .cache diff --git a/Makefile b/Makefile index 71da10cb..2b4dabc2 100644 --- a/Makefile +++ b/Makefile @@ -9,7 +9,6 @@ clean: rm -rf logs rm -rf .eunit rm -f test/*.beam - rm -rf eqc rm -rf _build rm -f c_src/*.gcno c_src/*.gcda c_src/ryu/*.gcno c_src/ryu/*.gcda rm -f coverage.info coverage-jiffy.info @@ -30,10 +29,8 @@ eunit: check: build eunit -check-with-eqc: install_eqc build eunit - -install_eqc: - ./test/install_eqc_mini.sh +check-with-proper: + $(REBAR) as proper eunit # macos: # brew install lcov on macos @@ -42,7 +39,7 @@ install_eqc: # coverage: $(MAKE) clean - CFLAGS="--coverage -O0" CXXFLAGS="--coverage -O0" LDFLAGS="--coverage" $(MAKE) check-with-eqc + CFLAGS="--coverage -O0" CXXFLAGS="--coverage -O0" LDFLAGS="--coverage" $(MAKE) check-with-proper @lcov --capture --directory c_src -o coverage.info --ignore-errors inconsistent,unsupported @lcov --extract coverage.info '*/c_src/*' --exclude '*/ryu/*' -o coverage-jiffy.info --ignore-errors inconsistent,unsupported @genhtml coverage-jiffy.info -o coverage-html --title "jiffy lcov report" @@ -56,4 +53,4 @@ release: erlc -o test/ $< -.PHONY: all clean distclean depends build etap eunit check coverage +.PHONY: all clean distclean depends build etap eunit check check-with-proper coverage diff --git a/c_src/encoder.c b/c_src/encoder.c index c58ccbcd..7ac7c9fc 100644 --- a/c_src/encoder.c +++ b/c_src/encoder.c @@ -276,8 +276,16 @@ enc_ensure(Encoder* e, size_t req) return 0; } + // If we have_buffer we don't want to short-cut return unless that + // buffer size is big enough. If it isn't, release it and allocate a + // newer one of the right size. Otherwise we could return a small + // buffer then risk a memory overrwrite on a memcpy. if(e->have_buffer) { - return 1; + if(req < (e->buffer.size - e->i)) { + return 1; + } + enif_release_binary(&e->buffer); + e->have_buffer = 0; } } diff --git a/rebar.config b/rebar.config index 760e4c1e..0c2e033c 100644 --- a/rebar.config +++ b/rebar.config @@ -17,3 +17,12 @@ {eunit_opts, [ verbose ]}. + +% Use in CI only. It's totaly optional +% +{profiles, [ + {proper, [ + {deps, [{proper, "1.5.0"}]}, + {erl_opts, [{d, 'WITH_PROPER'}]} + ]} +]}. diff --git a/rebar.config.script b/rebar.config.script index 375ef6f5..cca51fd9 100644 --- a/rebar.config.script +++ b/rebar.config.script @@ -2,31 +2,6 @@ % See the LICENSE file for more information. % -% Only run the EQC checks when EQC is present. -% -% We have a helper tests/install_eqc_mini.sh script to install it ./eqc, so we -% look for it there first, and add that to the code path. Users may have also -% installed it directly into their OTP distribution, which should be fine, -% code:which(eqc) should find that as well. -% -LocalEQC = filename:join([filename:dirname(SCRIPT), "eqc", "ebin"]), -case filelib:is_dir(LocalEQC) of - true -> code:add_patha(LocalEQC); - false -> ok -end, -HaveEQC = code:which(eqc) =/= non_existing, -ErlOpts = if not HaveEQC -> []; true -> - [{d, 'HAVE_EQC'}] -end, - -Config1 = case lists:keyfind(erl_opts, 1, CONFIG) of - {erl_opts, Opts} -> - NewOpts = {erl_opts, Opts ++ ErlOpts}, - lists:keyreplace(erl_opts, 1, CONFIG, NewOpts); - false -> - CONFIG ++ [{erl_opts, ErlOpts}] -end, - IsRebar2 = case lists:keyfind(rebar, 1, application:loaded_applications()) of {rebar, _Desc, Vsn} -> case string:split(Vsn, ".") of @@ -39,9 +14,9 @@ end, case IsRebar2 of true -> - Config1; + CONFIG; false -> - Config1 ++ [ + CONFIG ++ [ {plugins, [{pc, "~> 1.15"}]}, case os:type() of {win32, _} -> {artifacts, ["priv/jiffy.dll"]}; diff --git a/test/install_eqc_mini.sh b/test/install_eqc_mini.sh deleted file mode 100755 index 5614c752..00000000 --- a/test/install_eqc_mini.sh +++ /dev/null @@ -1,40 +0,0 @@ -#!/bin/sh -# -# Install EQC mini in $project_dir/eqc, where rebar config will find it. -# -# Note: mind what version it's compiled for and make sure to test it on -# Erlang version no more than two major version numbers away from it. -# So, for example, 25 should be good for 25, 26 and 27. -# - -set -eu - -EQC_VERSION="2.02.0" -EQC_URL="https://www.quviq.com/downloads/eqcR25-${EQC_VERSION}.zip" -EQC_ZIP_DIR="Quviq QuickCheck Mini version ${EQC_VERSION} for OTP 25" - -PROJECT_DIR="$(cd "$(dirname "$0")/.." && pwd)" -INSTALL_DIR="${PROJECT_DIR}/eqc" - -if [ -d "${INSTALL_DIR}/ebin" ]; then - echo "Already installed at ${INSTALL_DIR}" - exit 0 -fi - -TMPDIR="${TMPDIR:-/tmp}" -ZIP_FILE="${TMPDIR}/eqc-mini-${EQC_VERSION}.zip" -EXTRACT_DIR="${TMPDIR}/eqc-mini-extract-$$" - -echo "Downloading ${EQC_URL}" -curl -sfL "${EQC_URL}" -o "${ZIP_FILE}" - -rm -rf "${EXTRACT_DIR}" -mkdir -p "${EXTRACT_DIR}" -unzip -q -o "${ZIP_FILE}" -d "${EXTRACT_DIR}" - -rm -rf "${INSTALL_DIR}" -mv "${EXTRACT_DIR}/${EQC_ZIP_DIR}/eqc-${EQC_VERSION}" "${INSTALL_DIR}" - -rm -rf "${EXTRACT_DIR}" "${ZIP_FILE}" - -echo "Installed in ${INSTALL_DIR}" diff --git a/test/jiffy_11_property_tests.erl b/test/jiffy_11_property_tests.erl index 15840f5d..395dcc4c 100644 --- a/test/jiffy_11_property_tests.erl +++ b/test/jiffy_11_property_tests.erl @@ -3,32 +3,42 @@ -module(jiffy_11_property_tests). --ifdef(HAVE_EQC). +% PropEr is optional used on CI only +-ifdef(WITH_PROPER). -compile(export_all). +-compile(nowarn_export_all). --include_lib("eqc/include/eqc.hrl"). --include_lib("eunit/include/eunit.hrl"). +-include("jiffy_prop.hrl"). -include("jiffy_util.hrl"). +% Keep ?BIN_INC_SIZE in sync with BIN_INC_SIZE in c_src/encoder.c. +-define(BIN_INC_SIZE, 2048). + + property_test_() -> - [ - run(prop_enc_dec), - run(prop_enc_dec_pretty), - run(prop_dec_trailer), - run(prop_enc_no_crash), - run(prop_dec_no_crash_bin), - run(prop_dec_no_crash_any), - run(prop_map_enc_dec) - ]. + ?JIFFY_QUICKCHECK(300, 1000). + + +% Props prop_enc_dec() -> - ?FORALL(Data, json(), begin - Data == jiffy:decode(jiffy:encode(Data)) - end). + ?FORALL(Data, json(), + begin + Data =:= jiffy:decode(jiffy:encode(Data)) + end + ). + + +prop_enc_dec_pretty() -> + ?FORALL(Data, json(), + begin + Data =:= jiffy:decode(jiffy:encode(Data, [pretty])) + end + ). prop_dec_trailer() -> @@ -44,62 +54,110 @@ prop_dec_trailer() -> ). -prop_enc_dec_pretty() -> - ?FORALL(Data, json(), - begin - Data == jiffy:decode(jiffy:encode(Data, [pretty])) - end - ). - - prop_map_enc_dec() -> ?FORALL(Data, json(), begin MapData = to_map_ejson(Data), - MapData == jiffy:decode(jiffy:encode(MapData), [return_maps]) + MapData =:= jiffy:decode(jiffy:encode(MapData), [return_maps]) end ). prop_enc_no_crash() -> - ?FORALL(Data, any(), begin catch jiffy:encode(Data), true end). + ?FORALL(Data, any_term(), begin catch jiffy:encode(Data), true end). prop_dec_no_crash_any() -> - ?FORALL(Data, any(), begin catch jiffy:decode(Data), true end). + ?FORALL(Data, any_term(), begin catch jiffy:decode(Data), true end). prop_dec_no_crash_bin() -> ?FORALL(Data, binary(), begin catch jiffy:decode(Data), true end). -opts() -> - [ - {numtests, [1000]} - ]. +% Go the extra mile to generate larger size strings to exercise values around +% our buffer size limit of 2KB. We want a string also with lots of funky +% escapes, especially shaped like ...lots of escapes ++ lots of ascii... +prop_enc_buffer_boundary() -> + ?FORALL({Bin, Opts}, {enc_stress_string(), enc_opts()}, + begin + Bin =:= jiffy:decode(jiffy:encode(Bin, Opts)) + andalso Bin =:= jiffy:decode(jiffy:encode(Bin, [force_utf8 | Opts])) + end +). + + +% Slashes short-cut our fast-forwards to make sure we tests both with and +% without escaping them. +enc_opts() -> + elements([[], [escape_forward_slashes]]). -apply_opts(Prop) -> - apply_opts(Prop, opts()). +enc_stress_string() -> + ?LET({Prefix, Run}, {enc_escape_prefix(), enc_ascii_run()}, + iolist_to_binary([Prefix, Run]) + ). + + +enc_escape_prefix() -> + ?LET({N, C}, {enc_prefix_len(), enc_escape_char()}, + binary:copy(<>, N) + ). -apply_opts(Prop, []) -> - Prop; +% With escapes we end up as 1 + 2*N sizes. Choose N to hit more buffer boundaries (2KB) -apply_opts(Prop, [{Name, Args} | Rest]) -> - NewProp = erlang:apply(eqc, Name, Args ++ [Prop]), - apply_opts(NewProp, Rest). +% that land it in the [2036, 2047] window of the initial 2048-byte buffer. +enc_prefix_len() -> + frequency([ + {5, choose(1018, 1023)}, % Close to the limit + {2, choose(1008, 1033)}, % A bit wider interval around the limit + {2, choose(0, 1200)}, % Small prefixes (this would be a default small int choice) + {1, choose(2030, 2080)} % Check over buffer up to 2 buffers worth + ]). -log(F, A) -> - io:format(standard_error, F, A). +% Two byte escapes +enc_escape_char() -> + elements([$\b, $\t, $\n, $\f, $\r, $", $\\]). -run(Name) -> - Prop = apply_opts(?MODULE:Name()), - {msg("~s", [Name]), [ - {timeout, 300, ?_assert(eqc:quickcheck(Prop))} - ]}. +% We also need strings made of non-escapes since want to test long runs +% over-buffer of ASCII only character. An escape char will short-cut it. +enc_ascii_run() -> + ?LET({Len, C}, {enc_run_len(), enc_run_char()}, + binary:copy(<>, Len) + ). + + +enc_run_len() -> + frequency([ + {3, choose(?BIN_INC_SIZE, 5000)}, % small overflow (needs ASan/valgrind) + {2, choose(8192, 70000)}, % crosses geometric chunk growth + {2, choose(1 bsl 20, 2 bsl 20)} % multi-MB: crashes even a plain build + ]). + + +% Suchthat filter for ascii only chars +enc_run_char() -> + ?SUCHTHAT(C, choose($\s, $~), C =/= $" andalso C =/= $\\). + + +% FORALL_TARGETED is fancy-pants target which uses simulated annealing to +% hill-climb towards some desired maxumum or minimum. We're doing what we did +% above, but just let the test automatically generate string lengths (Ns) +% closer the the desired BIN_INC_SIZE, instead of doing it by hand with +% frequency() + choose(). Let's keep both approaches for now just in case, it +% doesn't hurt the have a belt and suspenders. +prop_enc_boundary_targeted() -> + Run = binary:copy(<<"a">>, 4 * ?BIN_INC_SIZE), + ?FORALL_TARGETED(N, integer(0, 8 * ?BIN_INC_SIZE), + begin + ?MAXIMIZE((1 + 2 * N) rem ?BIN_INC_SIZE), + Bin = <<(binary:copy(<<$\b>>, N))/binary, Run/binary>>, + Bin =:= jiffy:decode(jiffy:encode(Bin)) + end + ). to_map_ejson({Props}) -> @@ -113,38 +171,30 @@ to_map_ejson(Val) -> % Random any term generation -any() -> - ?SIZED(Size, any(Size)). +any_term() -> + ?SIZED(Size, any_term(Size)). -any(0) -> +any_term(0) -> any_value(); -any(S) -> +any_term(Size) -> oneof(any_value_types() ++ [ - ?LAZY(any_list(S)), - ?LAZY(any_tuple(S)) + ?LAZY(any_list(Size)), + ?LAZY(any_tuple(Size)) ]). any_value() -> oneof(any_value_types()). -% As of OTP 27 0.0 =/= -0.0 so we cannot use exact matching on round trips any -% longer. Therefore we test the 0.0 and -0.0 round-trip explicilty somewhere -% else but here we exclude it. We don't want to use == for term matching -% either, because then we'd be losing a check that ints stay as ints and floats -% as floats. -% -real_non_neg_0() -> - ?SUCHTHAT(F, real(), F =/= -0.0). any_value_types() -> [ - largeint(), - int(), - real(), - atom(), + large_int(), + integer(), + float(), + atom_gen(), binary() ]. @@ -154,7 +204,7 @@ any_list(0) -> any_list(Size) -> ListSize = Size div 5, - vector(ListSize, any(Size div 2)). + vector(ListSize, any_term(Size div 2)). any_tuple(0) -> @@ -172,18 +222,18 @@ json() -> json(0) -> oneof([ - json_null(), - json_true(), - json_false(), + null, + true, + false, json_number(), json_string() ]); json(Size) -> frequency([ - {1, json_null()}, - {1, json_true()}, - {1, json_false()}, + {1, null}, + {1, true}, + {1, false}, {1, json_number()}, {1, json_string()}, {5, ?LAZY(json_array(Size))}, @@ -191,24 +241,15 @@ json(Size) -> ]). -json_null() -> - null. - - -json_true() -> - true. - - -json_false() -> - false. - - +% As of OTP 27, 0.0 =/= -0.0, so exclude -0.0 here (covered explicitly +% elsewhere) to keep exact round-trip matching (which also checks ints stay +% ints). json_number() -> - oneof([largeint(), int(), real_non_neg_0()]). + oneof([large_int(), integer(), ?SUCHTHAT(F, float(), F =/= -0.0)]). json_string() -> - utf8(). + json_utf8(). json_array(0) -> @@ -227,28 +268,42 @@ json_object(Size) -> combiner() -> - ?SIZED( - Size, - ?LET( - L, - vector((Size div 4) + 1, oneof([$\r, $\n, $\t, $\s])), + ?SIZED(Size, + ?LET(L, vector((Size div 4) + 1, oneof([$\r, $\n, $\t, $\s])), list_to_binary(L) ) ). +% PropEr unlike EQC doesn't seem to have a large int so we build one here +large_int() -> + ?LET({I, Shift}, {integer(), choose(0, 96)}, I bsl Shift). -atom() -> - ?LET(L, ?SIZED(Size, vector(Size rem 254, char())), list_to_atom(L)). +% Atom names (LATIN1) +atom_gen() -> + ?SIZED(Size, + ?LET(Cs, vector(Size rem 254, choose(0, 16#FF)), list_to_atom(Cs)) + ). -%% XXX: Add generators -% -% We should add generators that generate JSON binaries directly -% so we can test things that aren't produced by the encoder. -% -% We should also have a version of the JSON generator that inserts -% errors into the JSON that we can test for. +% Valid UTF-8 binaries +json_utf8() -> + ?LET(Cps, list(unicode_char()), + unicode:characters_to_binary(Cps, unicode, utf8) + ). + + +unicode_char() -> + ?SUCHTHAT(C, + frequency([ + {5, choose(16#20, 16#7E)}, % ASCII + {2, choose(16#00, 16#1F)}, % Control chars + {2, choose(16#80, 16#7FF)}, % 2-byte + {2, choose(16#800, 16#FFFF)}, % 3-byte + {1, choose(16#10000, 16#10FFFF)} % 4-byte + ]), + C < 16#D800 orelse C > 16#DFFF + ). -endif. diff --git a/test/jiffy_23_enc_buffer_boundary_tests.erl b/test/jiffy_23_enc_buffer_boundary_tests.erl new file mode 100644 index 00000000..c50af5fe --- /dev/null +++ b/test/jiffy_23_enc_buffer_boundary_tests.erl @@ -0,0 +1,60 @@ +% This file is part of Jiffy released under the MIT license. +% See the LICENSE file for more information. + +-module(jiffy_23_enc_buffer_boundary_tests). + + +-include_lib("eunit/include/eunit.hrl"). +-include("jiffy_util.hrl"). + + +% Specifically test large string around our buffer size of 2KB. We'd like the string +% to have lot of escapes but also runs of plain ASCII. + +roundtrip(Bin) -> + ?assertEqual(Bin, dec(enc(Bin))), + ?assertEqual(Bin, dec(enc(Bin, [force_utf8]))). + +lots_of_escapes_followed_by_ascii_test() -> + Bin = <<(binary:copy(<<$\b>>, 1018))/binary, (binary:copy(<<"a">>, 4096))/binary>>, + roundtrip(Bin). + +% Test around the 2KB boundary +boundary_sweep_2k_test_() -> + Run = binary:copy(<<"a">>, 4100), + {timeout, 60, fun() -> + lists:foreach(fun(N) -> + Bin = <<(binary:copy(<<$\b>>, N))/binary, Run/binary>>, + roundtrip(Bin) + end, lists:seq(990, 1060)) + end}. + +% Test around the doubling growth (we grow our buffer to MAX_CHUNK_SIZE = 64KB) +boundary_sweep_chunk_growth_test_() -> + Run = binary:copy(<<"a">>, 70000), + {timeout, 120, fun() -> + lists:foreach(fun(N) -> + Bin = <<(binary:copy(<<$\b>>, N))/binary, Run/binary>>, + roundtrip(Bin) + end, lists:seq(64450, 64500)) + end}. + +% A test for escapes + forward slashes +forward_slash_test_() -> + Run = binary:copy(<<"https://example.com/a/b/">>, 4096), + {timeout, 30, fun() -> + lists:foreach(fun(N) -> + Bin = <<(binary:copy(<<$\b>>, N))/binary, Run/binary>>, + roundtrip(Bin) + end, lists:seq(1015, 1025)) + end}. + +% A test for escapae + forward slashes with forward slash escape option +escape_forward_slashes_test_() -> + Run = binary:copy(<<"a">>, 4100), + {timeout, 30, fun() -> + lists:foreach(fun(N) -> + Bin = <<(binary:copy(<<$\b>>, N))/binary, Run/binary>>, + ?assertEqual(Bin, dec(enc(Bin, [escape_forward_slashes]))) + end, lists:seq(1015, 1025)) + end}. diff --git a/test/jiffy_prop.hrl b/test/jiffy_prop.hrl new file mode 100644 index 00000000..21c337f0 --- /dev/null +++ b/test/jiffy_prop.hrl @@ -0,0 +1,22 @@ +% This file is part of Jiffy released under the MIT license. +% See the LICENSE file for more information. +% + +% Use from -ifdef(WITH_PROPER) sections. + +-include_lib("proper/include/proper.hrl"). +-include_lib("eunit/include/eunit.hrl"). + +% Auto-discover every prop_*/0 function exported by the including module. Idea copied from CouchDB. +% The ['prop_' .. 'prop`'] construct picks atoms beginning with "prop_" +-define(JIFFY_QUICKCHECK(Timeout, NumTests), + [ + {atom_to_list(F_), + {timeout, Timeout, + ?_assert(proper:quickcheck(?MODULE:F_(), + [{numtests, NumTests}, {to_file, user}]))}} + || {F_, 0} <- ?MODULE:module_info(exports), + F_ > 'prop_', F_ < 'prop`' + ]). + +-define(JIFFY_QUICKCHECK(Timeout), ?JIFFY_QUICKCHECK(Timeout, 200)).