Add clang-tidy to CI, fix findings, merge lint workflows

closes #2000

Author: jpr42

.clang-tidy

@@ -0,0 +1,23 @@
+---
+# SPDX-License-Identifier: Apache-2.0
+Checks: >
+  -*,
+  bugprone-*,
+  cert-*,
+  clang-analyzer-*,
+  performance-*,
+  portability-*,
+  -bugprone-assignment-in-if-condition,
+  -bugprone-branch-clone,
+  -bugprone-easily-swappable-parameters,
+  -bugprone-invalid-enum-default-initialization,
+  -bugprone-multi-level-implicit-pointer-conversion,
+  -bugprone-unchecked-string-to-number-conversion,
+  -clang-analyzer-optin.core.EnumCastOutOfRange,
+  -clang-analyzer-optin.performance.Padding,
+  -performance-no-int-to-ptr,
+  -portability-avoid-pragma-once
+WarningsAsErrors: '*'
+HeaderFilterRegex: '.*/loader/.*'
+ExcludeHeaderFilterRegex: '.*/loader/(generated/.*|cJSON\.h)$'
+FormatStyle: none

.github/workflows/format.yml .github/workflows/lint.yml.github/workflows/format.yml renamed to .github/workflows/lint.yml

@@ -3,7 +3,7 @@
 #
 # SPDX-License-Identifier: Apache-2.0
 
-name: format
+name: lint
 
 on:
     push:
@@ -33,3 +33,26 @@ jobs:
         done
 
         git diff --exit-code
+
+  clang-tidy:
+    name: clang-tidy
+    runs-on: ubuntu-26.04
+    timeout-minutes: 30
+
+    env:
+      CLANG_TIDY_VERSION: 22
+
+    steps:
+    - uses: actions/checkout@v7
+    - uses: actions/setup-python@v7
+      with:
+        python-version: '3.11'
+    - run: sudo apt update
+    - run: sudo apt install --yes --no-install-recommends libwayland-dev libxrandr-dev
+    - run: |
+        cmake -S. -B build \
+        -D CMAKE_BUILD_TYPE=Release \
+        -D UPDATE_DEPS=ON \
+        -D USE_CLANG_TIDY=ON \
+        -D CLANG_TIDY_EXE=$(command -v clang-tidy-${CLANG_TIDY_VERSION})
+    - run: cmake --build build

CMakeLists.txt

@@ -26,6 +26,14 @@ if (CODE_COVERAGE)
     add_code_coverage_all_targets()
 endif()
 
+option(USE_CLANG_TIDY "Run clang-tidy on the loader sources as part of the build")
+if (USE_CLANG_TIDY)
+    if (CMAKE_VERSION VERSION_LESS "3.27")
+        message(FATAL_ERROR "USE_CLANG_TIDY requires CMake 3.27 or newer (for the SKIP_LINTING source property)")
+    endif()
+    find_program(CLANG_TIDY_EXE NAMES clang-tidy REQUIRED)
+    set(CLANG_TIDY_COMMAND "${CLANG_TIDY_EXE}")
+endif()
 
 # This variable enables downstream users to customize the target API
 # variant (e.g. Vulkan SC)

loader/CMakeLists.txt

@@ -529,6 +529,12 @@ target_link_libraries(vulkan PRIVATE loader_specific_options)
 target_link_libraries(vulkan PRIVATE Vulkan::Headers)
 add_library(Vulkan::Loader ALIAS vulkan)
 
+if (USE_CLANG_TIDY)
+    set_target_properties(vulkan PROPERTIES C_CLANG_TIDY "${CLANG_TIDY_COMMAND}")
+    # cJSON is vendored third-party code; don't hold it to this project's clang-tidy checks.
+    set_source_files_properties(cJSON.c PROPERTIES SKIP_LINTING ON)
+endif()
+
 if (UNKNOWN_FUNCTIONS_SUPPORTED)
     target_compile_definitions(vulkan PRIVATE UNKNOWN_FUNCTIONS_SUPPORTED)
     add_dependencies(vulkan loader_asm_gen_files)

loader/asm_offset.c

@@ -130,6 +130,8 @@ int main(int argc, char **argv) {
     };
 
     FILE *file = loader_fopen("gen_defines.asm", "w");
+    // NOLINTBEGIN(cert-err33-c) - this is a build-time code generator run by developers, not part of the loader's
+    // runtime attack surface; a failed write here is immediately visible as a broken build, not worth per-call checks.
     fprintf(file, "\n");
     if (assembler == MASM) {
         for (size_t i = 0; i < sizeof(values) / sizeof(values[0]); ++i) {
@@ -168,5 +170,6 @@ int main(int argc, char **argv) {
                     values[i].comment);
         }
     }
+    // NOLINTEND(cert-err33-c)
     return fclose(file);
 }

loader/dev_ext_trampoline.c

@@ -286,6 +286,7 @@ VKAPI_ATTR void VKAPI_CALL vkdev_ext248(VkDevice device) ASM_NAME("vkdev_ext248"
 VKAPI_ATTR void VKAPI_CALL vkdev_ext249(VkDevice device) ASM_NAME("vkdev_ext249");
 
 void *loader_get_dev_ext_trampoline(uint32_t index) {
+    // NOLINTNEXTLINE(bugprone-switch-missing-default-case) - falls through to the return NULL below
     switch (index) {
 #define CASE_HANDLE(num) case num: return vkdev_ext##num
         CASE_HANDLE(0);

loader/dirent_on_windows.c

@@ -23,10 +23,10 @@ extern "C" {
 typedef ptrdiff_t handle_type; /* C99's intptr_t not sufficiently portable */
 
 struct DIR {
-    handle_type handle; /* -1 for failed rewind */
-    struct _finddata_t info;
-    struct dirent result; /* d_name null iff first time */
-    char *name;           /* null-terminated char string */
+    handle_type handle;       /* -1 for failed rewind */
+    struct _finddata_t info;  // NOLINT(bugprone-reserved-identifier) - matching the Windows CRT type name
+    struct dirent result;     /* d_name null iff first time */
+    char *name;               /* null-terminated char string */
 };
 
 DIR *opendir(const VkAllocationCallbacks *pAllocator, const char *name) {

loader/loader.c

@@ -74,6 +74,7 @@
 #endif  // LOADER_ENABLE_LINUX_SORT
 
 // Generated file containing all the extension data
+// NOLINTNEXTLINE(bugprone-suspicious-include) - intentionally including a .c file to inline generated tables
 #include "vk_loader_extensions.c"
 
 struct loader_struct loader = {0};
@@ -701,21 +702,21 @@ uint32_t loader_parse_version_string(char *vers_str) {
 
     vers_tok = thread_safe_strtok(vers_str, ".\"\n\r", &context);
     if (NULL != vers_tok) {
-        major = (uint16_t)atoi(vers_tok);
+        major = (uint16_t)strtoul(vers_tok, NULL, 10);
         vers_tok = thread_safe_strtok(NULL, ".\"\n\r", &context);
         if (NULL != vers_tok) {
-            minor = (uint16_t)atoi(vers_tok);
+            minor = (uint16_t)strtoul(vers_tok, NULL, 10);
             vers_tok = thread_safe_strtok(NULL, ".\"\n\r", &context);
             if (NULL != vers_tok) {
-                patch = (uint16_t)atoi(vers_tok);
+                patch = (uint16_t)strtoul(vers_tok, NULL, 10);
                 vers_tok = thread_safe_strtok(NULL, ".\"\n\r", &context);
                 // check that we are using a 4 part version string
                 if (NULL != vers_tok) {
                     // if we are, move the values over into the correct place
                     variant = major;
                     major = minor;
                     minor = patch;
-                    patch = (uint16_t)atoi(vers_tok);
+                    patch = (uint16_t)strtoul(vers_tok, NULL, 10);
                 }
             }
         }
@@ -1168,6 +1169,8 @@ VkResult loader_add_to_ext_list(const struct loader_instance *inst, struct loade
             ext_list->capacity *= 2;
         }
 
+        // every caller passes the address of an array element or on-stack struct, never NULL
+        // NOLINTNEXTLINE(clang-analyzer-core.NonNullParamChecker)
         memcpy(&ext_list->list[ext_list->count], cur_ext, sizeof(VkExtensionProperties));
         ext_list->count++;
     }
@@ -1213,6 +1216,8 @@ VkResult loader_add_to_dev_ext_list(const struct loader_instance *inst, struct l
         ext_list->capacity *= 2;
     }
 
+    // every caller passes the address of an array element or on-stack struct, never NULL
+    // NOLINTNEXTLINE(clang-analyzer-core.NonNullParamChecker)
     memcpy(&ext_list->list[idx].props, props, sizeof(*props));
     if (entrys) {
         ext_list->list[idx].entrypoints = *entrys;
@@ -1592,7 +1597,7 @@ VkResult loader_get_icd_loader_instance_extensions(const struct loader_instance
 
     // Check if a user wants to disable the instance extension filtering behavior
     env_value = loader_getenv("VK_LOADER_DISABLE_INST_EXT_FILTER", inst);
-    if (NULL != env_value && atoi(env_value) != 0) {
+    if (NULL != env_value && strtol(env_value, NULL, 10) != 0) {
         filter_extensions = false;
     }
     loader_free_getenv(env_value, inst);
@@ -2358,6 +2363,7 @@ void loader_initialize(void) {
 
     char *loader_disable_dynamic_library_unloading_env_var = loader_getenv("VK_LOADER_DISABLE_DYNAMIC_LIBRARY_UNLOADING", NULL);
     if (loader_disable_dynamic_library_unloading_env_var &&
+        // NOLINTNEXTLINE(bugprone-not-null-terminated-result) - n=2 intentionally excludes "1x" values like "10"
         0 == strncmp(loader_disable_dynamic_library_unloading_env_var, "1", 2)) {
         loader_disable_dynamic_library_unloading = true;
         loader_log(NULL, VULKAN_LOADER_WARN_BIT, 0, "Vulkan Loader: library unloading is disabled");
@@ -2746,7 +2752,7 @@ void remove_all_non_valid_override_layers(struct loader_instance *inst, struct l
                 }
             } else {
                 if (global_layer_index == -1) {
-                    global_layer_index = i;
+                    global_layer_index = (int)i;
                 } else {
                     loader_log(
                         inst, VULKAN_LOADER_WARN_BIT | VULKAN_LOADER_LAYER_BIT, 0,
@@ -2875,7 +2881,7 @@ VkResult loader_read_layer_json(const struct loader_instance *inst, struct loade
         result = VK_ERROR_INITIALIZATION_FAILED;
         goto out;
     }
-    props.info.implementationVersion = atoi(implementation_version);
+    props.info.implementationVersion = (uint32_t)strtoul(implementation_version, NULL, 10);
 
     // Parse description
 
@@ -3057,7 +3063,7 @@ VkResult loader_read_layer_json(const struct loader_instance *inst, struct loade
             result = loader_parse_json_string(ext_item, "spec_version", &spec_version);
             if (result == VK_ERROR_OUT_OF_HOST_MEMORY) goto out;
             if (NULL != spec_version) {
-                ext_prop.specVersion = atoi(spec_version);
+                ext_prop.specVersion = (uint32_t)strtoul(spec_version, NULL, 10);
             }
             loader_instance_heap_free(inst, spec_version);
             bool ext_unsupported = wsi_unsupported_instance_extension(&ext_prop);
@@ -3092,7 +3098,7 @@ VkResult loader_read_layer_json(const struct loader_instance *inst, struct loade
             result = loader_parse_json_string(ext_item, "spec_version", &spec_version);
             if (result == VK_ERROR_OUT_OF_HOST_MEMORY) goto out;
             if (NULL != spec_version) {
-                ext_prop.specVersion = atoi(spec_version);
+                ext_prop.specVersion = (uint32_t)strtoul(spec_version, NULL, 10);
             }
             loader_instance_heap_free(inst, spec_version);
 
@@ -5010,7 +5016,7 @@ VkResult loader_create_instance_chain(const VkInstanceCreateInfo *pCreateInfo, c
         }
 
         // Create instance chain of enabled layers
-        for (int32_t i = inst->expanded_activated_layer_list.count - 1; i >= 0; i--) {
+        for (int32_t i = (int32_t)inst->expanded_activated_layer_list.count - 1; i >= 0; i--) {
             struct loader_layer_properties *layer_prop = inst->expanded_activated_layer_list.list[i];
             loader_platform_dl_handle lib_handle;
 
@@ -5409,7 +5415,7 @@ VkResult loader_create_device_chain(const VkPhysicalDevice pd, const VkDeviceCre
         loader_create_info.pNext = &chain_info;
 
         // Create instance chain of enabled layers
-        for (int32_t i = inst->expanded_activated_layer_list.count - 1; i >= 0; i--) {
+        for (int32_t i = (int32_t)inst->expanded_activated_layer_list.count - 1; i >= 0; i--) {
             struct loader_layer_properties *layer_prop = inst->expanded_activated_layer_list.list[i];
             loader_platform_dl_handle lib_handle = layer_prop->lib_handle;
 
@@ -5666,7 +5672,7 @@ VkResult loader_validate_instance_extensions(struct loader_instance *inst, const
 
         // Check if a user wants to disable the instance extension filtering behavior
         env_value = loader_getenv("VK_LOADER_DISABLE_INST_EXT_FILTER", inst);
-        if (NULL != env_value && atoi(env_value) != 0) {
+        if (NULL != env_value && strtol(env_value, NULL, 10) != 0) {
             check_if_known = false;
         }
         loader_free_getenv(env_value, inst);
@@ -5814,6 +5820,9 @@ VKAPI_ATTR VkResult VKAPI_CALL terminator_CreateInstance(const VkInstanceCreateI
     // what the application requested, and thus will increase the instance version to a level that suites their needs.
     if (pCreateInfo->pApplicationInfo && pCreateInfo->pApplicationInfo->apiVersion) {
         loader_api_version altered_version = loader_make_version(pCreateInfo->pApplicationInfo->apiVersion);
+        // per LLP_LAYER_21 (docs/LoaderLayerInterface.md), a layer that clobbers pInstance before calling down is
+        // documented to crash the loader; this is not recoverable here.
+        // NOLINTNEXTLINE(clang-analyzer-core.NullDereference)
         if (altered_version.major != ptr_instance->app_api_version.major ||
             altered_version.minor != ptr_instance->app_api_version.minor) {
             ptr_instance->app_api_version = altered_version;
@@ -5848,12 +5857,14 @@ VKAPI_ATTR VkResult VKAPI_CALL terminator_CreateInstance(const VkInstanceCreateI
     } else {
         for (uint32_t j = 0; j < pCreateInfo->enabledExtensionCount; j++) {
             if (!strcmp(pCreateInfo->ppEnabledExtensionNames[j], VK_KHR_GET_PHYSICAL_DEVICE_PROPERTIES_2_EXTENSION_NAME)) {
+                // NOLINTNEXTLINE(clang-analyzer-core.NullDereference) - see LLP_LAYER_21 note above
                 ptr_instance->supports_get_dev_prop_2 = true;
                 break;
             }
         }
     }
 
+    // NOLINTNEXTLINE(clang-analyzer-core.NullDereference) - see LLP_LAYER_21 note above
     for (uint32_t i = 0; i < ptr_instance->icd_tramp_list.count; i++) {
         icd_term = loader_icd_add(ptr_instance, &ptr_instance->icd_tramp_list.scanned_list[i]);
         if (NULL == icd_term) {
@@ -6221,6 +6232,9 @@ VKAPI_ATTR VkResult VKAPI_CALL terminator_CreateDevice(VkPhysicalDevice physical
                    dev, dev->loader_dispatch.core_dispatch.magic);
     }
 
+    // per LLP_LAYER_22 (docs/LoaderLayerInterface.md), a layer that clobbers pDevice before calling down is
+    // documented to crash the loader; this is not recoverable here.
+    // NOLINTNEXTLINE(clang-analyzer-core.NullDereference)
     dev->phys_dev_term = phys_dev_term;
 
     icd_exts.list = NULL;
@@ -6599,7 +6613,7 @@ VkResult setup_loader_tramp_phys_devs(struct loader_instance *inst, uint32_t phy
             for (uint32_t cur_idx = 0; cur_idx < old_count; ++cur_idx) {
                 if (old_to_new_index[cur_idx] == -1) {
                     new_phys_devs[new_idx] = inst->phys_devs_tramp[cur_idx];
-                    old_to_new_index[cur_idx] = new_idx;
+                    old_to_new_index[cur_idx] = (int32_t)new_idx;
                     found_count++;
                     break;
                 }
@@ -6667,7 +6681,7 @@ bool is_linux_sort_enabled(struct loader_instance *inst) {
     bool sort_items = inst->supports_get_dev_prop_2;
     char *env_value = loader_getenv("VK_LOADER_DISABLE_SELECT", inst);
     if (NULL != env_value) {
-        int32_t int_env_val = atoi(env_value);
+        int32_t int_env_val = (int32_t)strtol(env_value, NULL, 10);
         loader_free_getenv(env_value, inst);
         if (int_env_val != 0) {
             sort_items = false;
@@ -7792,6 +7806,9 @@ VKAPI_ATTR VkResult VKAPI_CALL terminator_EnumeratePhysicalDeviceGroups(
 
         // Create a temporary array (on the stack) to keep track of the
         // returned VkPhysicalDevice values.
+        // total_count may legitimately be 0 (no physical device groups); every subsequent use of local_phys_dev_groups
+        // is bounded by total_count, so a 0-byte alloca is never touched.
+        // NOLINTNEXTLINE(clang-analyzer-optin.portability.UnixAPI)
         local_phys_dev_groups = loader_stack_alloc(sizeof(struct loader_physical_device_group_term) * total_count);
         // Initialize the memory to something valid
         memset(local_phys_dev_groups, 0, sizeof(struct loader_physical_device_group_term) * total_count);
@@ -7912,6 +7929,12 @@ VKAPI_ATTR VkResult VKAPI_CALL terminator_EnumeratePhysicalDeviceGroups(
                                    icd_term->scanned_icd->lib_name);
                         goto out;
                     }
+                    // Same guard as the other two paths above: the ICD can report a larger completed count here than it did
+                    // during the counting pass. tmp_group_props and local_phys_dev_groups were both sized to the earlier
+                    // (clamped) count_this_time, so trusting a larger value here would overflow both stack allocations.
+                    if (count_this_time > total_count - cur_icd_group_count) {
+                        count_this_time = total_count - cur_icd_group_count;
+                    }
                     for (uint32_t group = 0; group < count_this_time; ++group) {
                         uint32_t cur_index = group + cur_icd_group_count;
                         local_phys_dev_groups[cur_index].group_props = tmp_group_props[group];

loader/loader_environment.c

@@ -520,7 +520,7 @@ VkResult loader_add_environment_layers(struct loader_instance *inst, const char
         }
 
         // We found a layer we're interested in, but has it been disabled...
-        bool adding = true;
+        bool adding;
         bool is_implicit = (0 == (source_prop->type_flags & VK_LAYER_TYPE_FLAG_EXPLICIT_LAYER));
         bool disabled_by_type =
             (is_implicit) ? (filters->disable_filter.disable_all_implicit) : (filters->disable_filter.disable_all_explicit);
@@ -530,11 +530,10 @@ VkResult loader_add_environment_layers(struct loader_instance *inst, const char
             loader_log(inst, VULKAN_LOADER_WARN_BIT | VULKAN_LOADER_LAYER_BIT, 0,
                        "Layer \"%s\" ignored because it has been disabled by env var \'%s\'", source_prop->info.layerName,
                        VK_LAYERS_DISABLE_ENV_VAR);
-            adding = false;
         }
 
-        // If we are supposed to filter through all layers, we need to compare the layer name against the filter.
-        // This can override the disable above, so we want to do it second.
+        // Whether a layer is force-enabled only depends on the check below - the disable check above exists purely to log
+        // why a layer was skipped, since a layer is only ever added by this function via VK_LOADER_LAYERS_ENABLE.
         // Also make sure the layer isn't already in the output_list, skip adding it if it is.
         if (check_name_matches_filter_environment_var(source_prop->info.layerName, &filters->enable_filter) &&
             !loader_find_layer_name_in_list(source_prop->info.layerName, target_list)) {

loader/loader_json.c

@@ -133,10 +133,14 @@ static VkResult loader_read_entire_file(const struct loader_instance *inst, cons
         goto out;
     }
     *out_len = stats.st_size + 1;
+    // out_buff was allocated with stats.st_size + 1 bytes above, and the fread check already verified the file's
+    // contents did not change size out from under us; this is the last valid byte.
+    // NOLINTNEXTLINE(clang-analyzer-security.ArrayBound)
     (*out_buff)[stats.st_size] = '\0';
 
 out:
     if (NULL != file) {
+        // NOLINTNEXTLINE(cert-err33-c) - file was only opened for reading, nothing to flush; res already reflects any read failure
         fclose(file);
     }
     return res;