Layers: Correctly update iterator when erasing vector element #256
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ApiLayerInterface::LoadApiLayers would load the manifest files for explicit layers in order to match them against requested layers. As the layers are matched, they would be erased from the manifest vector in order to prevent double-matching. However, in the erase case, the iterator wasn't being updated correctly.
|
Oh, I forgot about the repro. I hit this while trying to enable the core validation layer with |
|
@bradgrantham-lunarg thanks for looking into this. Is there some way we could extend the loader test to test for this? I just checked and the loader test passed fine here as-is (once I disabled the active runtime, at least - that part never seems to work quite right) |
|
@robbiesri Would you mind adding a changelog fragment in |
Oh, I don't get any credit for this, Rob just filed it independently. This does now match the idiom cppreference.com reports for vector::erase. :sheepish: However, If I remember this correctly, the outer loop walks the layers enabled by environment variable or by CreateInstance. The inner loop walks the manifest list to make sure the enabled layer was discovered, and if so it deletes that name from the discovered list so it's ignored if it's in the enabled layer list twice. I mean, that is wrong, and in that case the iterator would have been double-incremented, but I'm wondering what the failure would be in the case that there's just one explicit layer. Crash because iterator was end() and then was incremented restarting the loop body? I thought I had tested the case of one layer but maybe I didn't test properly or was sloppy. @robbiesri , what is the failure and could you post the output of XR_LOADER_DEBUG=all? |
|
Hey all!
The failure was indeed iterating past end with the iterator for Let me get you the dump from XR_LOADER_DEBUG.
Yep, no problem! I'll add that in a minute! |
|
here's the output from XR_LOADER_DEBUG |
|
Added the changelog fragment! |
rpavlik
approved these changes
May 20, 2021
|
Thanks very much! |
rpavlik
added a commit
that referenced
this pull request
Jun 8, 2021
This release features an important fix to the loader for an
invalid-iterator bug introduced in 1.0.16. All developers shipping the
loader are strongly encouraged to upgrade. It also includes a variety of
new vendor extensions.
- Registry
- Add XR_MSFT_scene_understanding vendor extension. (internal MR
2032)
- Add XR_MSFT_scene_understanding_serialization vendor extension.
(internal MR 2032)
- Add XR_MSFT_composition_layer_reprojection vendor extension.
(internal MR 2033)
- Add XR_OCULUS_audio_device_guid vendor extension. (internal MR
2053)
- Add version 3 of XR_FB_swapchain_update_state vendor extension,
which splits platform and graphics API specific structs into
separate extensions. (internal MR 2059)
- Apply formatting to registry XML by selectively committing
changes made by https://github.com/rpavlik/PrettyRegistryXml.
(internal MR 2070, OpenXR-SDK-Source/#256)
- Enforce that all xrCreate functions must be able to return
XR_ERROR_LIMIT_REACHED and XR_ERROR_OUT_OF_MEMORY, and adjust
lists of error codes accordingly. (internal MR 2064)
- Fix a usage of > without escaping as an XML entity. (internal MR
2064)
- Fix all cases of a success code (most often
XR_SESSION_LOSS_PENDING) appearing in the errorcodes attribute
of a command. (internal MR 2064, internal issue 1566)
- Improve comments for several enum values. (internal MR 1982)
- Perform some script clean-up and refactoring, including
selective type annotation and moving the Conventions abstract
base class to spec_tools. (internal MR 2064)
- Sort return codes, with some general, popular codes made to be
early. Script sort_codes.py can be used to maintain this, though
it mangles other XML formatting, so use it with care.
https://github.com/rpavlik/PrettyRegistryXml can format, and
eventually sort return codes (currently sort order does not
match). (internal MR 2064, OpenXR-SDK-Source/#256)
- SDK
- Loader: Fix iteration over explicit layer manifests.
(OpenXR-SDK-Source/#256)
- validation layer: Don’t try to apply strlen to wchar_t-based
output buffers. (internal MR 2053)
rhabacker
pushed a commit
to rhabacker/OpenXR-SDK-Source
that referenced
this pull request
Nov 16, 2022
This release features an important fix to the loader for an
invalid-iterator bug introduced in 1.0.16. All developers shipping the
loader are strongly encouraged to upgrade. It also includes a variety of
new vendor extensions.
- Registry
- Add XR_MSFT_scene_understanding vendor extension. (internal MR
2032)
- Add XR_MSFT_scene_understanding_serialization vendor extension.
(internal MR 2032)
- Add XR_MSFT_composition_layer_reprojection vendor extension.
(internal MR 2033)
- Add XR_OCULUS_audio_device_guid vendor extension. (internal MR
2053)
- Add version 3 of XR_FB_swapchain_update_state vendor extension,
which splits platform and graphics API specific structs into
separate extensions. (internal MR 2059)
- Apply formatting to registry XML by selectively committing
changes made by https://github.com/rpavlik/PrettyRegistryXml.
(internal MR 2070, OpenXR-SDK-Source/KhronosGroup#256)
- Enforce that all xrCreate functions must be able to return
XR_ERROR_LIMIT_REACHED and XR_ERROR_OUT_OF_MEMORY, and adjust
lists of error codes accordingly. (internal MR 2064)
- Fix a usage of > without escaping as an XML entity. (internal MR
2064)
- Fix all cases of a success code (most often
XR_SESSION_LOSS_PENDING) appearing in the errorcodes attribute
of a command. (internal MR 2064, internal issue 1566)
- Improve comments for several enum values. (internal MR 1982)
- Perform some script clean-up and refactoring, including
selective type annotation and moving the Conventions abstract
base class to spec_tools. (internal MR 2064)
- Sort return codes, with some general, popular codes made to be
early. Script sort_codes.py can be used to maintain this, though
it mangles other XML formatting, so use it with care.
https://github.com/rpavlik/PrettyRegistryXml can format, and
eventually sort return codes (currently sort order does not
match). (internal MR 2064, OpenXR-SDK-Source/KhronosGroup#256)
- SDK
- Loader: Fix iteration over explicit layer manifests.
(OpenXR-SDK-Source/KhronosGroup#256)
- validation layer: Don’t try to apply strlen to wchar_t-based
output buffers. (internal MR 2053)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ApiLayerInterface::LoadApiLayers would load the manifest files for explicit layers in order to match them against requested layers. As the layers are matched, they would be erased from the manifest vector in order to prevent double-matching. However, in the erase case, the iterator wasn't being updated correctly.