Skip to content

Commit c17422c

Browse files
BoiledElectricitycharles-lunarg
authored andcommitted
loader: Match select/disable filters on full name
check_name_matches_filter_environment_var copied the name being tested into a fixed VK_MAX_EXTENSION_NAME_SIZE buffer, clamping it to the first 255 characters before matching. The driver select/disable filters run against the manifest filename, so any name past that length was matched on a truncated copy. Match against the name in place instead. Filter values are already lowercased at parse time, so a small helper folds the name side as it compares, with no copy and no length cap. Also keep the stored filter length in sync with the value buffer and null terminated so the matcher never reads past it. Fixes: #1913
1 parent 4b9fa87 commit c17422c

4 files changed

Lines changed: 121 additions & 45 deletions

File tree

loader/loader_environment.c

Lines changed: 44 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -275,14 +275,12 @@ VkResult parse_generic_filter_environment_var(const struct loader_instance *inst
275275
const char *actual_start;
276276
size_t actual_len;
277277
determine_filter_type(token, &cur_filter_type, &actual_start, &actual_len);
278-
if (actual_len > VK_MAX_EXTENSION_NAME_SIZE) {
279-
loader_strncpy(filter_struct->filters[filter_struct->count].value, VK_MAX_EXTENSION_NAME_SIZE, actual_start,
280-
VK_MAX_EXTENSION_NAME_SIZE);
281-
} else {
282-
loader_strncpy(filter_struct->filters[filter_struct->count].value, VK_MAX_EXTENSION_NAME_SIZE, actual_start,
283-
actual_len);
284-
}
285-
filter_struct->filters[filter_struct->count].length = actual_len;
278+
// value holds at most VK_MAX_EXTENSION_NAME_SIZE - 1 chars plus the null terminator. Keep the stored length in sync
279+
// with what actually fits so the matcher never walks past the buffer, and make sure it stays null terminated.
280+
size_t stored_len = actual_len < VK_MAX_EXTENSION_NAME_SIZE - 1 ? actual_len : VK_MAX_EXTENSION_NAME_SIZE - 1;
281+
loader_strncpy(filter_struct->filters[filter_struct->count].value, VK_MAX_EXTENSION_NAME_SIZE, actual_start, stored_len);
282+
filter_struct->filters[filter_struct->count].value[stored_len] = '\0';
283+
filter_struct->filters[filter_struct->count].length = stored_len;
286284
filter_struct->filters[filter_struct->count++].type = cur_filter_type;
287285
if (filter_struct->count >= MAX_ADDITIONAL_FILTERS) {
288286
break;
@@ -346,14 +344,13 @@ VkResult parse_layers_disable_filter_environment_var(const struct loader_instanc
346344
disable_struct->disable_all_explicit = true;
347345
}
348346
} else {
349-
if (actual_len > VK_MAX_EXTENSION_NAME_SIZE) {
350-
loader_strncpy(disable_struct->additional_filters.filters[cur_count].value, VK_MAX_EXTENSION_NAME_SIZE,
351-
actual_start, VK_MAX_EXTENSION_NAME_SIZE);
352-
} else {
353-
loader_strncpy(disable_struct->additional_filters.filters[cur_count].value, VK_MAX_EXTENSION_NAME_SIZE,
354-
actual_start, actual_len);
355-
}
356-
disable_struct->additional_filters.filters[cur_count].length = actual_len;
347+
// Keep the stored length in sync with what actually fits (value is VK_MAX_EXTENSION_NAME_SIZE bytes including
348+
// the null terminator) so the matcher never reads past the buffer.
349+
size_t stored_len = actual_len < VK_MAX_EXTENSION_NAME_SIZE - 1 ? actual_len : VK_MAX_EXTENSION_NAME_SIZE - 1;
350+
loader_strncpy(disable_struct->additional_filters.filters[cur_count].value, VK_MAX_EXTENSION_NAME_SIZE, actual_start,
351+
stored_len);
352+
disable_struct->additional_filters.filters[cur_count].value[stored_len] = '\0';
353+
disable_struct->additional_filters.filters[cur_count].length = stored_len;
357354
disable_struct->additional_filters.filters[cur_count].type = cur_filter_type;
358355
disable_struct->additional_filters.count++;
359356
if (disable_struct->additional_filters.count >= MAX_ADDITIONAL_FILTERS) {
@@ -385,6 +382,18 @@ VkResult parse_layer_environment_var_filters(const struct loader_instance *inst,
385382
return res;
386383
}
387384

385+
// Case-insensitive compare of `count` bytes of a name against a filter value. Filter values are already lowercased when
386+
// they get parsed (see parse_generic_filter_environment_var), so we only need to fold the name side as we go. The caller
387+
// guarantees both sides have at least `count` valid bytes.
388+
static bool name_segment_matches_filter_value(const char *name_segment, const char *lowercase_filter_value, size_t count) {
389+
for (size_t iii = 0; iii < count; ++iii) {
390+
if ((char)tolower((unsigned char)name_segment[iii]) != lowercase_filter_value[iii]) {
391+
return false;
392+
}
393+
}
394+
return true;
395+
}
396+
388397
// Check to see if the provided layer name matches any of the filter strings.
389398
// This will properly check against:
390399
// - substrings "*string*"
@@ -394,54 +403,44 @@ VkResult parse_layer_environment_var_filters(const struct loader_instance *inst,
394403
bool check_name_matches_filter_environment_var(const char *name, const struct loader_envvar_filter *filter_struct) {
395404
bool ret_value = false;
396405
size_t name_len = strlen(name);
397-
// name may be a manifest filename (from the driver select/disable filters) which, unlike layer names, is not capped to
398-
// VK_MAX_EXTENSION_NAME_SIZE at parse time. Clamp so the lowercase copy stays inside lower_name.
399-
if (name_len >= VK_MAX_EXTENSION_NAME_SIZE) {
400-
name_len = VK_MAX_EXTENSION_NAME_SIZE - 1;
401-
}
402-
char lower_name[VK_MAX_EXTENSION_NAME_SIZE];
403-
for (uint32_t iii = 0; iii < name_len; ++iii) {
404-
lower_name[iii] = (char)tolower((unsigned char)name[iii]);
405-
}
406-
lower_name[name_len] = '\0';
406+
// Compare each filter against `name` directly. name_segment_matches_filter_value does a case-insensitive compare
407+
// (filter values are already lowercased at parse time), so there's no need to make a lowercased copy of name first
408+
// and no limit on how long name can be.
407409
for (uint32_t filt = 0; filt < filter_struct->count; ++filt) {
408-
// Check if the filter name is longer (this is with all special characters removed), and if it is
409-
// continue since it can't match.
410-
if (filter_struct->filters[filt].length > name_len) {
410+
const struct loader_envvar_filter_value *filter = &filter_struct->filters[filt];
411+
// The filter (with its wildcards stripped) is longer than the name, so it can't possibly match.
412+
if (filter->length > name_len) {
411413
continue;
412414
}
413-
switch (filter_struct->filters[filt].type) {
415+
switch (filter->type) {
414416
case FILTER_STRING_SPECIAL:
415-
if (!strcmp(VK_LOADER_DISABLE_ALL_LAYERS_VAR_1, filter_struct->filters[filt].value) ||
416-
!strcmp(VK_LOADER_DISABLE_ALL_LAYERS_VAR_2, filter_struct->filters[filt].value) ||
417-
!strcmp(VK_LOADER_DISABLE_ALL_LAYERS_VAR_3, filter_struct->filters[filt].value)) {
417+
if (!strcmp(VK_LOADER_DISABLE_ALL_LAYERS_VAR_1, filter->value) ||
418+
!strcmp(VK_LOADER_DISABLE_ALL_LAYERS_VAR_2, filter->value) ||
419+
!strcmp(VK_LOADER_DISABLE_ALL_LAYERS_VAR_3, filter->value)) {
418420
ret_value = true;
419421
}
420422
break;
421423

422424
case FILTER_STRING_SUBSTRING:
423-
if (NULL != strstr(lower_name, filter_struct->filters[filt].value)) {
424-
ret_value = true;
425+
// Slide the filter along the name looking for a match, stopping as soon as one is found.
426+
for (size_t start = 0; start + filter->length <= name_len; ++start) {
427+
if (name_segment_matches_filter_value(name + start, filter->value, filter->length)) {
428+
ret_value = true;
429+
break;
430+
}
425431
}
426432
break;
427433

428434
case FILTER_STRING_SUFFIX:
429-
if (0 == strncmp(lower_name + name_len - filter_struct->filters[filt].length, filter_struct->filters[filt].value,
430-
filter_struct->filters[filt].length)) {
431-
ret_value = true;
432-
}
435+
ret_value = name_segment_matches_filter_value(name + name_len - filter->length, filter->value, filter->length);
433436
break;
434437

435438
case FILTER_STRING_PREFIX:
436-
if (0 == strncmp(lower_name, filter_struct->filters[filt].value, filter_struct->filters[filt].length)) {
437-
ret_value = true;
438-
}
439+
ret_value = name_segment_matches_filter_value(name, filter->value, filter->length);
439440
break;
440441

441442
case FILTER_STRING_FULLNAME:
442-
if (0 == strncmp(lower_name, filter_struct->filters[filt].value, name_len)) {
443-
ret_value = true;
444-
}
443+
ret_value = (name_len == filter->length) && name_segment_matches_filter_value(name, filter->value, filter->length);
445444
break;
446445
}
447446
if (ret_value) {

loader/loader_json.c

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,18 @@ static VkResult loader_read_entire_file(const struct loader_instance *inst, cons
5757
if (MultiByteToWideChar(CP_UTF8, 0, filename, -1, filename_utf16, filename_utf16_size) == filename_utf16_size) {
5858
file_handle =
5959
CreateFileW(filename_utf16, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
60+
// A path at or beyond MAX_PATH won't open unless we opt into long paths with the "\\?\" prefix, so retry with it.
61+
if (INVALID_HANDLE_VALUE == file_handle && filename_utf16_size >= MAX_PATH) {
62+
int prefixed_utf16_size = filename_utf16_size + 4; // room for the leading "\\?\"
63+
wchar_t *prefixed_utf16 = (wchar_t *)loader_stack_alloc(prefixed_utf16_size * sizeof(wchar_t));
64+
prefixed_utf16[0] = L'\\';
65+
prefixed_utf16[1] = L'\\';
66+
prefixed_utf16[2] = L'?';
67+
prefixed_utf16[3] = L'\\';
68+
memcpy(prefixed_utf16 + 4, filename_utf16, filename_utf16_size * sizeof(wchar_t));
69+
file_handle =
70+
CreateFileW(prefixed_utf16, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
71+
}
6072
}
6173
}
6274
if (INVALID_HANDLE_VALUE == file_handle) {

tests/framework/util/folder_manager.cpp

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,13 @@ std::filesystem::path Folder::write_manifest(std::filesystem::path const& name,
6565
std::filesystem::path out_path = (folder / name).lexically_normal();
6666
if (!::testing::internal::InDeathTestChild()) {
6767
auto file = std::ofstream(out_path, std::ios_base::trunc | std::ios_base::out);
68+
#if defined(_WIN32)
69+
// If the manifest path is too long for Windows, retry with the \\?\ absolute-path prefix.
70+
if (!file) {
71+
std::wstring long_path = L"\\\\?\\" + std::filesystem::absolute(out_path).native();
72+
file = std::ofstream(long_path.c_str(), std::ios_base::trunc | std::ios_base::out);
73+
}
74+
#endif
6875
if (!file) {
6976
std::cerr << "Failed to create manifest " << name << " at " << out_path << "\n";
7077
return out_path;

tests/loader_envvar_tests.cpp

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -714,6 +714,64 @@ TEST(EnvVarICDOverrideSetup, FilterSelectDriver) {
714714
ASSERT_FALSE(env.debug_log.find_prefix_then_postfix("CDE_ICD.json", "ignored because it was disabled by env var"));
715715
}
716716

717+
// Exercise the driver select/disable filters against a manifest name that is much longer than VK_MAX_EXTENSION_NAME_SIZE
718+
// would have allowed when the matcher copied the name into a fixed-size buffer. The whole name has to be considered for
719+
// every filter type, not just the first 255 characters. See KhronosGroup/Vulkan-Loader#1913.
720+
TEST(EnvVarICDOverrideSetup, FilterDriverLongManifestName) {
721+
FrameworkEnvironment env{};
722+
EnvVarWrapper filter_select_env_var{"VK_LOADER_DRIVERS_SELECT"};
723+
EnvVarWrapper filter_disable_env_var{"VK_LOADER_DRIVERS_DISABLE"};
724+
725+
// Filesystems cap a single filename at NAME_MAX (255 on the platforms the tests run on), so use the longest practical
726+
// name. The distinguishing part lives at the very end so any matcher that stops early gets it wrong.
727+
const std::string long_name = "long_driver_" + std::string(220, 'a') + "_unique_ICD.json";
728+
ASSERT_LT(long_name.size(), static_cast<size_t>(255));
729+
env.add_icd(TEST_ICD_PATH_VERSION_6, ManifestOptions{}.set_json_name(long_name));
730+
731+
// A suffix filter that only matches via characters past the old 255 limit should still select the driver.
732+
filter_select_env_var.set_new_value("*_unique_ICD.json");
733+
{
734+
InstWrapper inst{env.vulkan_functions};
735+
FillDebugUtilsCreateDetails(inst.create_info, env.debug_log);
736+
inst.CheckCreate();
737+
ASSERT_TRUE(env.debug_log.find_prefix_then_postfix("Found ICD manifest file", long_name.c_str()));
738+
ASSERT_FALSE(env.debug_log.find_prefix_then_postfix(long_name.c_str(), "ignored because not selected by env var"));
739+
}
740+
741+
// A substring filter spanning the tail of the name should match as well.
742+
env.debug_log.clear();
743+
filter_select_env_var.set_new_value("*a_unique*");
744+
{
745+
InstWrapper inst{env.vulkan_functions};
746+
FillDebugUtilsCreateDetails(inst.create_info, env.debug_log);
747+
inst.CheckCreate();
748+
ASSERT_TRUE(env.debug_log.find_prefix_then_postfix("Found ICD manifest file", long_name.c_str()));
749+
ASSERT_FALSE(env.debug_log.find_prefix_then_postfix(long_name.c_str(), "ignored because not selected by env var"));
750+
}
751+
752+
// The full long name matched exactly must select the driver.
753+
env.debug_log.clear();
754+
filter_select_env_var.set_new_value(long_name);
755+
{
756+
InstWrapper inst{env.vulkan_functions};
757+
FillDebugUtilsCreateDetails(inst.create_info, env.debug_log);
758+
inst.CheckCreate();
759+
ASSERT_TRUE(env.debug_log.find_prefix_then_postfix("Found ICD manifest file", long_name.c_str()));
760+
ASSERT_FALSE(env.debug_log.find_prefix_then_postfix(long_name.c_str(), "ignored because not selected by env var"));
761+
}
762+
763+
// A disable filter keyed off the tail must drop the driver, leaving no usable ICD.
764+
env.debug_log.clear();
765+
filter_select_env_var.remove_value();
766+
filter_disable_env_var.set_new_value("*_unique_ICD.json");
767+
{
768+
InstWrapper inst{env.vulkan_functions};
769+
FillDebugUtilsCreateDetails(inst.create_info, env.debug_log);
770+
inst.CheckCreate(VK_ERROR_INCOMPATIBLE_DRIVER);
771+
ASSERT_TRUE(env.debug_log.find_prefix_then_postfix(long_name.c_str(), "ignored because it was disabled by env var"));
772+
}
773+
}
774+
717775
// Test that the driver filter disable disables driver manifest files that match the filter
718776
TEST(EnvVarICDOverrideSetup, FilterDisableDriver) {
719777
FrameworkEnvironment env{};

0 commit comments

Comments
 (0)