Review Request 126480: Fix assorted memory leaks and undefined accesses

[Michael Pyne]

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

Review request for KDE Frameworks, David Faure, Martin Tobias Holmedahl Sandsmark, and Patrick Spendrin.


Repository: khtml


Description
-------

This is a compendium of proposed fixes to the outstanding "High Impact" issues Coverity has reported against KHTML, and which I in my unilateral and infinite wisdom have judged are actually valid (more opinions are welcome! It should be possible to register for Coverity access to KDE runs if you ask from your kde.org address).

Coverity issues issues include CID #s: 257995, 257996, 259759, 257994, 257927, 257928, 1019703, 1019704, 1340892, 1340908

Fix-specific notes:

A few fixes for leaked auxiliary structs used to calculate a return value simply used a QScopedPointer, unless a mere "delete foo" was all that was required.

CID 259759, tiled pixmap cache misusage: QCache can in theory delete an object you insert as soon as you insert it, so we must guard against that possibility. If re-entrancy or concurrent use of the cache were possible then we would also need to guard against objects obtained from the cache being deleted, but I don't think that's possible so I didn't add sanity checking for that.

CID 257994, leaked font face during CSS parsing: I'm not completely sure this is possible but if it is, malicious web sites could induce possibly-large memory leaks. The leak would occur if a font with an empty/invalid name leaked into this routine (I think it would be even more restrictive than that, the leak would only be possible if no other font family were valid).

CID 257928, leak (mis-parse?) during CSS background property parsing: In essence we manually parse a list of value pairs, which are read into currValue{,2} and then migrated from currValue{,2} into value{,2} (a single value) and values{,2} (a list) in turn. The end of the procedure assumes that value{,2} are either both single values or both value lists, but the code path might result in a value list for the first with only a single value for the second. That single value for the second part of the pair would then be leaked (since the values2 list is empty at this point).

CID 1019703, 1019704, DTD Tag IDs: Potentially serious? At some point we added a tag type for "COMMENT" (which I would think the parser would eliminate from the AST, but maybe that is no longer the case?). However we didn't expand out some auxiliary data vectors we use for the tags, which Coverity flagged as a potential invalid memory access.


Diffs
-----

  src/css/css_webfont.cpp 6754a30 
  src/css/cssparser.cpp 112c867 
  src/ecma/kjs_html.cpp cc3c08a 
  src/editing/htmlediting_impl.cpp d56c4a8 
  src/html/dtd.cpp 71cad5c 
  src/rendering/render_object.cpp 06dba1a 
  src/xml/dom_docimpl.cpp 4f0be5c 

Diff: https://git.reviewboard.kde.org/r/126480/diff/


Testing
-------

Everything compiles, clicking around in Konq on various websites and opening local XML files seems to work fine. I can't get CMake to build the autotests however (though there aren't very many anyways).


Thanks,

Michael Pyne

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20151222/2db5b4ae/attachment.html>