Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bogus error: unreachable: code is dead #80

Closed
yurivict opened this issue Jan 12, 2019 · 4 comments
Closed

Bogus error: unreachable: code is dead #80

yurivict opened this issue Jan 12, 2019 · 4 comments
Labels
C-bug Category: Bug L-c Language: C

Comments

@yurivict
Copy link

yurivict commented Jan 12, 2019

I tried ikos on one file from the FreeBSD source tree, hexdump.c:

#ifndef lint
static const char copyright[] =
"@(#) Copyright (c) 1989, 1993\n\
        The Regents of the University of California.  All rights reserved.\n";
#endif /* not lint */

#ifndef lint
#if 0
static char sccsid[] = "@(#)hexdump.c   8.1 (Berkeley) 6/6/93";
#endif
#endif /* not lint */
#include <sys/cdefs.h>
__FBSDID("$FreeBSD: stable/11/usr.bin/hexdump/hexdump.c 331722 2018-03-29 02:50:57Z eadler $");

#include <sys/types.h>
#include <locale.h>
#include <stdlib.h>
#include <stdio.h>
#include <string.h>
#include "hexdump.h"

FS *fshead;                             /* head of format strings */
int blocksize;                          /* data block size */
int exitval;                            /* final exit value */
int length = -1;                        /* max bytes to read */

int
main(int argc, char *argv[])
{
        FS *tfs;
        char *p;

        (void)setlocale(LC_ALL, "");

        if (!(p = strrchr(argv[0], 'o')) || strcmp(p, "od"))
                newsyntax(argc, &argv);
        else
                oldsyntax(argc, &argv);

        /* figure out the data block size */
        for (blocksize = 0, tfs = fshead; tfs; tfs = tfs->nextfs) {
                tfs->bcnt = size(tfs);
                if (blocksize < tfs->bcnt)
                        blocksize = tfs->bcnt;
        }
        /* rewrite the rules, do syntax checking */
        for (tfs = fshead; tfs; tfs = tfs->nextfs)
                rewrite(tfs);

        (void)next(argv);
        display();
        exit(exitval);
}

It printed this warning, among others:

hexdump.c: In function 'main':
hexdump.c:70:52: unreachable: code is dead
	for (blocksize = 0, tfs = fshead; tfs; tfs = tfs->nextfs) {
	                                                  ^
hexdump.c: In function 'main':
hexdump.c:71:8: unreachable: code is dead
		tfs->bcnt = size(tfs);
		     ^

This code is obviously reachable. This for loop might never execute, if fshead is left to be NULL, but ikos has no way of knowing this without seeing newsyntax or oldsyntax functions since fshead isn't static.

It also printed:

hexdump.c:64:12: warning: ignored possible side effect of function call, could not infer information about pointer '*argv'. Analysis might be unsound.
	if (!(p = strrchr(argv[0], 'o')) || strcmp(p, "od"))
	          ^
hexdump.c: In function 'main':
hexdump.c:64:20: warning: possible buffer overflow, pointer 'argv' with offset 0 bytes points to argv
	if (!(p = strrchr(argv[0], 'o')) || strcmp(p, "od"))
	                  ^
hexdump.c: In function 'main':
hexdump.c:64:38: warning: ignored possible side effect of function call, could not infer information about pointer 'p'. Analysis might be unsound.
	if (!(p = strrchr(argv[0], 'o')) || strcmp(p, "od"))
	                                    ^
hexdump.c: In function 'main':
hexdump.c:79:8: warning: ignored possible side effect of function call, could not infer information about pointer 'argv'. Analysis might be unsound.
	(void)next(argv);
	      ^

The type of argv is defined, and these statements seem to be valid.

ikos-2.1.52
FreeBSD 11.2 amd64

@yurivict yurivict changed the title Bogus errror: unreachable: code is dead Bogus error: unreachable: code is dead Jan 12, 2019
@yurivict
Copy link
Author

Instead, a valid warning would be don't have the source of newsyntax available.

@arthaud
Copy link
Member

arthaud commented Jan 13, 2019

Hi @yurivict,

You are right. Since ikos doesn't have the code of the functions newsyntax and oldsyntax, it assumes that they don't have side effects (i.e, they don't update global variables). In that case, fshead is null and the body of the loop is unreachable (according to ikos).

I agree that in this case we should emit 2 more warnings. I will make a patch soon, ikos should emit something like:

hexdump.c:36:16: warning: ignored possible side effect of function call to newsyntax. Analysis might be unsound.
                newsyntax(argc, &argv);
	        ^
hexdump.c:38:16: warning: ignored possible side effect of function call to oldsyntax. Analysis might be unsound.
                oldsyntax(argc, &argv);
	        ^

The other warnings are also related to this. IKOS doesn't know strrchr, strcmp and next. I can make a patch to teach him about strrchr and strcmp. We will still have a warning about next.

Even after fixing these, we will still get unreachable: code is dead for line 71, because we assume external functions do not update global variables. I think this is pretty reasonable and we don't want to change this. Assuming an external functions can update global variables would raise a lot of false positives. At best, we could add this as an analysis option. What do you think?

@arthaud arthaud added the C-bug Category: Bug label Jan 13, 2019
@yurivict
Copy link
Author

yurivict commented Jan 13, 2019

What do you think?

I agree. You should teach him all simple library functions.

@arthaud
Copy link
Member

arthaud commented Feb 8, 2019

Using 79942e6, I'm now getting:

test.c: In function 'main':
test.c:33:15: warning: ignored side effect of call to extern function 'setlocale'. Analysis might be unsound.
        (void)setlocale(LC_ALL, "");
              ^
test.c: In function 'main':
test.c:35:19: warning: ignored side effect of call to extern function 'strrchr'. Analysis might be unsound.
        if (!(p = strrchr(argv[0], 'o')) || strcmp(p, "od"))
                  ^
test.c: In function 'main':
test.c:35:27: warning: possible buffer overflow, pointer 'argv' with offset 0 bytes points to argv
        if (!(p = strrchr(argv[0], 'o')) || strcmp(p, "od"))
                          ^
test.c: In function 'main':
test.c:36:17: warning: ignored side effect of call to extern function 'newsyntax'. Analysis might be unsound.
                newsyntax(argc, &argv);
                ^
test.c: In function 'main':
test.c:38:17: warning: ignored side effect of call to extern function 'oldsyntax'. Analysis might be unsound.
                oldsyntax(argc, &argv);
                ^
test.c: In function 'main':
test.c:50:15: warning: ignored side effect of call to extern function 'next'. Analysis might be unsound.
        (void)next(argv);
              ^
test.c: In function 'main':
test.c:51:9: warning: ignored side effect of call to extern function 'display'. Analysis might be unsound.
        display();
        ^

It is now more clear that the implementation of newsyntax, oldsyntax, next and display are missing.

IKOS still doesn't know about libc functions setlocale and strrchr. That will come later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Bug L-c Language: C
Projects
None yet
Development

No branches or pull requests

2 participants