search

microsoft / vscode-cpptools

Official repository for the Microsoft C/C++ extension for VS Code.
Other
5.45k stars 1.53k forks source link

some clang-tidy warnings are ignored by the Problem Solver; they do not show up in PROBLEMS or the Editor #11757

Closed Burt-Silverman closed 7 months ago

Burt-Silverman commented 9 months ago

Environment

Bug Summary and Steps to Reproduce

Bug Summary: I use clang-tidy on a file with function strcat. Warning messages show up fine in the C/C++ OUTPUT window if I C_Cpp.loggingLevel": "Information". But there is no highlighting in the editor. I am using code from Nordic Semiconductor nRF Connect SDK. FYI: It seems that warnings that are not followed by a note are displayed fine. Warnings that are followed by a Note in the OUTPUT have the problem. See OUTPUT further down.

Steps to reproduce:

  1. Go to editor for file /ncs/v2.5.0/nrf/applications/asset_tracker_v2/src/events/app_module_event.c

  2. Right Click on editor window and click on Run Code Analysis for Active File

  3. Scroll down and find the C/C++ OUTPUT shown below in Configuration and Logs:

  4. See error: Although the warning is underlined in the OUTPUT and you can Ctrl+Click to the correct spot in the editor window, there is no automatic highlightling

Expected behavior: "strcat" should be underlined in the editor window and the problem should be listed in the PROBLEMS window.

Configuration and Logs

Settings.json:
{
    "files.associations": {
        "kernel.h": "c",
        "date_time.h": "c",
        "stdio.h": "c",
        "stddef.h": "c",
        "kernel_includes.h": "c",
        "atomic.h": "c",
        "app_module_event.h": "c",
        "common_module_event.h": "c"
    },
    "C_Cpp.codeAnalysis.clangTidy.args": [
        "--extra-arg=--target=arm-zephyr-eabi-gcc",
        "--system-headers"
      ],
      "C_Cpp.codeAnalysis.clangTidy.checks.enabled": ["*"],
      "C_Cpp.codeAnalysis.clangTidy.checks.disabled":[
        "bugprone-macro-parentheses", "bugprone-branch-clone",
        "modernize-macro-to-enum",
        "llvm-header-guard","llvmlibc-restrict-system-libc-headers",
        "altera-unroll-loops",
        "hicpp-no-assembler",
        "readability-identifier-length",
        "readability-avoid-const-params-in-decls",
        "bugprone-reserved-identifier",
        "cert-dcl37-c","cert-dcl51-cpp",
        "altera-struct-pack-align",
        "cppcoreguidelines-avoid-non-const-global-variables",
        "readability-function-cognitive-complexity",
        "altera-id-dependent-backward-branch"
        ],
    "C_Cpp.loggingLevel": "Information",
    "C_Cpp.default.cStandard": "c11"
}

c_cpp_properties.json:
{
    "configurations": [
        {
            "name": "Win32",
            "includePath": [
                "${workspaceFolder}/**"
            ],
            "defines": [
                "_DEBUG",
                "UNICODE",
                "_UNICODE"
            ],
            "compilerPath": "C:/ncs/toolchains/c57af46cb7/opt/zephyr-sdk/arm-zephyr-eabi/bin/arm-zephyr-eabi-gcc.exe",
            "cStandard": "c17",
            "cppStandard": "c++14",
            "intelliSenseMode": "windows-clang-arm",
            "configurationProvider": "nordic-semiconductor.nrf-connect"
        }
    ],
    "version": 4
}

Log Diagnostics:
...
C:\ncs2\v2.5.0\nrf\applications\asset_tracker_v2\src\events\app_module_event.c:87:4: warning: Call to function 'strcat' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcat'. CWE-119 [clang-analyzer-security.insecureAPI.strcpy]
   87 |                         strcat(data_types, type2str(event->data_list[i]));
      |                         ^
C:/ncs/toolchains/c57af46cb7/opt/zephyr-sdk/arm-zephyr-eabi/arm-zephyr-eabi/sys-include\ssp/string.h:110:26: note: expanded from macro 'strcat'
  110 | #define strcat(dst, src) __ssp_bos_check2(strcat, dst, src)
      |                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
C:/ncs/toolchains/c57af46cb7/opt/zephyr-sdk/arm-zephyr-eabi/arm-zephyr-eabi/sys-include\ssp/string.h:58:5: note: expanded from macro '__ssp_bos_check2'
   58 |     __builtin___ ## fun ## _chk(dst, src, __ssp_bos0(dst)) : \
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~
note: expanded from here
C:\ncs2\v2.5.0\nrf\applications\asset_tracker_v2\src\events\app_module_event.c:87:4: note: Call to function 'strcat' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcat'. CWE-119
   87 |                         strcat(data_types, type2str(event->data_list[i]));
      |                         ^
C:/ncs/toolchains/c57af46cb7/opt/zephyr-sdk/arm-zephyr-eabi/arm-zephyr-eabi/sys-include\ssp/string.h:110:26: note: expanded from macro 'strcat'
  110 | #define strcat(dst, src) __ssp_bos_check2(strcat, dst, src)
      |                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
C:/ncs/toolchains/c57af46cb7/opt/zephyr-sdk/arm-zephyr-eabi/arm-zephyr-eabi/sys-include\ssp/string.h:58:5: note: expanded from macro '__ssp_bos_check2'
   58 |     __builtin___ ## fun ## _chk(dst, src, __ssp_bos0(dst)) : \
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~
note: expanded from here

Other Extensions

nRF Connect for VS Code Extension Pack

Additional context

Lines 84-87 of source file if (strlen(type2str(event->data_list[i])) > max_append_len) { return; } strcat(data_types, type2str(event->data_list[i])); //strncat(data_types, type2str(event->data_list[i]), // sizeof data_types - strlen(data_types) - 1);

Burt-Silverman commented 9 months ago

But studying this more, it appears to be an issue with the nordic-semiconductor.nrf-connect configuration provider being optimized for IntelliSense but providing the wrong set of includes (or something) for code analysis. Nevertheless, if someone here can provide insight into the Log Diagnostics I show in the ticket, prior to closing the ticket, that would be appreciated.

sean-mcmanus commented 9 months ago

@Burt-Silverman I believe the cause is "C:/ncs/toolchains/c57af46cb7/opt/zephyr-sdk/arm-zephyr-eabi/arm-zephyr-eabi/sys-include\ssp/string.h" is matching an excluded file -- warnings with a "note: expanded from macro" from an excluded file are filtered out of the Problems (squiggles) by default -- we could potentially add a setting to alter that filtering behavior.

Making sure that header isn't excluded would be a workaround.

Burt-Silverman commented 9 months ago

I am just a bit over my head, sorry, Sean. Is there something I can do to ensure that the header isn't excluded?

I see there is a variable _FORTIFY_SOURCE in ...picolib/include/sys/features.h that may be related to some of this, but I cannot even find where it gets set. I am over my head! But any help is appreciated, including adding a setting to alter the filtering behavior. Thanks.

Burt-Silverman commented 9 months ago

I figured out the fix at least for cases involving the macro _FORTIFY_SOURCE (e.g., strcat). [I read https://developers.redhat.com/articles/2023/07/04/developers-guide-secure-coding-fortifysource# to get up to speed.] Configuration provider nordic-semiconductor.nrf-connect puts -D_FORTIFY_SOURCE=1 into its list of -D options. If I add --extra-arg==D_FORTIFY_SOURCE=0 to C_Cpp.codeAnalysis.clangTidy.args, I solve the issue. [For example,

C:\ncs2\v2.5.0\nrf\applications\asset_tracker_v2\src\events\app_module_event.c:89:4: warning: Call to function 'strcat' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcat'. CWE-119 [clang-analyzer-security.insecureAPI.strcpy] 89 | strcat(data_types, type2str(event->data_list[i])); | ^~ is now passed to Problems (squiggles).]

I am still not entirely clear how to express this in terms of an "excluded file" that you discussed, Sean. And now I guess this should be solved at the nrf-connect and settings.json level? (Question:-) )

github-actions[bot] commented 7 months ago

This feature request is being closed due to insufficient upvotes. Please leave a 👍-upvote or 👎-downvote reaction on the issue to help us prioritize it. When enough upvotes are received, this issue will be eligible for our backlog.