Fix clang warnings #351
Fix clang warnings #351
Conversation
| @@ -5311,7 +5311,7 @@ int LVBaseFont::DrawTextString( LVDrawBuf * buf, int x, int y, | |||
| } | |||
|
|
|||
| #if (USE_BITMAP_FONTS==1) | |||
| bool LBitmapFont::getGlyphInfo( lUInt32 code, LVFont::glyph_info_t * glyph, lChar16 def_char, bool is_fallback=false ) | |||
| bool LBitmapFont::getGlyphInfo( lUInt32 code, LVFont::glyph_info_t * glyph, lChar16 def_char, bool is_fallback ) | |||
There was a problem hiding this comment.
Heh, good catch (I think gcc and clang both probably output this during compilation too though? :-))
It says this in the header after all:
virtual bool getGlyphInfo( lUInt32 code, LVFont::glyph_info_t * glyph, lChar16 def_char=0, bool is_fallback=false );
There was a problem hiding this comment.
This is dead code as USE_BITMAP_FONTS is undefined.
crengine/src/lvrend.cpp
Outdated
| @@ -1915,6 +1905,7 @@ class CCRTable { | |||
| line_flags |= RN_LINE_IS_RTL; | |||
| context.AddLine(last_y, table_y0 + table_h, line_flags); | |||
| last_y = table_y0 + table_h; | |||
| (void)last_y; // not read after here | |||
There was a problem hiding this comment.
Why not comment the assignation on the previous line?
Is it saved across iterations in a loop or something?
There was a problem hiding this comment.
No, it's really not used.
But as I took care updating it all along, it broke my heart to have to not update at the end (and commenting it out is a bit more equivoque, was it wrong, or not to be done, or just not needed).
There was a problem hiding this comment.
You should be able to (void) the assign directly, then?
There was a problem hiding this comment.
You mean (void)last_y = table_y0 + table_h; would just work the same ?
There was a problem hiding this comment.
I know it's a standard idiom for return values from functions you want to discard to shut SA up, so it'd stand to reason that that'd work here, too ;).
There was a problem hiding this comment.
Nope, not accepted on a single line:
lvrend.cpp:1907:39: error: invalid use of 'void'
(void)last_y = table_y0 + table_h; // not read after here, (void) silences clang warning
crengine/src/lvtinydom.cpp
Outdated
| @@ -770,6 +770,7 @@ bool CacheFile::writeIndex() | |||
| int sz = sizeof(CacheFileItem) * (count * 2 + 100); | |||
| allocBlock(CBT_INDEX, 0, sz); | |||
| indexItem = findBlock(CBT_INDEX, 0); | |||
| (void)indexItem; // avoid clang warning: value stored is never read | |||
There was a problem hiding this comment.
Also not used. But this code, I don't understand it at all - so not taking the risk to change the author initial wording.
crengine/src/lvtextfm.cpp
Outdated
| // bit dubious to avoid clang-tidy warning "call to 'malloc' has an allocation | ||
| // size of 0 bytes" without having to add checks for NULL pointer (looks like | ||
| // we might never be called with some empty text). | ||
| int alloc_len = len ? len : 1; |
There was a problem hiding this comment.
size_t ;p
(Or, at the very least, lUint32, like len itself ;).)
(I'd also find it more readable as alloc_len = len > 0 ? len : 1)
There was a problem hiding this comment.
OK :)
(That means you're not finding that too ugly I'm allocating a 1byte chunk , just to have something to free() and avoid checking conditions ?)
There was a problem hiding this comment.
Oh, well, you can also just set the ptr to NULL if len == 0. free(NULL) is valid ;).
There was a problem hiding this comment.
That's assuming pline->t.text is not used in anything flagged as only accepting nonnull pointers... ;).
There was a problem hiding this comment.
That's assuming pline->t.text is not used
... when NULL / len=0.
That's what I want to avoid to have to check everywhere it might be used/iterated:)
crengine/src/lvmemman.cpp
Outdated
| @@ -31,7 +31,7 @@ | |||
| static char file_to_remove_on_crash[2048] = ""; | |||
|
|
|||
| void crSetFileToRemoveOnFatalError(const char * filename) { | |||
| strcpy(file_to_remove_on_crash, filename == NULL ? "" : filename); | |||
| strncpy(file_to_remove_on_crash, filename == NULL ? "" : filename, 2048); | |||
There was a problem hiding this comment.
Easily used incorrectly; doesn't always \0-terminate or check for invalid pointers [MS-banned] (CWE-120).
There was a problem hiding this comment.
Bah, one is CWE-119, now that one is CWE-120... I might just go with // NOLINT, unless @NiLuJe rewrites this correctly (I saw that you fixed that kind of stuff, and I'm a bit lost with google finding the right way to do that... and I don't even really care)
There was a problem hiding this comment.
strncpy(file_to_remove_on_crash, filename == NULL ? "" : filename, sizeof(file_to_remove_on_crash - 1));
file_to_remove_on_crash[sizeof(file_to_remove_on_crash) - 1] = '\0';(This is still liable to trip an SA up, but it's safe. It'll truncate on "overflow", but it'll always be NUL-padded, NUL-terminated and never actually write outside of buffer boundaries).
There was a problem hiding this comment.
(c.f., all my strncpy calls are annotated with an ignore for Coverity ;p).
And KFMon uses a set of custom str5* functions: https://github.com/NiLuJe/kfmon/blob/master/str5/str5cpy.c
poire-z
force-pushed
the
fix_clang_warnings
branch
20 times, most recently
from
f7a2041
to
98aec19
Compare
|
Some links regarding: https://stackoverflow.com/questions/54163566/is-it-possible-to-get-clang-static-analyzer-to-understand-reference-counting Might mask some other/future real problems :/ |
poire-z
force-pushed
the
fix_clang_warnings
branch
4 times, most recently
from
b84aa7b
to
885e721
Compare
poire-z
force-pushed
the
fix_clang_warnings
branch
from
885e721
to
efaa48a
Compare
|
Phew, finally done with this chore... I left in a few, all also reference-counting-wrapper related, but they were in some code we don't use or I don't usually touch (so, I didn't feel confortable workaround them):
Except for the "use of memory after it is freed", all the other were valid warnings. Some links that didn't help much: |
|
The remaining ones, for reference: |
|
Please have a quick review, just to notice if there's something really ugly that jumps at you, thanks :) |
| @@ -2985,13 +2985,13 @@ bool LVXMLParser::Parse() | |||
| } | |||
| if ( ch=='-' && PeekCharFromBuffer(1)=='-' | |||
| && PeekCharFromBuffer(2)=='>' ) | |||
| ch = PeekNextCharFromBuffer(2); | |||
| PeekNextCharFromBuffer(2); | |||
There was a problem hiding this comment.
I haven't checked the code, but given the function name, I assume a peek simply returns the char without actually moving the stream position?
As such, if the return value isn't actually used, it would become a NOOP, so, err, what's it supposed to do?
There was a problem hiding this comment.
PeekCharFromBuffer does not advance the position, but PeekNextCharFromBuffer does ("skip current char (was already peeked), peek next").
crengine/src/rtfimp.cpp
Outdated
| @@ -526,7 +526,7 @@ bool LVRtfParser::Parse() | |||
| // control | |||
| bool asteriskFlag = false; | |||
| if ( ch2=='*' ) { | |||
| ch = *(++p); | |||
| // ch = *(++p); | |||
There was a problem hiding this comment.
Does avoiding the pointer increment not break the logic?
There was a problem hiding this comment.
Probably :) Will fix it.
| @@ -526,7 +526,7 @@ bool LVRtfParser::Parse() | |||
| // control | |||
| bool asteriskFlag = false; | |||
| if ( ch2=='*' ) { | |||
| ch = *(++p); | |||
| // ch = *(++p); | |||
| ch2 = *(++p); | |||
There was a problem hiding this comment.
i.e., at the very least, that's no longer ch2 but ch, first of its name ^^.
There was a problem hiding this comment.
Grrrr, ++pointers++ messing with my head...
Please confirm that the real fix is just:
- ch = *(++p);
+ p++; // no matter, p++ or ++p are the same here
ch2 = *(++p);There was a problem hiding this comment.
Well, let's get my head and this (void)ch :)
There was a problem hiding this comment.
Yup!
(Just posting to make the 👍 obvious ;)).
- Redefinition of default argument - Call to function 'strcpy' is insecure - Call to 'malloc' has an allocation size of 0 bytes - Call to virtual function during construction/destruction
clang-tidy does not really handle well our reference counting classes instances. Add //NOLINT to the places that triggers these warnings.
clang-tidy does not really handle well our reference counting classes instances like LVRef. The code was working fine and does not need any fix, but to silence these warnings, instead of fetching the css_style_rec_t or LVFont, we keep using the LVRef (that wrap these) we get from a node. This should be equivalent to the previous code (with may be an added indirection to resolve more often).
poire-z
force-pushed
the
fix_clang_warnings
branch
from
efaa48a
to
8c5e396
Compare
|
@poire-z you said such checks are clearly unnecessary :) |
|
If clang-tidy does not complain, I consider they are unnecessary :) |
| if ( buf->empty() ) | ||
| buf->reserve(1); // ensure buf->_array is no more NULL for _stream->Read(dstbuf->get() above |
There was a problem hiding this comment.
gcc-10 on arm (not gcc10 on x64, nor gcc8 on arm) complains about this one:
In file included from crengine/src/../include/../include/lvstream.h:34,
from crengine/src/../include/../include/lvxml.h:21,
from crengine/src/../include/../include/lvtinydom.h:33,
from crengine/src/../include/pdbfmt.h:7,
from crengine/src/pdbfmt.cpp:1:
In member function 'void LVArray<T>::reserve(int) [with T = unsigned char]',
inlined from 'void LVArray<T>::reserve(int) [with T = unsigned char]' at crengine/src/../include/../include/lvarray.h:88:10,
inlined from 'bool PDBFile::readRecord(int, LVArray<unsigned char>*)' at crengine/src/pdbfmt.cpp:601:25,
inlined from 'bool PDBFile::readBlock(int)' at crengine/src/pdbfmt.cpp:619:30,
inlined from 'bool PDBFile::readBlock(int)' at crengine/src/pdbfmt.cpp:614:10,
inlined from 'bool PDBFile::seek(lvpos_t)' at crengine/src/pdbfmt.cpp:642:29,
inlined from 'virtual lverror_t PDBFile::Read(void*, lvsize_t, lvsize_t*)' at crengine/src/pdbfmt.cpp:1090:24:
crengine/src/../include/../include/lvarray.h:95:27: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
95 | new_array[i] = _array[i];
| ~~~~~~~~~~~~~^~~~~~~~~
crengine/src/../include/../include/lvarray.h: In member function 'virtual lverror_t PDBFile::Read(void*, lvsize_t, lvsize_t*)':
crengine/src/../include/../include/lvarray.h:92:28: note: at offset 0 to an object with size 0 allocated by 'operator new []' here
92 | T* new_array = new T[ size ];
| ^~~~~~~~~~~~~
May be my change with reserve(1) was a bit loosy to fix another clang warning, but if I use:
buf->reserve(2); // or 3 or 4 or 5 or 6 : the same warnings as above
while:
buf->reserve(7); // or 8 or 10 : no more warnings !
Any idea what's happening ?!
crengine/crengine/include/lvarray.h
Lines 88 to 101 in 69b3f55
There was a problem hiding this comment.
stringop-overflow is not perfect, and prone to false positives. If it doesn't make sense, I'd ignore it ;).
There was a problem hiding this comment.
Any specific way (comments, etc...) to have gcc ignore it ?
Or should I just go with this: ? (pdb being the least of my interests)
buf->reserve(8); // needs at least 7 (?!) to avoid warnings from -Wstringop-overflo
There was a problem hiding this comment.
The whole thing doesn't make sense, if the reserve is for the Read in readRecordNoUnpack, it's completely redundant, because it already calls addSpace, which calls... reserve ;).
TL;DR: Remove the whole initial workaround?
if ( buf->empty() )
buf->reserve(1); // ensure buf->_array is no more NULL for _stream->Read(dstbuf->get() above
There was a problem hiding this comment.
Remove the whole initial workaround?
I don't remember what was the clang-tidy warning this ridiculous workaround was solving - I'll see on my next PR :)
Trying to fix some clang-tidy warnings. See #349 (comment).
Let's see how I'm doing so far.
Still 49 "Use of memory after it is freed" to look at.
There are some dubious and some quick&dirty stuff...
This change is