clang-tidy problem on read non-blocking file descriptor

[dpetrosy]

Hi!
I found some suspicious behavior of clang-tidy(version 19.1.3). When I call read on the non-blocking file descriptor, it reports [clang-analyzer-unix.BlockInCriticalSection]

Here is some example code:

std::mutex mtx;

void foo()
{
    std::lock_guard<std::mutex> lock(mtx);

    const char *filename = "example.txt";
    int fd = open(filename, O_RDONLY | O_NONBLOCK);

    char buffer[200] = {};
    read(fd, buffer, 199);
    close(fd);
}

int main()
{
    std::thread th(foo);
    th.join();
    return 0;
}

[llvmbot]

@llvm/issue-subscribers-clang-tidy

Author: Davit Petrosyan (dpetrosy)

Hi! I found some suspicious behavior of clang-tidy(version 19.1.3). When I call read on the non-blocking file descriptor, it reports [clang-analyzer-unix.BlockInCriticalSection]

Here is some example code:

std::mutex mtx;

void foo()
{
    std::lock_guard<std::mutex> lock(mtx);

    const char *filename = "example.txt";
    int fd = open(filename, O_RDONLY | O_NONBLOCK);

    char buffer[200] = {};
    read(fd, buffer, 199);
    close(fd);
}

int main()
{
    std::thread th(foo);
    th.join();
    return 0;
}

[llvmbot]

@llvm/issue-subscribers-clang-static-analyzer

Author: Davit Petrosyan (dpetrosy)

Hi! I found some suspicious behavior of clang-tidy(version 19.1.3). When I call read on the non-blocking file descriptor, it reports [clang-analyzer-unix.BlockInCriticalSection]

Here is some example code:

std::mutex mtx;

void foo()
{
    std::lock_guard<std::mutex> lock(mtx);

    const char *filename = "example.txt";
    int fd = open(filename, O_RDONLY | O_NONBLOCK);

    char buffer[200] = {};
    read(fd, buffer, 199);
    close(fd);
}

int main()
{
    std::thread th(foo);
    th.join();
    return 0;
}

[steakhal]

Ah yes. Thanks for the report.
This one should be easy to fix.

Currently, the logic is really simple: do we call a set of APIs such as "read" while holding a mutex? If so, warn.
https://clang.llvm.org/doxygen/BlockInCriticalSectionChecker_8cpp_source.html

I don't think we should overcomplicate this, so a suppression should be enough to get rid of this FP.
I think we should have a pathsensitive bug report visitor looking for the origin of the file descriptor (aka. the open call) and check if it was created by using O_NONBLOCK. If that was the case, mark the report invalid.

In details:

• Create a new bug report visitor and add it to the bug report. Grab the memregion of the filedescriptor argument of the "read" call, and it to the visitor.
• The visitor should have a Satisfied flag, indicating if the visitor is done or not. Eg. it's done if found the origin of the file descriptor.
• In the Visit callback, it should check if the in the current node we have a value for the filedescriptor, but we no longer have in the predecessor node. That means that we assigned the value of the file descriptor in the current node. The ProgramPoint should be a StmtPoint, which should refer to a CallExpr statement. You can grab that and see if the identifier of the callee decl is "open" for instance, and the call has the expected number of arguments.
• Grab the Expr of the 2nd argument of the "open" call and get the sval of it from the State. Use the clang::ento::tryExpandAsInteger("O_NONBLOCK", PP) to see the concrete value of O_NONBLOCK (remember, this value may be different on different platforms). Now that we have both of these, we can getAsInteger() on the symbolic value and then we can test if the required bit is on. If it was on, call R->markInvalid().

[llvmbot]

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

1. Check that no other contributor has already been assigned to this issue. If you believe that no one is actually working on it despite an assignment, ping the person. After one week without a response, the assignee may be changed.
2. In the comments of this issue, request for it to be assigned to you, or just create a pull request after following the steps below. Mention this issue in the description of the pull request.
3. Fix the issue locally.
4. Run the test suite locally. Remember that the subdirectories under test/ create fine-grained testing targets, so you can e.g. use make check-clang-ast to only run Clang's AST tests.
5. Create a Git commit.
6. Run git clang-format HEAD~1 to format your changes.
7. Open a pull request to the upstream repository on GitHub. Detailed instructions can be found in GitHub's documentation. Mention this issue in the description of the pull request.

If you have any further questions about this issue, don't hesitate to ask via a comment in the thread below.

[llvmbot]

@llvm/issue-subscribers-good-first-issue

Author: Davit Petrosyan (dpetrosy)

Hi! I found some suspicious behavior of clang-tidy(version 19.1.3). When I call read on the non-blocking file descriptor, it reports [clang-analyzer-unix.BlockInCriticalSection]

Here is some example code:

std::mutex mtx;

void foo()
{
    std::lock_guard<std::mutex> lock(mtx);

    const char *filename = "example.txt";
    int fd = open(filename, O_RDONLY | O_NONBLOCK);

    char buffer[200] = {};
    read(fd, buffer, 199);
    close(fd);
}

int main()
{
    std::thread th(foo);
    th.join();
    return 0;
}

[KHariHaraChaitanya]

@dpetrosy I am interested to take this issue, I am beginner in LLVM.

[steakhal]

@dpetrosy I am interested to take this issue, I am beginner in LLVM.

Sounds good. I tried to leave the detailed steps in my previous comment to give you some aid.

You should look at other checkers that create custom visitors and look at the git blame when and how they were introduced to get a rough idea what it would take to make one like I described.
(Basically grep for Visit in the directory where the Checkers live and you will find them)

Likely even chatgpt can do the bulk of the work if you ask it nicely :D

And of course, I'm here to help to answer any questions.
Have fun!

[dpetrosy]

@KHariHaraChaitanya Hi!
Take it and do your best🙂

[sidsabh]

@KHariHaraChaitanya How's it going on this one? Any chance I can help out? Also looking for a first issue in LLVM.

[KHariHaraChaitanya]

Hey @sidsabh, I'm still actively working on it. I appreciate your offer and will definitely reach out if I hit any blockers.

[steakhal]

From my perspective, as a maintainer, I'm more interested in the outcomes.
That said, I'd encourage people working on issues, no matter if someone else claims already working on it.
I'll review any PRs, if there is one.

The first one having a PR would then drive the discussion, but anyone could chim in.
For instance, if you were involved with the case but someone else posted a PR before you, it's okay.
Come to the review and share your learnings, suggest test cases and challenge their proposal in a constructive way.
Anyone with a substantial contribution will be rewarded as being a co-author of the PR when merged.

It's a collaborative effort. And as such, it should be asynchronous and outcome driven.
This is why I'm generally against using the "assignee" field of issues or PRs.