* Static analysis results
@ 2015-04-21 16:28 Alexander Monakov
2015-04-21 16:39 ` Alexander Monakov
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Alexander Monakov @ 2015-04-21 16:28 UTC (permalink / raw)
To: musl
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
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
duplocale.c:17
neither of the conditions cannot hold
dynlink.c:1503
the first two conditions cannot hold after check at line 1489 and exit at
line 1501
fcntl.c:42
F_SETLKW is already taken care of at line 16
also, why does this file cast arg to 'void *' in several places?
regcomp.c:2848
condition 'stack != NULL' cannot hold
dynlink.c:428
on 64-bit arches, multiplication can overflow in 32-bit type before assignment
Alexander
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Static analysis results
2015-04-21 16:28 Static analysis results Alexander Monakov
@ 2015-04-21 16:39 ` Alexander Monakov
2015-04-21 16:57 ` Rich Felker
2015-04-23 16:41 ` Rich Felker
2 siblings, 0 replies; 5+ messages in thread
From: Alexander Monakov @ 2015-04-21 16:39 UTC (permalink / raw)
To: musl
> regcomp.c:2848
> condition 'stack != NULL' cannot hold
I meant to say "must always hold".
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Static analysis results
2015-04-21 16:28 Static analysis results Alexander Monakov
2015-04-21 16:39 ` Alexander Monakov
@ 2015-04-21 16:57 ` Rich Felker
2015-04-23 16:41 ` Rich Felker
2 siblings, 0 replies; 5+ messages in thread
From: Rich Felker @ 2015-04-21 16:57 UTC (permalink / raw)
To: musl
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Static analysis results
2015-04-21 16:28 Static analysis results Alexander Monakov
2015-04-21 16:39 ` Alexander Monakov
2015-04-21 16:57 ` Rich Felker
@ 2015-04-23 16:41 ` Rich Felker
2015-04-23 18:08 ` Szabolcs Nagy
2 siblings, 1 reply; 5+ messages in thread
From: Rich Felker @ 2015-04-23 16:41 UTC (permalink / raw)
To: musl
Aside from regcomp which I want to ask nsz on before committing
anything, I believe these are all fixed now by the following commits:
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
c5ab5bd3be15eb9d49222df132a51ae8e8f78cbc remove always-true conditional in dynamic linker TLSDESC processing
> 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
086793ad99dc625fd1c47f96fc31ea8aa316b438 fix mmap leak in sem_open failure path for link call
> duplocale.c:17
> neither of the conditions cannot hold
873e0ec7fc4d466cfcdec16a7648cc18609ba702 fix duplocale clobbering of new locale struct with memcpy of old
> dynlink.c:1503
> the first two conditions cannot hold after check at line 1489 and exit at
> line 1501
97b72d22ad53e8f1306bf8e943571b698058f49d remove redundant code in do_dlsym function
> fcntl.c:42
> F_SETLKW is already taken care of at line 16
> also, why does this file cast arg to 'void *' in several places?
ea1b6bb6123d2177508ddca438669ec96cfa0021 remove dead case for F_SETLKW in fcntl
> regcomp.c:2848
> condition 'stack != NULL' cannot hold
[open but not a bug]
> dynlink.c:428
> on 64-bit arches, multiplication can overflow in 32-bit type before assignment
[not considered a bug at this time; see other email]
Rich
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Static analysis results
2015-04-23 16:41 ` Rich Felker
@ 2015-04-23 18:08 ` Szabolcs Nagy
0 siblings, 0 replies; 5+ messages in thread
From: Szabolcs Nagy @ 2015-04-23 18:08 UTC (permalink / raw)
To: musl
* Rich Felker <dalias@libc.org> [2015-04-23 12:41:51 -0400]:
> Aside from regcomp which I want to ask nsz on before committing
> anything, I believe these are all fixed now by the following commits:
>
> > regcomp.c:2848
> > condition 'stack != NULL' cannot hold
>
> [open but not a bug]
>
stack is always non-zero there
all the NULL checks in the error path are unnecessary
as far as i can see
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-04-23 18:08 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-21 16:28 Static analysis results Alexander Monakov
2015-04-21 16:39 ` Alexander Monakov
2015-04-21 16:57 ` Rich Felker
2015-04-23 16:41 ` Rich Felker
2015-04-23 18:08 ` Szabolcs Nagy
Code repositories for project(s) associated with this public inbox
https://git.vuxu.org/mirror/musl/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).