[webkit-dev] Eliminate potential null pointer dereference?

[John Yani]

On 20 April 2012 08:18, Alexey Proskuryakov <ap at webkit.org> wrote:
>
> I noticed a number of patches going in recently that add null checks, or refactor the code under the premise of "eliminating potential null dereference". What does that mean, and why are we allowing such patches to be landed?
>
> For example, this change doesn't look nice: <http://trac.webkit.org/changeset/114678/trunk/Source/WebCore/css/CSSStyleSelector.cpp>.

It means that there might be a situation when selector is null after
first loop finishes:

2310     while (selector) {
2311	        // Allow certain common attributes (used in the default
style) in the selectors that match the current element.
2312	        if (selector->isAttributeSelector() &&
!isCommonAttributeSelectorAttribute(selector->attribute()))
2313	            return true;
2314	        if (selectorListContainsUncommonAttributeSelector(selector))
2315	            return true;
2316	        if (selector->relation() != CSSSelector::SubSelector)
2317	            break;
2318	        selector = selector->tagHistory();
2319	    };

Now selector is null and we are trying to call tagHistory():

2320	
2321	    for (selector = selector->tagHistory(); selector; selector =
selector->tagHistory()) {
2322	        if (selector->isAttributeSelector())
2323	            return true;
2324	        if (selectorListContainsUncommonAttributeSelector(selector))
2325	            return true;
2326	    }

Which will result in segfault.

> We should not try to confuse ourselves with unusual for loop forms, and there is no explanation at all of why these changes are needed. No regression tests either.

To construct an obvious test it would be needed to move static inline
function "containsUncommonAttributeSelector" to either
CSSStyleSelector.h or a new file CSSStyleSelectorHelper.h

Then the test would be trivial:

WebCore::CSSSelector selector;
bool doesContain = WebCore::containsUncommonAttributeSelector(&selector);
ASSERT_TRUE(!doesContain);

Without the mentioned patch the above lines would result in a crash.

If your concern is only about missing explanation on why changes
required to add a test are not practical, I totally agree. Otherwise
I'm missing the point.

>
> - WBR, Alexey Proskuryakov
>
> _______________________________________________
> webkit-dev mailing list
> webkit-dev at lists.webkit.org
> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev