[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 21 Apr 2015 12:57:53 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: Static analysis results
On Tue, Apr 21, 2015 at 07:28:30PM +0300, Alexander Monakov wrote:
> New round of static analysis results. This time it's mostly opportunities for
> very minor cleanups (I'm showing only a few results that I think make sense).
> If there's a problem in balance of usefulness vs annoyance, please let me know.
>
> dynlink.c:343
> 'if (runtime)' is already established as true at line 337
This is a matter of a mechanical transform of all calls to error()
that was recently made to get the longjmp out of error(). It could
just be removed.
> sem_open.c:sem_open
> I didn't try to follow the code in detail, but it seems possible that 'goto
> fail' can be executed from e.g. line 133 after successful mmap, in which
> case the region is not unmapped
I agree. I think immediately after line 128 (if (!e) break;) we need
munmap(map,sizeof(sem_t)). That covers unmapping for both the failure
and retry cases.
> duplocale.c:17
> neither of the conditions cannot hold
Indeed, that was cruft from before the body of the function was added.
The whole memcpy seems wrong though; it undoes work done above. I need
to look into this.
> dynlink.c:1503
> the first two conditions cannot hold after check at line 1489 and exit at
> line 1501
These useless checks were added as the only content of commit
637dd2d383cc1f63bf02a732f03786857b22c7bd claiming it fixed a
regression, but I don't have any information on whether that
regression was observed (in which case it must have been a problem
somewhere else) or just theoretical and incorrect. I would guess the
latter.
> fcntl.c:42
> F_SETLKW is already taken care of at line 16
Yes.
> also, why does this file cast arg to 'void *' in several places?
It's an utter mess. Really we should be calling va_arg with the right
type for the command but this results in much larger code and has no
practical benefits at this time.
> regcomp.c:2848
> condition 'stack != NULL' cannot hold
Didn't you mean it's always true?
> dynlink.c:428
> on 64-bit arches, multiplication can overflow in 32-bit type before assignment
It can on 32-bit too. At present the validity of stuff from loaded ELF
files is not scrutinized. We're going to be running code from them
anyway. There's been some interest in making it safe against invalid
ELF files for the sake of ldd, but that would be a big project.
Rich
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
