ConvertFromParams discards the inverted luma/chroma matrix: BToA3 gets an empty matrix element, so the reverse transform fails to initialise and the profile is invalid (IccEncoding.cpp:371-388)

[colourbill-ctrl]

Summary

CIccDefaultEncProfileConverter::ConvertFromParams computes the inverse of the
luma/chroma matrix for the B2A direction, then throws it away. It creates a
CIccMpeMatrix and attaches it to the BToA3 tag without ever calling SetSize
or copying lumMtx into it, so the element goes out default-constructed:
0->0 channels, m_pMatrix == NULL, m_pConstants == NULL.

The result is a profile that ConvertFromParams reports as successfully
converted (icEncConvertOk) but whose reverse transform cannot be used at all
and which fails ICC validation as a critical error.

Found while fixing #1985 in the same function; filed separately rather than
folded into PR #1989, which deliberately does not touch this.

Mechanism

IccEncoding.cpp:371-388 on master (be114f02):

  if (bHaveLumMtx) {
    if (!icMatrixInvert3x3(&lumMtx[0])) {     // <-- inverse computed here
      delete pMpeTag;
      delete pIcc;
      return icEncConvertBadParams;
    }

    pMtx = (CIccMpeMatrix*)CIccMultiProcessElement::Create(icSigMatrixElemType);
    if (!pMtx) {
      delete pMpeTag;
      delete pIcc;
      return icEncConvertMemoryError;
    }
    pMpeTag->Attach(pMtx);                    // <-- attached empty
  }

Compare the A2B side of the same function, which is correct:

    if (!pMtx->SetSize(3, 3)) { ... }
    pLumMtx->GetValues(pMtx->GetMatrix(), 0, 9);   // <-- data copied in
    pLumMtx->GetValues(&lumMtx[0], 0, 9);
    pMpeTag->Attach(pMtx);

icMatrixInvert3x3 inverts lumMtx in place and nothing ever reads it
again, so the inverse is computed and discarded. The B2A block is missing both
the SetSize(3, 3) and the copy.

CIccMpeMatrix's constructor leaves m_nInputChannels = m_nOutputChannels = 0,
m_pMatrix = NULL, m_pConstants = NULL (IccMpeBasic.cpp), which is exactly
what the attached element keeps.

Observed

Driving the public converter handler with D65 primaries and a deliberately
non-identity luma matrix [0.5 0 0 0 2 0 0 0 4], whose inverse
[2 0 0 0 0.5 0 0 0 0.25] would be unmistakable if it were propagated:

ConvertFromParams -> 0

forward luma matrix supplied : [0.5 0 0  0 2 0  0 0 4]
expected inverse (B2A)      : [2 0 0  0 0.5 0  0 0 0.25]

  AToB3: 2 element(s), tag channels 3->3
    [0] CIccMpeMatrix      3->3  matrix=present constants=present  [0.5 0 0 0 2 0 0 0 4]
    [1] CIccMpeMatrix      3->3  matrix=present constants=present  [0.608292 0.285137 0.142568 0.33 0.6 0.06 0.0326717 0.108906 0.860356]
  BToA3: 2 element(s), tag channels 3->3
    [0] CIccMpeMatrix      3->3  matrix=present constants=present  [2.19721 -0.990629 -0.295012 -1.21551 2.23605 0.045481 0.0704234 -0.245426 1.16776]
    [1] CIccMpeMatrix      0->0  matrix=NULL constants=NULL

Begin(AToB3) -> true
Begin(BToA3) -> false   <-- transform unusable if false

Validate -> status 3
  Error! - BToA3Tag:(CIccMpeMatrix->CIccMpeMatrix Mis-matching number of channels in last process element!

Three separate consequences, all reproduced:

1. The inverse is silently lost. [2 0 0 0 0.5 0 0 0 0.25] appears nowhere
   in the profile. The B2A chain has an inverse-primaries matrix but no inverse
   luma step, so it is not the inverse of the A2B chain.
2. The B2A transform cannot be initialised.
   CIccTagMultiProcessElement::Begin walks the chain checking
   i->ptr->NumInputChannels() != last->NumOutputChannels(); the empty element
   reports 0 against the preceding element's 3, so Begin returns false. The
   reverse direction is dead, not merely inaccurate — note Begin(AToB3) is
   true in the same profile.
3. The profile is invalid. Validate returns icValidateCriticalError.

Apply is therefore not reachable for this element — Begin gates it — so
this is a correctness defect rather than a memory-safety one.

Scope

Reached whenever the colorEncodingParams struct carries a
ceptLumaChromaMatrixMbr (lmat) member of 9 or more floats, which is the only
thing that sets bHaveLumMtx. It is not conditional on any failure path, so
every profile the converter produces from params containing lmat has this
broken BToA3 tag. Profiles without lmat are unaffected.

Provenance

git log --all --reverse -S "if (!icMatrixInvert3x3(&lumMtx[0]))" -- IccProfLib/IccEncoding.cpp
gives 1f0a9dd2 (2015-09-29, initial repository creation) — the same commit
already bisected for #1817, #1982 and #1985. git log -S "pMtx->SetSize(3, 3)"
shows the B2A block has never carried a SetSize, so this is original, not a
regression.

Suggested fix

Mirror the A2B block: size the element, then copy the inverted matrix into it.

     pMtx = (CIccMpeMatrix*)CIccMultiProcessElement::Create(icSigMatrixElemType);
     if (!pMtx) {
       delete pMpeTag;
       delete pIcc;
       return icEncConvertMemoryError;
     }
+    if (!pMtx->SetSize(3, 3)) {
+      delete pMtx;
+      delete pMpeTag;
+      delete pIcc;
+      return icEncConvertMemoryError;
+    }
+    memcpy(pMtx->GetMatrix(), &lumMtx[0], 9 * sizeof(icFloatNumber));
     pMpeTag->Attach(pMtx);

(delete pMtx on the new failure path because pMtx is not owned by pMpeTag
until the Attach below it — the same ownership point PR #1989 corrects on the
A2B side.)

Worth deciding as part of the fix: whether an element that reports 0->0 with
null data should be rejected by CIccTagMultiProcessElement::Attach rather than
accepted and caught later by Begin/Validate. That would turn this class of
bug into an immediate failure instead of a profile that converts "successfully".

Related

• CIccDefaultEncProfileConverter::ConvertFromParams frees a borrowed colorEncodingParams element — heap use-after-free write, then double free (IccEncoding.cpp:259) #1985 / PR fix(#1985): stop the encoding converter freeing a borrowed params element #1989 — the borrowed-element use-after-free in the A2B half of this
  same block.
• Bisect: 1f0a9dd CIccCamConverter::ReferenceConditions() dbz #1817, CI Report: dbz at IccCAM.cpp:306 #1950, Research: validate ceptWhitePointLuminanceMbr #1980 — earlier defects in this converter.
• Research: CMM: honor D2B/B2D opt-out for dual-PCS profiles #1982 — dual-PCS transform selection, same bisect commit.

Reproduction source

Build against IccProfLib, e.g.:

clang++ -g -O0 -std=c++17 b2a-probe.cpp -o b2a-probe \
  -I<repo>/IccProfLib -I<build>/IccProfLib -L<build>/IccProfLib -lIccProfLib2d

// Probe: does ConvertFromParams emit a BToA3 matrix element carrying the
// inverted luma/chroma matrix, or an empty one?
#include "IccEncoding.h"
#include "IccProfile.h"
#include "IccTag.h"
#include "IccTagComposite.h"
#include "IccTagBasic.h"
#include "IccTagMPE.h"
#include "IccMpeBasic.h"
#include <cstdio>
#include <cstring>
#include <string>

#ifdef USEICCDEVNAMESPACE
using namespace iccDEV;
#endif

static bool attachFloats(CIccTagStruct *p, icSignature sig,
                         const icFloatNumber *v, icUInt32Number n)
{
  CIccTagFloat32 *t = (CIccTagFloat32 *)CIccTag::Create(icSigFloat32ArrayType);
  if (!t || !t->SetSize(n)) { delete t; return false; }
  for (icUInt32Number i = 0; i < n; i++) (*t)[i] = v[i];
  if (!p->AttachElem(sig, t)) { delete t; return false; }
  return true;
}

static void dumpMpe(const char *szTag, CIccTagMultiProcessElement *pMpe)
{
  if (!pMpe) { std::printf("  %s: ABSENT\n", szTag); return; }
  std::printf("  %s: %u element(s), tag channels %u->%u\n", szTag,
              (unsigned)pMpe->NumElements(),
              (unsigned)pMpe->NumInputChannels(),
              (unsigned)pMpe->NumOutputChannels());
  for (icUInt32Number i = 0; i < pMpe->NumElements(); i++) {
    CIccMultiProcessElement *pElem = pMpe->GetElement((int)i);
    if (!pElem) { std::printf("    [%u] NULL\n", (unsigned)i); continue; }
    std::printf("    [%u] %-18s %u->%u", (unsigned)i, pElem->GetClassName(),
                (unsigned)pElem->NumInputChannels(),
                (unsigned)pElem->NumOutputChannels());
    if (pElem->GetType() == icSigMatrixElemType) {
      CIccMpeMatrix *pM = (CIccMpeMatrix *)pElem;
      icFloatNumber *m = pM->GetMatrix();
      std::printf("  matrix=%s constants=%s",
                  m ? "present" : "NULL", pM->GetConstants() ? "present" : "NULL");
      if (m) {
        std::printf("  [");
        for (int k = 0; k < 9; k++) std::printf("%s%g", k ? " " : "", (double)m[k]);
        std::printf("]");
      }
    }
    std::printf("\n");
  }
}

int main()
{
  CIccTagStruct *pParams = (CIccTagStruct *)CIccTag::Create(icSigTagStructType);
  pParams->SetTagStructType(icSigColorEncodingParamsSruct);

  const icFloatNumber white[2] = {0.3127f, 0.3290f};
  const icFloatNumber red[2]   = {0.6400f, 0.3300f};
  const icFloatNumber green[2] = {0.3000f, 0.6000f};
  const icFloatNumber blue[2]  = {0.1500f, 0.0600f};
  // Deliberately NOT the identity, so a correctly-propagated inverse is
  // visibly different from the forward matrix.
  const icFloatNumber lumaMtx[9] = {
    0.5f, 0.0f, 0.0f,
    0.0f, 2.0f, 0.0f,
    0.0f, 0.0f, 4.0f
  };

  attachFloats(pParams, icSigCeptWhitePointChromaticityMbr, white, 2);
  attachFloats(pParams, icSigCeptMediumWhitePointChromaticityMbr, white, 2);
  attachFloats(pParams, icSigCeptLumaChromaMatrixMbr, lumaMtx, 9);
  attachFloats(pParams, icSigCeptRedPrimaryXYZMbr, red, 2);
  attachFloats(pParams, icSigCeptGreenPrimaryXYZMbr, green, 2);
  attachFloats(pParams, icSigCeptBluePrimaryXYZMbr, blue, 2);

  icHeader hdr;
  memset(&hdr, 0, sizeof(hdr));
  hdr.deviceClass = icSigColorSpaceClass;
  hdr.colorSpace = icSigRgbData;
  hdr.pcs = icSigXYZData;
  hdr.version = icVersionNumberV5;
  hdr.renderingIntent = icPerceptual;

  CIccProfilePtr newIcc = NULL;
  icStatusEncConvert stat =
    IIccEncProfileConverter::GetHandler()->ConvertFromParams(newIcc, pParams, &hdr);
  std::printf("ConvertFromParams -> %d\n", (int)stat);
  if (!newIcc) { delete pParams; return 1; }

  std::printf("\nforward luma matrix supplied : [0.5 0 0  0 2 0  0 0 4]\n");
  std::printf("expected inverse (B2A)      : [2 0 0  0 0.5 0  0 0 0.25]\n\n");

  dumpMpe("AToB3", (CIccTagMultiProcessElement *)newIcc->FindTag(icSigAToB3Tag));
  dumpMpe("BToA3", (CIccTagMultiProcessElement *)newIcc->FindTag(icSigBToA3Tag));

  CIccTagMultiProcessElement *pA2B = (CIccTagMultiProcessElement *)newIcc->FindTag(icSigAToB3Tag);
  CIccTagMultiProcessElement *pB2A = (CIccTagMultiProcessElement *)newIcc->FindTag(icSigBToA3Tag);
  std::printf("\nBegin(AToB3) -> %s\n", pA2B && pA2B->Begin() ? "true" : "false");
  std::printf("Begin(BToA3) -> %s   <-- transform unusable if false\n",
              pB2A && pB2A->Begin() ? "true" : "false");

  std::string report;
  icValidateStatus vs = newIcc->Validate(report);
  std::printf("\nValidate -> status %d\n", (int)vs);
  size_t pos = report.find("Matrix");
  if (pos != std::string::npos) {
    size_t b = report.rfind('\n', pos); b = (b == std::string::npos) ? 0 : b + 1;
    size_t e = report.find('\n', pos);
    std::printf("  %s\n", report.substr(b, e - b).c_str());
  }

  delete pParams;
  delete newIcc;
  return 0;
}

[github-actions]

Thanks for opening this issue.

The triage bot added initial labels based on the template and issue text. Maintainers may adjust labels as they review scope, reproduction details, CTest/regression needs, and security impact.

If you have questions, contact a listed Maintainer.