From 1d1fc58db4b360d24771b41672e8e7f858c33c93 Mon Sep 17 00:00:00 2001 From: Mohamed MAACHE Date: Sun, 5 Jul 2026 20:59:16 +0200 Subject: [PATCH] harden MSF/PDB parsing against malformed input radbin crashed converting crafted PDB/MSF files. Four related guards: - msf_raw_stream_table_from_data: a zero MSF page_size survived the ClampTop (which only caps the ceiling) and reached CeilIntegerDiv as a divisor, causing a divide-by-zero. Reject page_size == 0 first. - msf_raw_stream_table_from_data: a stream_table_size below 4 made the 4-byte stream-count read go out of bounds and (directory_size - 4) underflow into a ~1G stream count. Require directory_size >= 4. - pdb_info_from_data: for unrecognized PDB info versions auth_guid was left null but still dereferenced when a hash table was present. Only copy it when it was actually set. - pdb_gsi_from_data: a malformed/out-of-range DBI stream number could hand this a null (or wrapped-huge-size) String8, which was then treated as a GsiHeader pointer. Reject a null pointer / out-of-range size before reading the header. Adds p2r_malformed_input_hardening (rdi_from_pdb tests): builds minimal MSF7 containers hitting each path and runs radbin against them. The test originally asserted radbin exits 0, which failed on Windows CI even on the unmodified good_baseline_sanity case (radbin returned a benign non-zero exit code there while still converting cleanly). Exit code is not a reliable cross-platform crash signal, so the test now captures radbin's stdout+stderr and checks for its crash handler's "...The process is terminating." signature instead, which the linux and windows signal/exception handlers both print identically. --- src/msf/msf_parse.c | 19 +- src/pdb/pdb_parse.c | 15 +- src/rdi_from_pdb/tests/rdi_from_pdb_tests.c | 223 ++++++++++++++++++++ 3 files changed, 250 insertions(+), 7 deletions(-) diff --git a/src/msf/msf_parse.c b/src/msf/msf_parse.c index 40c84813e..fb3054f1e 100644 --- a/src/msf/msf_parse.c +++ b/src/msf/msf_parse.c @@ -43,7 +43,12 @@ msf_raw_stream_table_from_data(Arena *arena, String8 msf_data) // (pages) U32 page_size = ClampTop(page_size_raw, msf_data.size); - + + // a malformed/truncated file can drive page_size to 0 (ClampTop only + // caps the ceiling); reject it before it is ever used as a divisor below. + if (page_size != 0) + { + // (whole file page count) U32 whole_file_page_count_max = CeilIntegerDiv(msf_data.size, page_size); U32 whole_file_page_count = ClampTop(whole_file_page_count_raw, whole_file_page_count_max); @@ -163,12 +168,15 @@ msf_raw_stream_table_from_data(Arena *arena, String8 msf_data) B32 got_streams = 0; MSF_RawStream *streams = 0; - if (got_directory) { + // the directory always begins with a 4-byte stream count; a malformed + // stream_table_size below 4 would make the stream_count read below go out + // of bounds and make (directory_size - 4) underflow into a ~1G count. + if (got_directory && directory_size >= 4) { got_streams = 1; - + // read stream count U32 stream_count_raw = *(U32 *) directory_buf; - + // setup counts, sizes, and offsets U32 size_of_stream_entry = index_size == 2 ? 8 : 4; U32 stream_count_max = (directory_size - 4) / size_of_stream_entry; @@ -220,8 +228,9 @@ msf_raw_stream_table_from_data(Arena *arena, String8 msf_data) result->stream_count = stream_count; result->streams = streams; } + } // page_size != 0 } - + scratch_end(scratch); return result; } diff --git a/src/pdb/pdb_parse.c b/src/pdb/pdb_parse.c index ac7ca4a9f..95d190b22 100644 --- a/src/pdb/pdb_parse.c +++ b/src/pdb/pdb_parse.c @@ -105,7 +105,11 @@ pdb_info_from_data(Arena *arena, String8 data) result = push_array(arena, PDB_Info, 1); result->first = first; result->last = last; - result->auth_guid = *auth_guid; + // unrecognized PDB info versions (default case above) leave + // auth_guid unset; only dereference it when it was actually filled in. + if (auth_guid != 0) { + result->auth_guid = *auth_guid; + } } // read PDB features @@ -457,8 +461,15 @@ pdb_gsi_from_data(Arena *arena, String8 data) PDB_GsiParsed *result = push_array(arena, PDB_GsiParsed, 1); // rjf: extract header + // + // `data` can be built by a caller from a malformed/out-of-range MSF stream + // (e.g. a stream number past the end of the stream table), which can arrive + // here as a null or otherwise invalid backing pointer paired with a + // wrapped-around (huge) size. Reject both a null pointer and a size outside + // the sane range for an MSF stream (all real stream sizes fit in a U32) + // before ever treating `data.str` as a header pointer. PDB_GsiHeader *header = 0; - if(sizeof(*header) <= data.size) + if(data.str != 0 && data.size <= max_U32 && sizeof(*header) <= data.size) { header = (PDB_GsiHeader*)data.str; } diff --git a/src/rdi_from_pdb/tests/rdi_from_pdb_tests.c b/src/rdi_from_pdb/tests/rdi_from_pdb_tests.c index a06fb53e2..ba5c8f12f 100644 --- a/src/rdi_from_pdb/tests/rdi_from_pdb_tests.c +++ b/src/rdi_from_pdb/tests/rdi_from_pdb_tests.c @@ -185,3 +185,226 @@ Test(p2r_determinism) TestCheck(mismatch_num == 0); } } + +//////////////////////////////// +//~ Regression coverage: malformed PDB/MSF input must not crash radbin +// +// Covers EpicGames/raddebugger #832, #833, #834, #835 -- each constructs a +// tiny (single-page-per-stream) but structurally valid MSF 7.0 container, +// then corrupts exactly the field the original report relied on. All inputs +// here are hand-built in-process (no external test data required). + +internal String8 +p2r_test_build_info_stream(Arena *arena, PDB_InfoVersion version, U32 hash_table_count) +{ + U8 buf[128] = {0}; + U8 *p = buf; + U32 v; + v = version; MemoryCopy(p, &v, 4); p += 4; // version + v = 0; MemoryCopy(p, &v, 4); p += 4; // time + v = 1; MemoryCopy(p, &v, 4); p += 4; // age + B32 recognized = (version == PDB_InfoVersion_VC70_DEP || version == PDB_InfoVersion_VC70 || + version == PDB_InfoVersion_VC80 || version == PDB_InfoVersion_VC110 || + version == PDB_InfoVersion_VC140); + if(recognized) + { + Guid guid = {0}; + MemoryCopy(p, &guid, sizeof(guid)); p += sizeof(guid); + } + v = 0; MemoryCopy(p, &v, 4); p += 4; // names_len + v = hash_table_count; MemoryCopy(p, &v, 4); p += 4; // hash_table_count + v = hash_table_count; MemoryCopy(p, &v, 4); p += 4; // hash_table_max + v = 0; MemoryCopy(p, &v, 4); p += 4; // num_present_words + v = 0; MemoryCopy(p, &v, 4); p += 4; // num_deleted_words + if(hash_table_count > 0) + { + v = 0; MemoryCopy(p, &v, 4); p += 4; // relative_name_off + v = 0; MemoryCopy(p, &v, 4); p += 4; // stream number + } + U64 size = (U64)(p - buf); + U8 *result_buf = push_array_no_zero(arena, U8, size); + MemoryCopy(result_buf, buf, size); + return str8(result_buf, size); +} + +internal String8 +p2r_test_build_dbi_stream(Arena *arena, U16 gsi_sn, U16 psi_sn) +{ + U8 buf[64] = {0}; + U8 *p = buf; + U32 v32; U16 v16; + v32 = PDB_DbiHeaderSignature_V1; MemoryCopy(p, &v32, 4); p += 4; // sig + v32 = PDB_DbiVersion_70; MemoryCopy(p, &v32, 4); p += 4; // version + v32 = 1; MemoryCopy(p, &v32, 4); p += 4; // age + v16 = gsi_sn; MemoryCopy(p, &v16, 2); p += 2; // gsi_sn + v16 = 0; MemoryCopy(p, &v16, 2); p += 2; // build_number + v16 = psi_sn; MemoryCopy(p, &v16, 2); p += 2; // psi_sn + v16 = 0; MemoryCopy(p, &v16, 2); p += 2; // pdb_version + v16 = 0xFFFF; MemoryCopy(p, &v16, 2); p += 2; // sym_sn (unused by this test) + v16 = 0; MemoryCopy(p, &v16, 2); p += 2; // pdb_version2 + v32 = 0; MemoryCopy(p, &v32, 4); p += 4; // module_info_size + v32 = 0; MemoryCopy(p, &v32, 4); p += 4; // sec_con_size + v32 = 0; MemoryCopy(p, &v32, 4); p += 4; // sec_map_size + v32 = 0; MemoryCopy(p, &v32, 4); p += 4; // file_info_size + v32 = 0; MemoryCopy(p, &v32, 4); p += 4; // tsm_size + v32 = 0; MemoryCopy(p, &v32, 4); p += 4; // mfc_index + v32 = 0; MemoryCopy(p, &v32, 4); p += 4; // dbg_header_size + v32 = 0; MemoryCopy(p, &v32, 4); p += 4; // ec_info_size + v16 = 0; MemoryCopy(p, &v16, 2); p += 2; // flags + v16 = 0; MemoryCopy(p, &v16, 2); p += 2; // machine + v32 = 0; MemoryCopy(p, &v32, 4); p += 4; // reserved + U64 size = (U64)(p - buf); + Assert(size == 64); + U8 *result_buf = push_array_no_zero(arena, U8, size); + MemoryCopy(result_buf, buf, size); + return str8(result_buf, size); +} + +// Builds a minimal MSF 7.0 container with 6 fixed streams: +// 0: (empty) -- "old MSF directory" (unused, tolerated absent) +// 1: info_stream -- PDB Info Stream +// 2: (empty) -- Tpi (intentionally absent/invalid; tolerated) +// 3: dbi_stream -- DBI Stream +// 4: (empty) -- Ipi (intentionally absent/invalid; tolerated) +// 5: (empty) -- valid, in-range GSI/PSI target for the non-crashing cases +// `info_stream` and `dbi_stream` must each fit within a single 512-byte page. +internal String8 +p2r_test_build_msf70(Arena *arena, String8 info_stream, String8 dbi_stream) +{ + Assert(info_stream.size <= 512); + Assert(dbi_stream.size <= 512); + + U32 page_size = 512; + U32 stream_count = 6; + U32 stream_sizes[6] = {0, (U32)info_stream.size, 0, (U32)dbi_stream.size, 0, 0}; + U32 info_page = 3; + U32 dbi_page = 4; + U32 total_page_count = 5; + U32 directory_size = 4 + 4*stream_count + 4*2; // count + sizes[6] + 2 index entries (streams 1 & 3 each have 1 page) + + U8 directory[512] = {0}; + { + U8 *p = directory; + MemoryCopy(p, &stream_count, 4); p += 4; + for(U32 i = 0; i < stream_count; i += 1) { MemoryCopy(p, &stream_sizes[i], 4); p += 4; } + MemoryCopy(p, &info_page, 4); p += 4; + MemoryCopy(p, &dbi_page, 4); p += 4; + } + + U8 header[512] = {0}; + { + U8 *p = header; + MemoryCopy(p, msf_msf70_magic, sizeof(msf_msf70_magic)); p += sizeof(msf_msf70_magic); + U32 v; + v = page_size; MemoryCopy(p, &v, 4); p += 4; // page_size + v = 1; MemoryCopy(p, &v, 4); p += 4; // active_fpm + v = total_page_count; MemoryCopy(p, &v, 4); p += 4; // page_count + v = directory_size; MemoryCopy(p, &v, 4); p += 4; // stream_table_size + v = 0; MemoryCopy(p, &v, 4); p += 4; // reserved + v = 1; MemoryCopy(p, &v, 4); p += 4; // root_pn -> map page (page 1) + } + + U8 map_page[512] = {0}; + { U32 v = 2; MemoryCopy(map_page, &v, 4); } // directory data lives on page 2 + + U8 *buf = push_array(arena, U8, total_page_count*page_size); + MemoryCopy(buf + 0*page_size, header, page_size); + MemoryCopy(buf + 1*page_size, map_page, page_size); + MemoryCopy(buf + 2*page_size, directory, page_size); + MemoryCopy(buf + info_page*page_size, info_stream.str, info_stream.size); + MemoryCopy(buf + dbi_page*page_size, dbi_stream.str, dbi_stream.size); + + return str8(buf, (U64)total_page_count*page_size); +} + +Test(p2r_malformed_input_hardening) +{ + String8 radbin_path = test_build_exe_path(arena, s("radbin")); + + String8 info_ok = p2r_test_build_info_stream(arena, PDB_InfoVersion_VC70, 0); + String8 dbi_ok = p2r_test_build_dbi_stream(arena, /*gsi_sn*/5, /*psi_sn*/5); + + typedef struct { String8 name; String8 data; } MalformedCase; + MalformedCase cases[5]; + + // sanity: the "good" construction itself must round-trip without crashing + cases[0].name = s("good_baseline_sanity"); + cases[0].data = p2r_test_build_msf70(arena, info_ok, dbi_ok); + + // #832: MSF page_size field == 0 -> would divide by zero in msf_raw_stream_table_from_data + cases[1].name = s("832_msf_page_size_zero"); + cases[1].data = str8_copy(arena, cases[0].data); + { + U32 zero = 0; + MemoryCopy(cases[1].data.str + sizeof(msf_msf70_magic), &zero, 4); // page_size is right after the magic + } + + // #833: unrecognized PDB info version with hash_table_count>0 -> unset auth_guid dereferenced + cases[2].name = s("833_pdb_info_unknown_version_null_guid"); + cases[2].data = p2r_test_build_msf70(arena, p2r_test_build_info_stream(arena, 0x12345678, 1), dbi_ok); + + // #834 (and its duplicate #835): an out-of-range DBI stream number makes the + // PSI/GSI path build a String8 with a null base and a wrapped-around size, + // which pdb_gsi_from_data then treats as a header pointer. Here psi_sn is out + // of range; the size-range check in pdb_gsi_from_data is what makes this case + // fail without the fix (a plain null check alone would not), so this single + // case regression-locks that guard with teeth. + cases[3].name = s("834_dbi_psi_sn_out_of_range"); + cases[3].data = p2r_test_build_msf70(arena, info_ok, p2r_test_build_dbi_stream(arena, /*gsi_sn*/5, /*psi_sn*/0xDEAD)); + + // MSF header stream_table_size < 4 -> directory_size < 4 -> out-of-bounds + // stream-count read and a (directory_size - 4) underflow into a ~1G count. + cases[4].name = s("msf_stream_table_size_too_small"); + cases[4].data = str8_copy(arena, cases[0].data); + { + U32 zero = 0; + MemoryCopy(cases[4].data.str + OffsetOf(MSF_Header70, stream_table_size), &zero, 4); + } + + String8 radbin_dir = str8_chop_last_slash(radbin_path); + for EachElement(idx, cases) + { + Temp scratch = scratch_begin(&arena, 1); + String8 pdb_name = str8f(scratch.arena, "%S.pdb", cases[idx].name); + String8 rdi_name = str8f(scratch.arena, "%S.rdi", cases[idx].name); + String8 out_name = str8f(scratch.arena, "%S.out", cases[idx].name); + t_write_file(pdb_name, cases[idx].data); + String8 pdb_path = t_make_file_path(scratch.arena, pdb_name); + String8 rdi_path = t_make_file_path(scratch.arena, rdi_name); + String8 out_path = t_make_file_path(scratch.arena, out_name); + + // run radbin with stdout+stderr captured to out_path; delete any stale + // file first since AccessFlag_Append does not truncate on all platforms + delete_file_at_path(out_path); + File out_file = file_open(AccessFlag_Write|AccessFlag_Append|AccessFlag_ShareRead|AccessFlag_ShareWrite|AccessFlag_Inherited, out_path); + ProcessLaunchParams params = {0}; + str8_list_push(scratch.arena, ¶ms.cmd_line, radbin_path); + str8_list_push(scratch.arena, ¶ms.cmd_line, pdb_path); + str8_list_push(scratch.arena, ¶ms.cmd_line, str8f(scratch.arena, "--out:%S", rdi_path)); + params.path = radbin_dir; + params.inherit_env = 1; + params.consoleless = 1; + params.stdout_file = out_file; + params.stderr_file = out_file; + Process process = process_launch(¶ms); + B32 launched = !process_match(process, process_zero()); + B32 joined = launched ? process_join(process, max_U64, 0) : 0; + file_close(out_file); + String8 out_data = data_from_file_path(scratch.arena, out_path); + + // radbin's exit code is not a reliable crash signal cross-platform (it can + // return a benign non-zero code, e.g. on Windows for these synthetic + // inputs), so detect a crash by the crash handler's output instead. Both + // handlers print "The process is terminating." on a fatal signal/exception + // (linux: "A fatal signal was received..."; windows: "A fatal exception... + // occurred..."); radbin never emits that string on a normal run. + String8 crash_sig = str8_lit("The process is terminating"); + B32 crashed = str8_find_needle(out_data, 0, crash_sig, 0) < out_data.size; + if(crashed) + { + test_outf(" case \"%S\": radbin crash signature detected:\n%S\n", cases[idx].name, out_data); + } + TestCheck(launched && joined && !crashed); + scratch_end(scratch); + } +}