Description: We've used our static analyzer Svace to check the Libreoffice source code (version 6.0.7.1), both for benchmarking our analysis and for being helpful to the open source community. We have found the following suspicious code snippet (source locations are cited as in the current master): In sd/source/filter/eppt/epptso.cxx there is PPTWriter::ImplWriteParagraphs method on line 662. On lines 667-670, there is the following code: nDepth = pPara->nDepth; if ( nDepth > 4) nDepth = 4; This condition suggests that pPara->nDepth may be greater than 4, yet later in the code when calling mpStyleSheet->IsHardAttribute at lines 714, 718, 721, 724, pPara->nDepth is used as nLevel parameter. Then inside IsHardAttribute the nLevel variable is used as the index for the maParaLevel array of size 5 in line 414 of pptx-stylesheet.cxx: const PPTExParaLevel& rPara = mpParaSheet[ nInstance ]->maParaLevel[ nLevel ]; and then rPara is dereferenced inside the switch operator. It seems that nDepth should be used instead of pPara->nDepth as in the similar calls to IsHardAttribute at lines 730, 745 and others. We would like to know if such bug reports are useful for you and whether this is the right place to file them. Steps to Reproduce: Found the suspicious source code as a result of running static analysis over it. Actual Results: Inconsistencies in using nDepth and pPara->nDepth in the cited code. Expected Results: Consistently use nDepth instead. Reproducible: Didn't try User Profile Reset: No Additional Info:
Hi Caolán, Mike, another bug from a static analyzer. Would you mind checking the nice report written by Arina Dudina ?
I'd say that this is definitely helpful. And we have precedents of similar reports here in Bugzilla, so I suppose this to be OK. This specific report is about harmless yet confusing checks. Actually, the value of paragraph depth is only assigned non-negative values less than 5 (in ParagraphObj::ImplGetParagraphValues); still, we obviously need to change the code. (I see that Caolán already took it)
Yes, bugzilla is a good place to report them. If the output from Svace is long, you can attach it as a file instead. Thanks
Yeah, I agree with mike, I can only see one place where the nDepth member is assigned to in ParagraphObj::ImplGetParagraphValues, where the value is limited to legal limits, so the tooling is correct that the checks are inconsistent, but seems we should simply remove them. https://gerrit.libreoffice.org/#/c/64223/ Yeah, I'd be interesting in seeing more Svace output
Thank you all for your feedback! It's good to know we can help to improve your project. I think we will look for some more real defects by inspecting Svace output and file them as separate bugs if that suits you well.
Thanks! I'd say don't filter out issues like this. While hey don't result in a code doing bad things, it's still bad code that needs changing.
A single bug with all the findings attached like bug 120703 rather that e.g. 250 individual ones is probably better.
Caolán McNamara committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/+/20b8522e6c40625e662a2225521881f5c029fd90%5E%21 tdf#121795 bogus checks on depth limit It will be available in 6.3.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.