Skip to content

False positive in C/C++ dead code detection #19399

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.

Sign up for GitHub

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

Jump to bottom
Open
Faycal572 opened this issue Apr 28, 2025 · 3 comments
Open

False positive in C/C++ dead code detection #19399

Faycal572 opened this issue Apr 28, 2025 · 3 comments
Labels
awaiting-response The CodeQL team is awaiting further input or clarification from the original reporter of this issue. false-positive

Comments

@Faycal572
Copy link

Faycal572 commented Apr 28, 2025 •
edited
Loading

Description of the false positive

The query for unreachable code (BasicBlock where not bb1.isReachable()) incorrectly detects live code as dead code in C/C++.
Specifically, in the function Tcl_TranslateFileName, it incorrectly flags a for loop that is clearly reachable and executed in normal control flow.
tclPlatform is a global variable, so the condition if (tclPlatform == TCL) can be true at runtime, making the code reachable.

This leads to several false positives in real codebases.

Code samples or links to source code

Here is an example:

char *Tcl_TranslateFileName(Tcl_Interp *interp, CONST char *name, Tcl_DString *bufferPtr)
{
    Tcl_Obj *path = Tcl_NewStringObj(name, -1);
    Tcl_Obj *transPtr;

    Tcl_IncrRefCount(path);
    transPtr = Tcl_FSGetTranslatedPath(interp, path);
    if (transPtr == NULL)
    {
        Tcl_DecrRefCount(path);
        return NULL;
    }

    Tcl_DStringInit(bufferPtr);
    Tcl_DStringAppend(bufferPtr, Tcl_GetString(transPtr), -1);
    Tcl_DecrRefCount(path);
    Tcl_DecrRefCount(transPtr);

    if (tclPlatform == TCL)
    {
        char *p;
        for (p = Tcl_DStringValue(bufferPtr); *p != '\0'; p++)  // <-- this block is reported as unreachable, but it is reachable
        {
            if (*p == '/')
            {
                *p = '\\';
            }
        }
    }
    return Tcl_DStringValue(bufferPtr);
}

Query used to detect unreachable blocks:

import cpp

predicate isUserFile(File f) {
  not f.getFile().getAbsolutePath().matches("%/include/%") and 
  not (f.toString().matches("%file://:0:0:0:0%")) 
}

from BasicBlock bb1
where
  isUserFile(bb1.getEnclosingFunction().getFile()) and
  not bb1.isReachable()
select 
  bb1.getEnclosingFunction().getName(),
  bb1.getEnclosingFunction().getLocation(),
  bb1.getStart().getLocation().getStartLine(),
  bb1.getEnd().getLocation().getStartLine()

The block:

{
    char *p;
    for (p = Tcl_DStringValue(bufferPtr); *p != '\0'; p++)

is wrongly reported as unreachable (dead code), but it is active and necessary to correctly translate file paths.

This false positive happens in several similar real-world cases, and makes the dead code detection query unreliable.
@jketema
Copy link
Contributor

jketema commented Apr 28, 2025 •
edited
Loading

Hi @Faycal572,

Unfortunately I cannot reproduce the problem you're reporting. I've tried is to create a database as follows:

git clone https://github.com/tcltk/tcl
cd tcl/unix
./configure
codeql database create -l cpp -c make ../db

When I then run your query on that database, I do not see the problem you're reporting. Could you provide instructions on how to reproduce the issue?

@jketema jketema added the awaiting-response The CodeQL team is awaiting further input or clarification from the original reporter of this issue. label Apr 28, 2025
@Faycal572
Copy link
Author

Hi, thanks for your answer.

The code where I observe this false positive comes from an internal tool developed by my company, and unfortunately I cannot share the full project for confidentiality reasons.

However, the root cause seems clear:
In my code, tclPlatform is a global variable whose value is only determined at runtime, not at compile time.
Thus, the condition if (tclPlatform == TCL) can be true at runtime, and the for loop inside is reachable.
But currently, the dead code query (BasicBlock not reachable) incorrectly considers it unreachable.

I have seen this false positive in several similar cases.

Thanks again!

@jketema
Copy link
Contributor

jketema commented Apr 29, 2025

Thanks for your answer,

In my code, tclPlatform is a global variable whose value is only determined at runtime, not at compile time.
Thus, the condition if (tclPlatform == TCL) can be true at runtime, and the for loop inside is reachable.
But currently, the dead code query (BasicBlock not reachable) incorrectly considers it unreachable.

That's an incorrect diagnosis. First, isReachable does not look at the values of global variables. Second, if you look at the code from https://github.com/tcltk/tcl you'll see that is very similar https://github.com/tcltk/tcl/blob/4fa72e4d115f34198b849b5354a75ae458aeb981/generic/tclFileName.c#L1026, and also depends on a global variable https://github.com/tcltk/tcl/blob/4fa72e4d115f34198b849b5354a75ae458aeb981/generic/tclFileName.c#L23 Yet, the problem you report does not occur.

I suspect there's some deeper problem with the quality of your database, but given that you cannot share any reproducer, this is impossible for me to diagnose any further here. However, given our license and the fact that you cannot share any further details here will mean that your company is a paying customer, the way forward here would be to open a support ticket with GitHub support as explained here https://docs.github.com/en/support/contacting-github-support/creating-a-support-ticket. That will allow for a more private way of sharing data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
awaiting-response The CodeQL team is awaiting further input or clarification from the original reporter of this issue. false-positive
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@jketema @Faycal572