Review Request 127835: KHTML: Try again to fix Coverity memory leak in CSS background property

[Michael Pyne]

> On May 21, 2016, 1:56 p.m., Andrea Iacovitti wrote:
> > src/css/cssparser.cpp, lines 2143-2158
> > <https://git.reviewboard.kde.org/r/127835/diff/2/?file=464783#file464783line2143>
> >
> >     If value->length() is 0 we should not return true because we failed to parse the value.
> >     Testcase: right now with your change applyed the following invalid css declaration "background: left left" will lead to a crash.
> >     
> >     That said, here, value->length() should never be zero: if we are executing this part of code it means we successfully parsed values (did not exit from the while loop through the "goto failed" condition).
> >     However this actually doesn't happen because of a bug i just found involving parsing background-position when in shorthand property, i have a fix and will commit it soon.
> >      
> >     Afterwards, something like this code:
> >     
> >         //assert(value->length());
> >         if (value->length() == 1) {
> >             // Downgrade single-element lists to simple value
> >             retValue1 = value->item(0);
> >             if (value2->length()) {
> >                 retValue2 = value2->item(0);
> >             } else {
> >                 delete value2;
> >             }
> >         } else {
> >             retValue1 = value;
> >             if (value2->length()) {
> >                 retValue2 = value2;
> >             } else {
> >                 delete value2;
> >             }
> >         }
> >         return true;
> >     
> >     should be ok, but i don't know if it will make coverity happy.

Coverity was more annoyed that we'd leak currValue2 in some possible edge cases, so the reorganized code with that fix applied should still make Coverity happy I think. I'll look at revising the patch and then also testing with the testcase you provided on Bugzilla.


- Michael


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127835/#review95672
-----------------------------------------------------------


On May 18, 2016, 10:06 p.m., Michael Pyne wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127835/
> -----------------------------------------------------------
> 
> (Updated May 18, 2016, 10:06 p.m.)
> 
> 
> Review request for KDE Frameworks, Andrea Iacovitti and Bernd Buschinski.
> 
> 
> Repository: khtml
> 
> 
> Description
> -------
> 
> In KHTML commit b52325eb49 I attempted to fix a Coverity error (CID 257928) indicating that the CSS background property parser could sometimes leak `CSSValueImpl` objects.
> 
> Coverity shows that the leak is still possible (and I think it's correct, even with my change, in situations where the first return value is upgraded to a list while the second return value remains unset).
> 
> Because of that I'm trying a different approach to fix the leak by factoring out the code that promotes a saved return value into a value-list when needed, in the hopes that simpler code will be more correct.
> 
> Note for reviewers that, as far as I can see, the idea in the current code is that there's 3 levels of values, for 2 separate return values:
> 
> 1. "currValue", the value found during this pass through the main loop
> 2. "value", the saved return value when it's still only a single value (assigned from currValue)
> 3. "values", the saved return value when it's now a list of values (assigned from value and/or currValue)
> 
> Similar concepts apply to currValue2/value2/values2. I am unsure why there's a distinction between a single value and a value list in the return value -- it may be that this distinction is unneeded.
> 
> 
> Diffs
> -----
> 
>   src/css/cssparser.cpp 6684f4c 
> 
> Diff: https://git.reviewboard.kde.org/r/127835/diff/
> 
> 
> Testing
> -------
> 
> The updated code compiles without warnings in cssparser.cpp, and runs fine in Konqueror.
> 
> To test I tried going to the MDN site on the CSS background properties (including background-position in particular, since that's the only CSS property that requires the second value/return value). I then used the DOM Inspector tool to manually verify that the CSS background properties were properly read, that the examples rendered as before, etc.
> 
> The Acid3 test makes it to 92/100, but I'm pretty sure it was only at 92/100 before this change as well. ;)
> 
> 
> Thanks,
> 
> Michael Pyne
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20160521/a07f3bcb/attachment-0001.html>