Validation layer: null pointer deref when struct contains handle

[rpavlik]

In the generated code below, there is no check for non-null value before performing value->actionSet

XrResult ValidateXrStruct(GenValidUsageXrInstanceInfo *instance_info, const std::string &command_name,
                          std::vector<GenValidUsageXrObjectInfo>& objects_info, bool check_members,
                          const XrActiveActionSet* value) {
    XrResult xr_result = XR_SUCCESS;
    // If we are not to check the rest of the members, just return here.
    if (!check_members || XR_SUCCESS != xr_result) {
        return xr_result;
    }
    {
        // writeValidateInlineHandleValidation
        ValidateXrHandleResult handle_result = VerifyXrActionSetHandle(&value->actionSet);
        if (handle_result != VALIDATE_XR_HANDLE_SUCCESS) {
            // Not a valid handle or NULL (which is not valid in this case)
            std::ostringstream oss;
            oss << "Invalid XrActionSet handle \"actionSet\" ";
            oss << HandleToHexString(value->actionSet);
            CoreValidLogMessage(instance_info, "VUID-XrActiveActionSet-actionSet-parameter",
                                VALID_USAGE_DEBUG_SEVERITY_ERROR, command_name,
                                objects_info, oss.str());
            return XR_ERROR_HANDLE_INVALID;
        }
    }
    // Everything checked out properly
    return xr_result;
}

[bradgrantham-lunarg]

Thanks. Do you have a test program?

[rpavlik]

No, this was found via static analysis with clang/CodeChecker.

[bradgrantham-lunarg]

I think I see. In

XrResult ValidateXrStruct(GenValidUsageXrInstanceInfo *instance_info, const std::string &command_name, std::vector<GenValidUsageXrObjectInfo>& objects_info, bool check_members, const XrActionsSyncInfo* value)

There's a check that activeActionSets != nullptr if countActiveActionSets > 0, but it only sets xr_result and nothing later tests that result. So this is similar to #160 in that it needs to gate the next call on xr_result == XR_SUCCESS or just return the validation error.

I'll put this in my queue.

(In the event that activeActionSets == nullptr && countActiveActionSets > 0, at least at the present time there will be a validation error printed.)

[rpavlik]

No, I don't think that's quite right.

So this addition to hello_xr causes a warning from the validation layer ("XR_ERROR_VALIDATION_FAILURE in xrSyncActions: (syncInfo->countActiveActionSets == 0)"), but it doesn't error. (Is that intended?)

diff --git a/src/tests/hello_xr/openxr_program.cpp b/src/tests/hello_xr/openxr_program.cpp
index 4e8976d..c2ae201 100644
--- a/src/tests/hello_xr/openxr_program.cpp
+++ b/src/tests/hello_xr/openxr_program.cpp
@@ -554,6 +554,11 @@ struct OpenXrProgram : IOpenXrProgram {
         InitializeActions();
         CreateVisualizedSpaces();
 
+        XrActionsSyncInfo info{XR_TYPE_ACTIONS_SYNC_INFO, nullptr};
+        info.activeActionSets = nullptr;
+        info.countActiveActionSets = 0;
+        xrSyncActions(m_session, &info);
+
         {
             XrReferenceSpaceCreateInfo referenceSpaceCreateInfo = GetXrReferenceSpaceCreateInfo(m_options->AppSpace);
             CHECK_XRCMD(xrCreateReferenceSpace(m_session, &referenceSpaceCreateInfo, &m_appSpace));

while this change will trigger the bad behavior of this bug:

diff --git a/src/tests/hello_xr/openxr_program.cpp b/src/tests/hello_xr/openxr_program.cpp
index 4e8976d..504a975 100644
--- a/src/tests/hello_xr/openxr_program.cpp
+++ b/src/tests/hello_xr/openxr_program.cpp
@@ -554,6 +554,11 @@ struct OpenXrProgram : IOpenXrProgram {
         InitializeActions();
         CreateVisualizedSpaces();
 
+        XrActionsSyncInfo info{XR_TYPE_ACTIONS_SYNC_INFO, nullptr};
+        info.activeActionSets = nullptr;
+        info.countActiveActionSets = 5; // this is a lie
+        xrSyncActions(m_session, &info);
+
         {
             XrReferenceSpaceCreateInfo referenceSpaceCreateInfo = GetXrReferenceSpaceCreateInfo(m_options->AppSpace);
             CHECK_XRCMD(xrCreateReferenceSpace(m_session, &referenceSpaceCreateInfo, &m_appSpace));

resulting in

[15:22:46.498][Info   ] Available reference spaces: 3
fish: “build/src/tests/hello_xr/hello_…” terminated by signal SIGSEGV (Address boundary error)

[bradgrantham-lunarg]

In what way is it "not quite right"?

If I apply your second patch (activeActionSets = nullptr and countActiveActionSets = 5) I get the message:

[VALID_ERROR | VUID-XrActionsSyncInfo-activeActionSets-parameter | xrSyncActions]: Structure XrActionsSyncInfo member countActiveActionSets is NULL, but value->countActiveActionSets is greater than 0
  Objects:
   [0] - XrSession (0x0000000000000002)

like I said it would. Does that not print out for you?

Then I get a crash, like the static check implies. That's because the pointer is checked but then the code continues on because xr_result is set and then never examined.

If I add if(xr_result != XR_SUCCESS) return xr_result; in ValidateXrStruct after if (0 != value->countActiveActionSets && nullptr == value->activeActionSets) { ... } and I wrap your call to xrSyncActions with CHECK_XRCMD then hello_xr closes down after a validation error. That's what I expect. Would you expect something different?

I think we need, like #160, to figure out whether to check xr_result after every test or just plain return. Do you disagree?

(The behavior from the layer on syncInfo->countActiveActionSets == 0 might be muddled. It's not an error, but I think it's probably not what apps want, so it gets a validation message. I would argue in any case that's not part of this bug.)

[rpavlik]

I don't think I got that message (or any message?) but perhaps I didn't have the layer configured right. As long as you see what's happening with the second patch, then we're good - that's much clearer than my prose from last week.

[bradgrantham-lunarg]

I did have to set the environment variable for logging to stdout. I think (grepping for "Env", not in front of my running build at the moment) it's XR_CORE_VALIDATION_EXPORT_TYPE=text. Do we maybe need a wiki page on GH explaining how to run the validation layers for us and for others?

[rpavlik]

Yeah, that or a readme. Any reason why logging to stdout isn't default?

[rpavlik-bot]

An issue (number 1322) has been filed to correspond to this issue in the internal Khronos GitLab.

If you have a Khronos account, you can access that issue at KHR:openxr/openxr#1322.

[rpavlik]

fwiw, if checking gets moved up, I'd prefer to convert the use of a pointer to a reference: lacking use of gsl::not_null passing a reference instead of a pointer makes this status clear.

[bradgrantham-lunarg]

Yeah, that or a readme. Any reason why logging to stdout isn't default?

It turns out there is a readme already. I didn't know it was there either.
https://github.com/KhronosGroup/OpenXR-SDK-Source/blob/master/src/api_layers/README_core_validation.md

[rpavlik]

Fix released in 1.0.9