[Low priority] Some minor complaints from cppcheck about emacs 26.2

[Low priority] Some minor complaints from cppcheck about emacs 26.2

[Adam Richter]

[-- Attachment #1: Type: text/plain, Size: 2702 bytes --]

Hi, everyone.

I just tried running the version of cppcheck from the github
repository (which fixes at least one false positive report that I have
been seeing from cppcheck 1.86) on emacs 26.2 sources, and cppcheck
appears to have a found a few bugs that probably don't cause external
misbehavior, but I'm not sure are completely safe.  So, I am sending
this message with the cppcheck output attached, in the hopes that by
saving emacs developers the time of building the latest cppcheck and
doing a superficial check of the warnings, one or more people on the
list might find it worth taking a glance that the messages that I
could not rule out as completely spurious, and also in the hopes of
saving time for anyone else who runs cppcheck on emacs and sees the
same warnings.

Here is a list of files listed in the attached cppcheck and my notes about them.

lib-src/ebrowse.c  <-- Not a bug.  strdup is supposed to "leak" a malloc.

lib-src/update-game-score.c <-- I submitted a patch attached to a
message to this mailing list a few minutes ago, which I think fixes
the legitimate complaints.  The last complaint ("realloc mistake")
does not appear to be a bug.

lib/stdint.h  <-- This is a real typographical bug and I don't know
what the author intended.

lib/nstrftime.c <-- Possibly a real bug, but only if multibyte is
disabled.  A local variable "width", set to -1, is used by a macro
that I think may expect a positive value, but I am not sure of this
fix.

nt/preprep.c <-- These are complaints about
copy_executable_and_move_sections() where the variable
import_delta_rva might not be set if import_section == NULL and
rva_to_section() also returns NULL.  I am not sure if this really can
happen and if the resultant undefined value is consequential in that
case.

src/regex.c <--I think the tiny memory leak for compile_stack.stack is
real.  Everything else is probably not a legitimate complaint,
although I'm not 100% sure about that.  I think the "Uninitialized
variable" complaints about len and buf_charlen are all wrong, possibly
due to cppcheck trying all combinations of #ifdef's and perhaps
finding one that does not define the macros that set these variables.

src/regex.h <-- This complaint seems completely spurious (use of
#ifdef in the middle of a function declaration).

lib-src/etags.c <-- I believe that this complaint about a possible
null pointer dereference is spurious because filename_is_absolute()
would need to return true in one place and false in another with the
same data in order for that to happen.

Anyhow, if anyone has any questions, please feel free to ask me or
discuss on this list, as you think appropriate.  I hope this
information is helpful.

Adam

[-- Attachment #2: emacs-26.2.cppcheck.log --]
[-- Type: application/octet-stream, Size: 2497 bytes --]

[lib-src/ebrowse.c:532]: (error) Allocation with xmalloc, strcpy doesn't release it.
[lib-src/update-game-score.c:329]: (error) Memory leak: filedata
[lib-src/update-game-score.c:334]: (error) Memory leak: filedata
[lib-src/update-game-score.c:339]: (error) Memory leak: filedata
[lib-src/update-game-score.c:410]: (error) Common realloc mistake: 'newscores' nulled but not freed upon failure
[lib/stdint.h:537]: (error) Syntax error in #if
[lib/nstrftime.c:660]: (error) Invalid memset() argument nr 3. The value is -2 but the valid values are '0:'.
[lib/nstrftime.c:660]: (error) Invalid wmemset() argument nr 3. The value is -2 but the valid values are '0:'.
[nt/preprep.c:570]: (error) Uninitialized variable: import_delta_rva
[nt/preprep.c:597]: (error) Uninitialized variable: import_delta_rva
[nt/preprep.c:663]: (error) Uninitialized variable: end_block
[nt/preprep.c:669]: (error) Uninitialized variable: end_block
[src/regex.c:2563]: (error) Memory leak: compile_stack.stack
[src/regex.c:5085]: (error) Pointer addition with NULL pointer.
[src/regex.c:2563]: (error) Uninitialized variable: len
[src/regex.c:2681]: (error) Uninitialized variable: len
[src/regex.c:2688]: (error) Uninitialized variable: len
[src/regex.c:2897]: (error) Uninitialized variable: len
[src/regex.c:2904]: (error) Uninitialized variable: len
[src/regex.c:2920]: (error) Uninitialized variable: len
[src/regex.c:2923]: (error) Uninitialized variable: len
[src/regex.c:3059]: (error) Uninitialized variable: len
[src/regex.c:3076]: (error) Uninitialized variable: len
[src/regex.c:3079]: (error) Uninitialized variable: len
[src/regex.c:3270]: (error) Uninitialized variable: len
[src/regex.c:3273]: (error) Uninitialized variable: len
[src/regex.c:3288]: (error) Uninitialized variable: len
[src/regex.c:3488]: (error) Uninitialized variable: len
[src/regex.c:4327]: (error) Uninitialized variable: buf_charlen
[src/regex.c:4328]: (error) Uninitialized variable: buf_charlen
[src/regex.c:4357]: (error) Uninitialized variable: buf_charlen
[src/regex.c:4358]: (error) Uninitialized variable: buf_charlen
[src/regex.c:5445]: (error) Uninitialized variable: buf_charlen
[src/regex.c:5498]: (error) Uninitialized variable: len
[src/regex.h:485]: (error) failed to expand 're_compile_pattern', it is invalid to use a preprocessor directive as macro parameter
[lib-src/etags.c:1708] -> [lib-src/etags.c:7162] -> [lib-src/etags.c:7027]: (error) Null pointer dereference: s1

real	3m46.640s
user	3m45.640s
sys	0m0.552s

Re: [Low priority] Some minor complaints from cppcheck about emacs 26.2

[Paul Eggert]

On 4/30/19 2:38 PM, Adam Richter wrote:
> lib-src/update-game-score.c <-- I submitted a patch attached to a
> message to this mailing list a few minutes ago, which I think fixes
> the legitimate complaints.  The last complaint ("realloc mistake")
> does not appear to be a bug.

Since update-game-score is about to exit when the 'free' calls you're
proposing would be executed, I'm not sure the changes are that helpful:
they complicate the source code and make the executable a tiny bit
bigger and slower and don't fix any real leaks. Perhaps if we do full
LeakSanitizer checking we can put in the change then, if only to pacify
the LeakSanitizer. (It's not clear to me that LeakSanitizer is a win
either, as it also reports quite a few false alarms.)


> lib/stdint.h  <-- This is a real typographical bug and I don't know
> what the author intended.

There is no lib/stdint.h in the Emacs distribution. On some platforms
lib/stdint.h is built from lib/stdint.in.h. If that build process didn't
work for you, it sounds like you need need to look into what went wrong
in that build process.


>
> lib/nstrftime.c <-- Possibly a real bug, but only if multibyte is
> disabled.  A local variable "width", set to -1, is used by a macro
> that I think may expect a positive value, but I am not sure of this
> fix.

I don't see any bug there; could you explain? If 'width' is negative
then the macro's _w is zero, which means _n < _w cannot be true (both _n
and _w are unsigned), and so the memset calls cannot be invoked.

> nt/preprep.c

Eli can look into that one if he has the time.


> src/regex.c <--I think the tiny memory leak for compile_stack.stack is
> real.

Is this when MATCH_MAY_ALLOCATE is not defined? If so, let's not worry
about it. It's always defined, and the !MATCH_MAY_ALLOCATE code has been
removed in Emacs 27.

Re: [Low priority] Some minor complaints from cppcheck about emacs 26.2

[Adam Richter]

[Sorry for the resend, Paul.  I forgot to cc emacs-devel.]

Thanks for the prompt detailed analysis, Paul.

I'm fine with whatever you want to do with update-game-score.c.  I
agree that exiting a few microseconds faster is more important than
appeasing a code checker in this case (not sarcasm).  If I had my
druthers, I'd probably put in a commented out free() so anyone else
running a similar check or trying to move the code to a library will
be less likely ask the same question, but whatever.  I'll look into
whether there is some comment that cppcheck will accept to indicate
that the leak is deliberate.

I see now that the stdint.h problems were substitutions from
stdint.in.h for HAVE_SIGNED_SIG_ATOMIC_T and two others (there were
apparently three such cases).  Trying another Linux environment just
now, my initial attempt to configure on an Ubuntu 19.04 system broke
and I have to run now and be away for the rest of the day, but I
suspect that you are right that it is my problem somewhere and hope to
send you an update confirming this in the next day or two.

Thanks for your explanation about nstrftime.c, which I read while
looking at the code.  I think you're right and am a bit surprised that
cppcheck did enough analysis to complain but not enough to rule it
out, given that cpp must have been using an ifdef combination that
would make the C preprocessor expand all of those macros.  I hope to
take a look at cppcheck side in the next day or two and will respond
further if I have some news.

I suspect that you're right in everything except for the
MATCH_MAY_ALLOCATE in regex.c.  If I run cppcheck
-DMATCH_MAY_ALLOCATE, it still generates the warning about the
compile_stack.stack.  I don't know that there is any urgency to fixing
it though, especially if you want to await a forthcoming code
simplification.

Adam

On Tue, Apr 30, 2019 at 3:23 PM Paul Eggert <eggert@cs.ucla.edu> wrote:
>
> On 4/30/19 2:38 PM, Adam Richter wrote:
> > lib-src/update-game-score.c <-- I submitted a patch attached to a
> > message to this mailing list a few minutes ago, which I think fixes
> > the legitimate complaints.  The last complaint ("realloc mistake")
> > does not appear to be a bug.
>
> Since update-game-score is about to exit when the 'free' calls you're
> proposing would be executed, I'm not sure the changes are that helpful:
> they complicate the source code and make the executable a tiny bit
> bigger and slower and don't fix any real leaks. Perhaps if we do full
> LeakSanitizer checking we can put in the change then, if only to pacify
> the LeakSanitizer. (It's not clear to me that LeakSanitizer is a win
> either, as it also reports quite a few false alarms.)
>
>
> > lib/stdint.h  <-- This is a real typographical bug and I don't know
> > what the author intended.
>
> There is no lib/stdint.h in the Emacs distribution. On some platforms
> lib/stdint.h is built from lib/stdint.in.h. If that build process didn't
> work for you, it sounds like you need need to look into what went wrong
> in that build process.
>
>
> >
> > lib/nstrftime.c <-- Possibly a real bug, but only if multibyte is
> > disabled.  A local variable "width", set to -1, is used by a macro
> > that I think may expect a positive value, but I am not sure of this
> > fix.
>
> I don't see any bug there; could you explain? If 'width' is negative
> then the macro's _w is zero, which means _n < _w cannot be true (both _n
> and _w are unsigned), and so the memset calls cannot be invoked.
>
> > nt/preprep.c
>
> Eli can look into that one if he has the time.
>
>
> > src/regex.c <--I think the tiny memory leak for compile_stack.stack is
> > real.
>
> Is this when MATCH_MAY_ALLOCATE is not defined? If so, let's not worry
> about it. It's always defined, and the !MATCH_MAY_ALLOCATE code has been
> removed in Emacs 27.
>

Re: [Low priority] Some minor complaints from cppcheck about emacs 26.2

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Tue, 30 Apr 2019 15:23:18 -0700
> Cc: emacs-devel@gnu.org
> 
> > nt/preprep.c
> 
> Eli can look into that one if he has the time.

It's for MSVC, which we no longer support.  We should at some point
delete that file, certainly not continue maintaining it.

Thanks.