Bug 101649 – m_columnLogicalWidthChanged is manipulating uninitialized memory

• Component: Tables
• Status: NEW

[Julien Chaffraix]

Because Vector<int> doesn't initialize the memory it allocates, m_columnPos contains uninitialized memory that we query before setting it in RenderTable::setColumnPosition:

m_columnLogicalWidthChanged |= m_columnPos[index] != position;

As we use the information only to propagate some layout bits to the table parts, this bug is not security sensitive.

[Julien Chaffraix]

Created attachment 173134 [details]
Proposed memory fix: initialize m_columnPos.

[Alexey Proskuryakov]

(In reply to comment #0)
> Because Vector<int> doesn't initialize the memory it allocates

Is this accurate?

        explicit Vector(size_t size) 
            : m_size(size)
            , m_buffer(size)
        {
            if (begin())
                TypeOperations::initialize(begin(), end());

[Julien Chaffraix]

(In reply to comment #2)
> (In reply to comment #0)
> > Because Vector<int> doesn't initialize the memory it allocates
> 
> Is this accurate?
> 
>         explicit Vector(size_t size) 
>             : m_size(size)
>             , m_buffer(size)
>         {
>             if (begin())
>                 TypeOperations::initialize(begin(), end());

The VectorTraits magic resolves what TypeOperations::initialize does. For ints, the logic determines that we don't need an initialization (see below) so it is a no-op.

template<typename T> struct VectorTraits : VectorTraitsBase<IsPod<T>::value, T> { };

template<typename T> struct VectorTraitsBase<true, T>
{
    static const bool needsInitialization = false;
    ...
}

I didn't link to the Chromium bug but this was reported by Valgrind and Memory Sanitizer.

[Alexey Proskuryakov]

Hrm, I wonder why this behavior was chosen for. My understanding is that this does not match std::vector behavior, and is thus likely to lead to bugs like this one.

[Maciej Stachowiak]

(In reply to comment #4)
> Hrm, I wonder why this behavior was chosen for. My understanding is that this does not match std::vector behavior, and is thus likely to lead to bugs like this one.

It was for efficiency, to avoid zero-filling in cases where it seems unnecessary. POD types don't get initialized at all. We could choose to zero-fill them instead if that seems like a better tradeoff. In particular, if it had no effect on performance to change   template<typename T> struct VectorTraitsBase<true, T> (the VectorTraits for POD types) to set needsInitialization to true and canInitializeWithMemset to true, then we should definitely do it. If it does cause a perf regression, we'd have to figure out how we can avoid it.

[Eric Seidel (no email)]

Comment on attachment 173134 [details]
Proposed memory fix: initialize m_columnPos.

I'm confused how this is covered by existing tests?  This only shows up in valgrind?

[Julien Chaffraix]

Comment on attachment 173134 [details]
Proposed memory fix: initialize m_columnPos.

(In reply to comment #6)
> (From update of attachment 173134 [details])
> I'm confused how this is covered by existing tests?  This only shows up in valgrind?

This bug only shows up under Valgrind. What I meant is that it reproduces with our existing tests so we don't need extra testing.

Clearing the review flag until I can explore the initialize POD by default option. Note that this patch avoids the issue but may not be as generalized as it could.

[Ahmad Saleem]

Fixed in Blink (2014) but didn't have any test case - https://src.chromium.org/viewvc/blink?view=revision&revision=174384

https://github.com/WebKit/WebKit/blob/8e087fe5157f63a97276f5f3155ff14974929491/Source/WTF/wtf/Vector.h#L73

etc.

Is this needed now? Thanks!

[Darin Adler]

Change doesn’t seem particularly valuable to me unless it’s actively preventing someone from using a memory analysis tool. The code in the change is a bit inelegant.