llvm/clang static analyzer reports

[Stephan Bergmann]

On 30/09/2020 21:36, Maarten Hoes wrote:
> [2]
> https://drive.google.com/file/d/1kbX_uo_jRqQkny1Qf6XYT1d72nUC8PO0/view?usp=sharing

The last discussion I find about running Clang Static Analyzer on the 
LibreOffice code base is the mail thread 
<https://lists.freedesktop.org/archives/libreoffice/2015-October/thread.html#70456> 
"Report from llvms static analysis tool" from five years ago.  Back then 
we had a list of 1385 issues, now we are at almost the same number of 
1348 issues.

 From a quick look at the list, I see instances of all of: clearly true 
positives, clearly false positives, and unclear findings.

* For example, "Use-after-move" 
2020-09-30-155842-506845-1/report-2cdbb5.html#EndPath is clearly a true 
positive (and one that e.g. Coverity Scan apparently doesn't find), see 
comment at 
<https://gerrit.libreoffice.org/c/core/+/78201/5#message-1b8efe85f0adf7e32bc1143393ac66c0a18ae22f> 
"tdf#126959 impress table border resize handle support".

* For example, "Use-after-free" 
2020-09-30-155842-506845-1/report-366f11.html#EndPath (presumably along 
with many, many other of the 827 Use-after-free) is clearly a false 
positive:  While VclPtr::disposeAndClear holds

   ::rtl::Reference<reference_type> aTmp(m_rInnerRef);

the call to

   m_rInnerRef.clear();

cannot decrement m_rInnerRef's ref count to zero, so the following

   if (aTmp.get()) {

will /not/ access freed memory.  We would need some mechanism to filter 
out such identified false positives, with whatever mechanism would be 
suitable: an annotation in the source code, a modification of the 
-analyzer-... command line options passed to clang, etc.  However, that 
filtering should be done in an auditable way, so that we can later 
discover that we are filtering false positives relating to a certain 
location in the code, and can learn the rationale why those were 
considered false positives.  (Something that can be a pain with the way 
we use Coverity Scan, see below.)

* For example, "Result of operation is garbage or undefined" 
2020-09-30-155842-506845-1/report-78b0b7.html#EndPath is an unclear 
finding:  SfxSplitWindow::MoveWindow does not check the 
SfxSplitWindow::GetWindowPos return value and unconditionally uses nL 
(which is uninitialized when GetWindowPos returned false).  It can 
either be the case that GetWindowPos cannot return false here, so it 
would be a false positive (and the code in SfxSplitWindow::MoveWindow 
should arguably assert that the return value of GetWindowPos is true), 
or it indeed is a true positive.  It is hard to find out whether this 
issue had already been found by Coverity Scan and dismissed as a false 
positive through its Web UI.