Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#9567 closed defect (fixed)

Small patch for stable behavior in wxGridCellAttrData

Reported by: kjones@… Owned by:
Priority: normal Milestone:
Component: GUI-generic Version: 2.8.7
Keywords: wxGridCellAttrData SetAttr NULL wxGridCellWithAttrArray wxGridCellAttr Cc: something.appropriate@…
Blocked By: Blocking:
Patch: yes

Description

This patch fixes a crash that can happen when calling wxGridCellAttrData::SetAttr twice on the same cell, as described below.

Imagine that you want to "cancel" any cell attributes on a given cell. It seems to me that the
logical way to proceed would be to call wxGridCellAttrData::SetAttr and pass in NULL to indicate
that you want to nullify any attributes that may or may not be present on that cell.

However, currently (i.e. without this patch), it is "safe" to call wxGridCellAttrData::SetAttr with NULL only if a non-null attribute has previously been assigned to that cell.

In cases were you want to make sure that no attributes are present on a cell, then given the current code you would need to perform a query on every cell first to see if it actually has attributes before you NULL them. This doesn't seem right.

With this very tiny patch I am submitting, you can now set NULL attributes on a cell without caring whether it previously had NULL or non-NULL attributes.

Without the patch, then here is the crash that can happen:

  1. Take a cell that already has NULL attributes. (For example, I will use row = 1, col = 1.)
  1. Call wxGridCellAttrData::SetAttr(NULL, 1, 1)
  1. That call to SetAttr will complete without any apparent difficulty.
  1. Now instantiate a non-NULL wxGridCellAttr and call SetAttr again.
  1. Call wxGridCellAttrData::SetAttr(p_nonNull, 1, 1)
  1. That will crash.

The reason why the second call crashes is because the first call created an entry in "m_attrs" (which is a wxGridCellWithAttrArray) and then stored the NULL in the array. Then in the second call to SetAttr we do NOT enter the wxNOT_FOUND block, when actually I believe that we should. A NULL attribute is the same as the cell not having an attribute, so therefore it should not be stored in the wxGridCellWithAttrArray. If the code had refrained from storing the NULL during the first call to SetAttr, then we would correctly enter the wxNOT_FOUND block during the second call, and we could then store our new (non-null) attribute without crashing the program.

Attachments (3)

mypatch.patch download (850 bytes) - added by kjones@… 6 years ago.
patch for grid.cpp as grid.cpp can be found in http://svn.wxwidgets.org/svn/wx/wxWidgets/branches/WX_2_8_BRANCH
mypatch.2.patch download (1.4 KB) - added by kjones@… 6 years ago.
SECOND version of this fix. This fixes TWO problems now instead of one.
attr.patch download (970 bytes) - added by neis 6 years ago.

Download all attachments as: .zip

Change History (13)

Changed 6 years ago by kjones@…

patch for grid.cpp as grid.cpp can be found in http://svn.wxwidgets.org/svn/wx/wxWidgets/branches/WX_2_8_BRANCH

comment:1 Changed 6 years ago by vadz

  • Resolution set to fixed
  • Status changed from new to closed

Thanks, I totally agree with your analysis and so applied your patch as r54176 to both trunk and 2.8 branch.

Thanks again!

comment:2 Changed 6 years ago by kjones@…

  • Resolution fixed deleted
  • Status changed from closed to reopened

Sorry to reopen this.

I continue to stand behind the merits of my earlier patch.

I now realize, however, that there was yet another subtle problem inside that same function.

I am attaching a SECOND patch that is still entirely devoted to the small function wxGridCellAttrData::SetAttr.

Just in case there is any confusion about whether my second patch should replace the first or whether they should both be applied (in the correct order), then the following text shows exactly how I now think the correct version of wxGridCellAttrData::SetAttr should look:


/// previous line is intended as wikiformatting for monospace 

void wxGridCellAttrData::SetAttr(wxGridCellAttr *attr, int row, int col)
{
    // If you pass in a NULL for attr, then:
    //        when there was a prior attr for this cell,
    //            then the prior attr is REMOVED, DecRef'ed, and not replaced.
    //        when there was no prior attr, then we do nothing,
    //            (meaning that this cell STILL has no attr).

    int n = FindIndex(row, col);
    if ( n == wxNOT_FOUND )
    {
        if ( attr )
        {
            // add the attribute
            m_attrs.Add(new wxGridCellWithAttr(row, col, attr));
        }
    }
    else
    {
        if ( attr )
        {
            // free the old attribute
            m_attrs[(size_t)n].attr->DecRef();
            // change the attribute
            m_attrs[(size_t)n].attr = attr;
        }
        else
        {
            // remove this attribute
            // No need to DecRef the attribute itself since this is
            // done be wxGridCellWithAttr's destructor!
            m_attrs.RemoveAt((size_t)n);
        }
    }
}

/// next line is intended as wikiformatting for monospace

Changed 6 years ago by kjones@…

SECOND version of this fix. This fixes TWO problems now instead of one.

comment:3 Changed 6 years ago by kjones@…

This is the rationale for my second patch:

When SetAttr is called for a cell that already has a valid (non-null) attribute, I have moved the line:

m_attrs[(size_t)n].attr->DecRef();

Prior to my second patch, that line was executed no matter whether the incoming replacement attribute was NULL or not.

The correct thing to do is to only call DecRef when we are NOT also planning to call m_attrs.RemoveAt.

Every call to "m_attrs.RemoveAt" will result in the destruction of a struct wxGridCellWithAttr. In the destructor ~wxGridCellWithAttr there is a call made to DecRef the corresponding wxGridCellAttr*.

The result is that the reference count is being decremented two-at-a-time and therefore "delete" is being called TOO SOON on pointers of type wxGridCellAttr*. "delete" is being called on these pointers even though there may still be objects in existence that would expect that they still have a valid claim over those attributes.

comment:4 Changed 6 years ago by kjones@…

By the way, PLEASE correct me if the following statement was incorrect:

"Every call to m_attrs.RemoveAt will result in the destruction of a struct wxGridCellWithAttr."

If I was wrong about that, then my patch is based on an incorrect belief. I do think, though, that the call to m_attrs.RemoveAt guarantees a call to the destructor of wxGridCellWithAttr.

comment:5 Changed 6 years ago by vadz

  • Resolution set to fixed
  • Status changed from reopened to closed

I again agree with your analysis of the problem but this time I don't quite agree with the fix. It looks like someone changed wxGridCellWithAttr to be object-like but forgot to finish doing it and so didn't implement its copy ctor and assignment operator correctly and forgot to remove the now unnecessary DecRef() calls.

So instead of keeping DecRef() before changing the attribute I implemented the correct reference counting semantics in assignment operator of this class and not this DecRef() is not needed at all.

Please let me know if you still have any problems after r54199 (and thanks for the patch, even if I didn't apply it, it was perfect this time), thanks!

Changed 6 years ago by neis

comment:6 follow-up: Changed 6 years ago by neis

  • Resolution fixed deleted
  • Status changed from closed to reopened

n addition to wxGridCellAttrData::SetAttr, there is also wxGridRowOrColAttrData::SetAttr, which probably should be fixed in a similar way. I've attached a patch - Vadim, could you please double check for correctness?

comment:7 in reply to: ↑ 6 ; follow-up: Changed 6 years ago by vadz

Replying to neis:

n addition to wxGridCellAttrData::SetAttr, there is also wxGridRowOrColAttrData::SetAttr, which probably should be fixed in a similar way. I've attached a patch - Vadim, could you please double check for correctness?

It looks correct to me, please apply it both branches, thanks!

comment:8 in reply to: ↑ 7 ; follow-up: Changed 6 years ago by neis

  • Resolution set to fixed
  • Status changed from reopened to closed

Replying to vadz:

Replying to neis:

n addition to wxGridCellAttrData::SetAttr, there is also wxGridRowOrColAttrData::SetAttr, which probably should be fixed in a similar way. I've attached a patch - Vadim, could you please double check for correctness?

It looks correct to me, please apply it both branches, thanks!

Actually, the fact that the patch was already partly included in trunk made me realize that it is not correct.
The point is that you didn't that wxGridCellAttr like I originally assumed but wxGridCellWithAttr, so the DecRef's in wxGridRowOrColAttrData::SetAttr are still needed to avoid a memory leak - at least I think so. What's happening in the original bug report is that it is calling SetAttr with the same attribute twice, so on the second call, we use DecRef to actually destroy the old attribute and then try to set the new attribute, but since it actually is the old one that doesn't work, so now I modified the function to just return if we try to set the same attribute that is already set.
The same problem seems to apply to wxGridCellWithAttr::operator=, so I added a check to avoid assigning the same attribute that's already assigned there as well.
More generally, this "DecRef/assign/IncRef" seems to generally have this problem, but I don't really know where to check/fix it...

Anyway, updated fix commited as r54244 (trunk) and r54245 (2_8_BRANCH).

comment:9 in reply to: ↑ 8 ; follow-up: Changed 6 years ago by vadz

Replying to neis:

Replying to vadz:

Replying to neis:

n addition to wxGridCellAttrData::SetAttr, there is also wxGridRowOrColAttrData::SetAttr, which probably should be fixed in a similar way. I've attached a patch - Vadim, could you please double check for correctness?

It looks correct to me, please apply it both branches, thanks!

Actually, the fact that the patch was already partly included in trunk made me realize that it is not correct.
The point is that you didn't that wxGridCellAttr like I originally assumed but wxGridCellWithAttr, so the DecRef's in wxGridRowOrColAttrData::SetAttr are still needed to avoid a memory leak - at least I think so.

The important thing is that wxGridCellWithAttrArray contains objects, not pointers, so it handles their reference count when they're updated in or deleted from the array.

What's happening in the original bug report is that it is calling SetAttr with the same attribute twice, so on the second call, we use DecRef to actually destroy the old attribute and then try to set the new attribute, but since it actually is the old one that doesn't work, so now I modified the function to just return if we try to set the same attribute that is already set.

In fact this problem wouldn't exist (and while this check is harmless it could also be omitted) if I didn't forget to check for self-assignment in wxGridCellWithAttr::operator=(), sorry about this and thanks a lot for noticing and fixing this bug!

comment:10 in reply to: ↑ 9 Changed 6 years ago by neis

Replying to vadz:

The important thing is that wxGridCellWithAttrArray contains objects, not pointers, so it handles their reference count when they're updated in or deleted from the array.

Yes, but the other important thing - the one what was confusing me in my first attempt and which apparently now confuses you as well - is that while wxGridCellAttrData uses wxGridCellWithAttr objects, wxGridRowOrColAttrData uses plain wxGridCellAttr (note: no With) pointers.

In fact this problem wouldn't exist (and while this check is harmless it could also be omitted) if I didn't forget to check for self-assignment in wxGridCellWithAttr::operator=(), sorry about this and thanks a lot for noticing and fixing this bug!

No, wxGridRowOrColAttrData doesn't actually use wxGridCellWithAttr's operator, so that check is still needed.

Note: See TracTickets for help on using tickets.