Static analysis results

Static analysis results

[Alexander Monakov]

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

Re: Static analysis results

[Alexander Monakov]

> regcomp.c:2848
>   condition 'stack != NULL' cannot hold

I meant to say "must always hold".

Re: Static analysis results

[Rich Felker]

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

Re: Static analysis results

[Rich Felker]

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

Re: Static analysis results

[Szabolcs Nagy]

* 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