fix(#2538): refuse a JSON parametric curve without exactly its parameters

.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 <cstdio>
+#include <string>
+
+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;
+}

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 "$<TARGET_FILE:iccParametricCurveParamsJsonTest>"
+    )
+  else()
+    add_test(
+      NAME iccdev.parametric-curve-params-json
+      COMMAND
+        "${CMAKE_COMMAND}" -E env
+        ${ICCDEV_TEST_ENV}
+        "$<TARGET_FILE:iccParametricCurveParamsJsonTest>"
+    )
+  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:$<TARGET_FILE_DIR:iccParametricCurveParamsJsonTest>"
+      "PATH=path_list_prepend:$<TARGET_FILE_DIR:${TARGET_LIB_ICCPROFLIB}>"
+      "PATH=path_list_prepend:$<TARGET_FILE_DIR:${TARGET_LIB_ICCJSON}>"
+    )
+    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()

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 &params = j["params"];
-    bool paramsOverflow = false;
-    icUInt32Number nParams = icJsonSafeU32(params.size(), &paramsOverflow);
-    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 &params = 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;
 }