From 5c48c5650c08079d72828fa68e83a280425f152b Mon Sep 17 00:00:00 2001 From: colourbill-ctrl Date: Sat, 12 Sep 2026 16:30:25 -0700 Subject: [PATCH] fix(#2538): refuse a JSON parametric curve without exactly its parameters --- .../parametric-curve-params-json.cpp | 194 ++++++++++++++++++ Build/Cmake/Testing/CMakeLists.txt | 64 ++++++ IccJSON/IccLibJSON/IccTagJson.cpp | 78 +++++-- 3 files changed, 316 insertions(+), 20 deletions(-) create mode 100644 .github/ci/regression/parametric-curve-params-json.cpp diff --git a/.github/ci/regression/parametric-curve-params-json.cpp b/.github/ci/regression/parametric-curve-params-json.cpp new file mode 100644 index 000000000..108622c62 --- /dev/null +++ b/.github/ci/regression/parametric-curve-params-json.cpp @@ -0,0 +1,194 @@ +/* + File: parametric-curve-params-json.cpp + + Contains: CTest helper for #2538, the uninitialised parameters that + CIccTagJsonParametricCurve::ParseJson() used to accept. + + SetFunctionType() allocates m_dParam for the function type's parameter + count with a bare new[] and fills nothing. The reader then wrote only + min(supplied, required) entries -- none at all when "params" was missing or + not an array -- and still returned true, so Write() encoded heap contents. + Measured on master fb5f65d0 with iccFromJson then iccToJson on an ASan + build, where the unwritten slots carry ASan's 0xbe malloc fill: + + functionType 4, params [2.4] -> 2.3999939, then six -0.3725433 + functionType 4, no params -> seven -0.3725433 + functionType 4, params "x" -> seven -0.3725433 + functionType 65540, seven params -> seven -0.3725433, function type 4 + functionType -65532, seven params -> seven -0.3725433, function type 4 + functionType 4, eight params -> the first seven, silently truncated + functionType 0, params [] -> accepted, its one parameter never written + + Rows four and five are not short arrays. functionType was read as an int + and the parameter count came from a switch on that int, while + SetFunctionType() received it narrowed to 16 bits: 65540 found no case (count + 0, nothing read) yet allocated seven slots as type 4. + + The XML twin refuses every one of these ("data.GetSize() != GetNumParam()", + plus a bounded function-type parse from #1851); measured on the same build, + iccFromXml writes no profile for the short, empty, long and 65540 variants + of Testing's #1851 control. This helper holds the JSON reader to that + contract. + + The reader is driven directly rather than through a profile document, as + in colorant-count-narrowing-json.cpp: ParseJson() is the function under fix, + and a profile carrying one tag draws unrelated validator failures. + + The discriminators are the return value and, for accepted documents, every + parameter's value. Neither depends on what the heap held, so the unfixed + reader fails this helper on a plain build, not just under a sanitizer -- + measured against master fb5f65d0, exactly the seven cases marked below + fail, each observing true. The accepted + cases compare all parameters of every known function type, so a count table + that is off by one for any type fails its own case even though the refusal + cases would pass. + + The unknown-function-type case pins behaviour this fix deliberately did NOT + change: type 5 has no defined parameters, nothing is left uninitialised, and + the binary reader keeps unknown types too, so it is still accepted. + + Exit codes: + 0 - expected results observed + 1 - unexpected result +*/ + +#include "IccTagJson.h" + +#include +#include + +static const char *kName = "parametric-curve-params-json"; + +// A parametricCurveType "data" object. pParams == NULL omits "params". +static IccJson makeCurve(long long functionType, const double *pParams, + size_t nParams) +{ + IccJson j = IccJson::object(); + j["type"] = "parametricCurveType"; + j["functionType"] = functionType; + if (pParams) { + IccJson arr = IccJson::array(); + for (size_t i = 0; i < nParams; i++) + arr.push_back(pParams[i]); + j["params"] = arr; + } + return j; +} + +// Parse j, and require it to be refused. +static int refusedCase(const IccJson &j, const char *label) +{ + CIccTagJsonParametricCurve tag; + std::string parseStr; + + if (tag.ParseJson(j, parseStr)) { + std::fprintf(stderr, + "%s: FAIL %s (ParseJson returned true, function type %u, " + "%u parameters)\n", + kName, label, (unsigned)tag.GetFunctionType(), + (unsigned)tag.GetNumParam()); + return 1; + } + + std::fprintf(stdout, "%s: PASS %s\n", kName, label); + return 0; +} + +// Parse j, and require it to be accepted as nFunctionType with exactly the +// nExpect parameters in pExpect. Values are chosen to be exact in a float. +static int acceptedCase(const IccJson &j, icUInt16Number nFunctionType, + const double *pExpect, icUInt16Number nExpect, + const char *label) +{ + CIccTagJsonParametricCurve tag; + std::string parseStr; + + if (!tag.ParseJson(j, parseStr)) { + std::fprintf(stderr, "%s: FAIL %s (ParseJson returned false: %s)\n", + kName, label, parseStr.c_str()); + return 1; + } + + if (tag.GetFunctionType() != nFunctionType || tag.GetNumParam() != nExpect) { + std::fprintf(stderr, + "%s: FAIL %s (function type %u with %u parameters, expected " + "%u with %u)\n", + kName, label, (unsigned)tag.GetFunctionType(), + (unsigned)tag.GetNumParam(), (unsigned)nFunctionType, + (unsigned)nExpect); + return 1; + } + + const icFloatNumber *pParams = tag.GetParams(); + for (icUInt16Number i = 0; i < nExpect; i++) { + if (!pParams || pParams[i] != (icFloatNumber)pExpect[i]) { + std::fprintf(stderr, "%s: FAIL %s (parameter %u is %g, expected %g)\n", + kName, label, (unsigned)i, + pParams ? (double)pParams[i] : 0.0, pExpect[i]); + return 1; + } + } + + std::fprintf(stdout, "%s: PASS %s\n", kName, label); + return 0; +} + +int main() +{ + int failures = 0; + + static const double kSeven[] = { 2.5, 0.75, 0.25, 0.125, 0.0625, 0.5, 0.375 }; + static const double kEight[] = { 2.5, 0.75, 0.25, 0.125, 0.0625, 0.5, 0.375, 123.0 }; + static const double kOne[] = { 2.5 }; + + // Refused: each of these seven returned true against master. + failures += refusedCase(makeCurve(4, kOne, 1), + "type 4 with one of its seven parameters is refused"); + failures += refusedCase(makeCurve(4, NULL, 0), + "type 4 with no params member is refused"); + { + IccJson j = makeCurve(4, NULL, 0); + j["params"] = "not-an-array"; + failures += refusedCase(j, "type 4 with a non-array params member is refused"); + } + failures += refusedCase(makeCurve(65540, kSeven, 7), + "functionType 65540, which narrowed to 4, is refused"); + failures += refusedCase(makeCurve(-65532, kSeven, 7), + "functionType -65532, which narrowed to 4, is refused"); + failures += refusedCase(makeCurve(4, kEight, 8), + "type 4 with eight parameters is refused, not truncated"); + failures += refusedCase(makeCurve(0, kOne, 0), + "type 0 with an empty params array is refused"); + + // Refused before and after: functionType itself is required. + { + IccJson j = makeCurve(4, kSeven, 7); + j.erase("functionType"); + failures += refusedCase(j, "a curve with no functionType is refused"); + } + + // Accepted: every known type with exactly its count, compared value by value. + static const icUInt16Number kCounts[] = { 1, 3, 4, 5, 7 }; + static const char *kLabels[] = { + "type 0 with exactly 1 parameter is accepted intact", + "type 1 with exactly 3 parameters is accepted intact", + "type 2 with exactly 4 parameters is accepted intact", + "type 3 with exactly 5 parameters is accepted intact", + "type 4 with exactly 7 parameters is accepted intact", + }; + for (icUInt16Number t = 0; t < 5; t++) + failures += acceptedCase(makeCurve(t, kSeven, kCounts[t]), t, kSeven, + kCounts[t], kLabels[t]); + + // Unchanged: an unknown type has no parameters to leave uninitialised. + failures += acceptedCase(makeCurve(5, kSeven, 0), 5, NULL, 0, + "unknown type 5 with no parameters is still accepted"); + + if (failures) { + std::fprintf(stderr, "%s: %d case(s) failed\n", kName, failures); + return 1; + } + + std::fprintf(stdout, "%s: all cases passed\n", kName); + return 0; +} diff --git a/Build/Cmake/Testing/CMakeLists.txt b/Build/Cmake/Testing/CMakeLists.txt index 37130da4c..262a1715f 100644 --- a/Build/Cmake/Testing/CMakeLists.txt +++ b/Build/Cmake/Testing/CMakeLists.txt @@ -4778,6 +4778,68 @@ function(iccdev_add_colorant_count_narrowing_json_test) endif() endfunction() +# #2538: CIccTagJsonParametricCurve::ParseJson() accepted a parametricCurveType +# with fewer parameters than its function type requires -- or none, or a +# non-array "params" -- and returned true with the rest of the tag's parameter +# array uninitialised, which Write() then encoded into the profile. A +# functionType outside 16 bits (65540, -65532) reached the same state by +# narrowing to type 4 after the parameter count had already been taken as 0. +# The helper drives the reader directly and asserts the return value and every +# parameter's value, so the unfixed reader fails it on a plain build, not only +# where a sanitizer fills the heap. Its accepted cases cover all five known +# function types at their exact counts, so a count table off by one for any type +# fails that type's case. +function(iccdev_add_parametric_curve_params_json_test) + if(NOT TARGET "${TARGET_LIB_ICCPROFLIB}" OR NOT TARGET "${TARGET_LIB_ICCJSON}") + return() + endif() + + iccdev_add_regression_executable(iccParametricCurveParamsJsonTest + "${ICCDEV_REPO_ROOT}/.github/ci/regression/parametric-curve-params-json.cpp" + ) + target_compile_features(iccParametricCurveParamsJsonTest PRIVATE cxx_std_17) + target_include_directories(iccParametricCurveParamsJsonTest PRIVATE + "${ICCDEV_REPO_ROOT}/IccJSON/IccLibJSON" + "${ICCDEV_REPO_ROOT}/IccProfLib" + ) + target_link_libraries(iccParametricCurveParamsJsonTest PRIVATE + ${TARGET_LIB_ICCJSON} ${TARGET_LIB_ICCPROFLIB}) + add_dependencies(check iccParametricCurveParamsJsonTest) + + if(WIN32) + add_test( + NAME iccdev.parametric-curve-params-json + COMMAND "$" + ) + else() + add_test( + NAME iccdev.parametric-curve-params-json + COMMAND + "${CMAKE_COMMAND}" -E env + ${ICCDEV_TEST_ENV} + "$" + ) + endif() + set_tests_properties(iccdev.parametric-curve-params-json PROPERTIES + TIMEOUT 300 + LABELS "iccdev;json;parametriccurve;issue-2538;regression" + ) + if(WIN32) + set(_parametric_params_json_windows_env_mods + "PATH=path_list_prepend:$" + "PATH=path_list_prepend:$" + "PATH=path_list_prepend:$" + ) + foreach(_runtime_path IN LISTS ICCDEV_WINDOWS_RUNTIME_PATHS) + list(APPEND _parametric_params_json_windows_env_mods + "PATH=path_list_prepend:${_runtime_path}") + endforeach() + set_tests_properties(iccdev.parametric-curve-params-json PROPERTIES + ENVIRONMENT_MODIFICATION "${_parametric_params_json_windows_env_mods}" + ) + endif() +endfunction() + # The XML twin of #2536, found while fixing it and fixed in the same commit: # CIccTagXmlColorantOrder::ParseXml() narrowed the same way at SetSize() and then # handed the UNNARROWED count to CIccUInt8Array::ParseArray() as its buffer size, @@ -7892,6 +7954,7 @@ if(WIN32) iccdev_add_namedcolor_findcolor_suffix_underflow_test() iccdev_add_colorant_count_narrowing_json_test() iccdev_add_colorant_count_narrowing_xml_test() + iccdev_add_parametric_curve_params_json_test() iccdev_add_colorant_count_narrowing_binary_test() iccdev_add_xform_create_tag_ownership_test() iccdev_add_xform_abstorel_adjust_test() @@ -8278,6 +8341,7 @@ iccdev_add_cmmsearch_namedcolor_ownership_test() iccdev_add_namedcolor_findcolor_suffix_underflow_test() iccdev_add_colorant_count_narrowing_json_test() iccdev_add_colorant_count_narrowing_xml_test() +iccdev_add_parametric_curve_params_json_test() iccdev_add_colorant_count_narrowing_binary_test() iccdev_add_xform_create_tag_ownership_test() iccdev_add_xform_abstorel_adjust_test() diff --git a/IccJSON/IccLibJSON/IccTagJson.cpp b/IccJSON/IccLibJSON/IccTagJson.cpp index 1d42ee51b..e4a046b7d 100644 --- a/IccJSON/IccLibJSON/IccTagJson.cpp +++ b/IccJSON/IccLibJSON/IccTagJson.cpp @@ -2135,27 +2135,65 @@ bool CIccTagJsonParametricCurve::ParseJson(const IccJson &j, std::string &parseS return ParseJson(j, icConvertFloat, parseStr); } -bool CIccTagJsonParametricCurve::ParseJson(const IccJson &j, icConvertType /*nType*/, std::string & /*parseStr*/) -{ - int funcType = 0; - if (!jGetValue(j, "functionType", funcType)) return false; - icUInt8Number nParam = 0; - // Determine param count from function type per ICC spec - switch (funcType) { case 0: nParam=1; break; case 1: nParam=3; break; case 2: nParam=4; break; case 3: nParam=5; break; case 4: nParam=7; break; default: nParam=0; } - if (!SetFunctionType((icUInt16Number)funcType)) return false; - if (jsonExistsField(j, "params") && j["params"].is_array()) { - const IccJson ¶ms = j["params"]; - bool paramsOverflow = false; - icUInt32Number nParams = icJsonSafeU32(params.size(), ¶msOverflow); - if (paramsOverflow) +bool CIccTagJsonParametricCurve::ParseJson(const IccJson &j, icConvertType /*nType*/, std::string &parseStr) +{ + // #2538: this reader used to accept a document that supplied fewer parameters + // than its function type requires, and returned true with the rest of + // m_dParam uninitialised. SetFunctionType() allocates m_dParam with a bare + // new[] and fills nothing; the loop below only wrote min(supplied, required) + // entries, and a missing or non-array "params" wrote none at all. Write() + // then encoded whatever the heap held, so iccFromJson + iccToJson on + // {"functionType": 4, "params": [2.4]} emitted six copies of ASan's 0xbebebebe + // fill (-0.3725...) and, on a Release build, whatever the allocator returned. + // + // A second way in reached the same state with no short array at all. + // functionType was read as an int, and the parameter count came from a local + // switch on that int while SetFunctionType() got it narrowed to 16 bits, so + // the two disagreed for any value outside 0..65535: 65540 found no case here + // (count 0, nothing read) but became function type 4 in the tag (7 slots + // allocated), leaving all seven uninitialised. -65532 did the same. + // + // Reading functionType as icUInt16Number refuses anything that does not fit + // the field -- jsonToValue range-checks the conversion -- and taking the count + // from GetNumParam() makes SetFunctionType()'s table the only one, so the + // count this reader enforces is by construction the size of the allocation. + icUInt16Number nFunctionType = 0; + if (!jGetValue(j, "functionType", nFunctionType)) { + parseStr += "parametricCurveType functionType is missing or out of range\n"; + return false; + } + if (!SetFunctionType(nFunctionType)) return false; + + // SetFunctionType() knows the five function types ICC.1 defines for + // parametricCurveType (0-4) and gives any other type zero parameters. Such a + // curve is still accepted, as it was before, since there is nothing to leave + // uninitialised; the binary reader likewise keeps unknown types rather than + // refusing them. + const icUInt16Number nParam = GetNumParam(); + if (!nParam) + return true; + + // The XML twin, CIccTagXmlParametricCurve::ParseXml(), has refused any count + // other than the exact one ("data.GetSize() != GetNumParam()") since the + // initial 2015 import, so a document it refuses was reaching the same tag + // through JSON. Requiring the same exact count here closes the uninitialised + // read and the divergence together. It also refuses a LONGER array, which + // this reader used to truncate silently; iccToJson always emits exactly + // GetNumParam() values, so no document it wrote is affected. + if (!jsonExistsField(j, "params") || !j["params"].is_array()) { + parseStr += "parametricCurveType params must be an array\n"; + return false; + } + const IccJson ¶ms = j["params"]; + if (params.size() != nParam) { + parseStr += "parametricCurveType params count does not match functionType\n"; + return false; + } + for (icUInt16Number i = 0; i < nParam; i++) { + double param = 0.0; + if (!jsonToValue(params[i], param)) return false; - icUInt8Number nParamToRead = nParams < nParam ? (icUInt8Number)nParams : nParam; - for (icUInt8Number i = 0; i < nParamToRead; i++) { - double param = 0.0; - if (!jsonToValue(params[i], param)) - return false; - m_dParam[i] = (icFloatNumber)param; - } + m_dParam[i] = (icFloatNumber)param; } return true; }