1808182 - Use of uninitialised values originating from NS_GetComplexLineBreaks (in nsPangoBreaker.cpp)

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Julian Seward [:jseward]]

Attachment: valgrind complainage

Running mochitests -f plain on valgrind over the holidays, I saw some
uninitialised-value complaints for
editor/libeditor/tests/browserscope/test_richtext2.html. These are shown in
the attachment.

The "root of the tree" is ComplexBreaker::GetBreaks in nsComplexBreaker.cpp.
This calls NS_ComplexLineBreaks, which IIUC writes memory which is inspected
by a loop immediately following the call, and subsequently calls onwards to
AddToCache:

  NS_GetComplexLineBreaks(aText, aLength, aBreakBefore);

  // As a very simple memory saving measure we trim off trailing elements that
  // are false before caching.
  auto* afterLastTrue = aBreakBefore + aLength;
  while (!*(afterLastTrue - 1)) {
    if (--afterLastTrue == aBreakBefore) {
      break;
    }
  }

  AddToCache(aText, aLength,
             nsTArray<uint8_t>(aBreakBefore, afterLastTrue - aBreakBefore));

The uninitialised values are reported at various locations, depending on
optimisation level and compiler. The attachment shows perhaps the most
understandable.

Some poking around suggests that the uninitialised values originate from
NS_GetComplexLineBreaks, in particular this:

  AutoTArray<PangoLogAttr, 2000> attrBuffer;
  // XXX(Bug 1631371) Check if this should use a fallible operation as it
  // pretended earlier.
  attrBuffer.AppendElements(aLength + 1);

AFAICS PangoLogAttr is a C struct with 15 one-bit fields, and no default
constructor (since it's C). Hence that AppendElements call adds space to the
array but doesn't initialise it. Doing the obvious thing ..

  memset(attrBuffer.Elements(), 0, attrBuffer.Length() * sizeof(PangoLogAttr));

makes all the uninitialised value complaints disappear.

Comment 1

[Jonathan Kew [:jfkthame]]

Nice find, this looks absolutely correct. I'll be happy to r+ a patch to "do the obvious (and right) thing" here if you've got a moment to put one up.

(This code should be going away fairly soon, and in practice the user-visible impact would be rare/minor, but nevertheless let's go ahead and apply the fix as the existing code is simply broken.)

Comment 2

[Julian Seward [:jseward]]

Attachment: Bug 1808182 - Use of uninitialised values originating from NS_GetComplexLineBreaks (in nsPangoBreaker.cpp). r=jfkthame.

NS_GetComplexLineBreaks (in nsPangoBreaker.cpp) adds elements of type
PangoLogAttr to an array with attrBuffer.AppendElements(aLength + 1);.
However, PangoLogAttr doesn't have a default constructor, so those elements
are uninitialised, and that eventually leaks back to the the caller,
ComplexBreaker::GetBreaks and are used in a couple of different places after
that. This patch fixes that by manually zeroing out the new area.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4c97e7235aad
Use of uninitialised values originating from NS_GetComplexLineBreaks (in nsPangoBreaker.cpp).  r=jfkthame.

Comment 4

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/4c97e7235aad