Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

audio: selector: fix init pointers #9473

Closed
wants to merge 1 commit into from

Conversation

cujomalainey
Copy link
Member

Selector has been broken since its last init migration as the memcpy passes in the first byte of data rather than the pointer to the data.

Fixes: 141efbd ("ipc4: convert selector to use the new module interface")
Fixes: oss-fuzz:59343

Selector has been broken since its last init migration as the memcpy
passes in the first byte of data rather than the pointer to the data.

Fixes: 141efbd ("ipc4: convert selector to use the new module interface")
Fixes: oss-fuzz:59343
Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>
@cujomalainey
Copy link
Member Author

This also suggests that our stub testing could go a bit further and actually initialize the components.

@cujomalainey cujomalainey added bug Something isn't working as expected IPC3 labels Sep 13, 2024
Copy link
Collaborator

@lyakh lyakh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the current code is correct. The commit is copying an address of the .data member of struct ipc_process, which doesn't look correct to me

@cujomalainey
Copy link
Member Author

I think the current code is correct. The commit is copying an address of the .data member of struct ipc_process, which doesn't look correct to me

Hmm, actually the changes appear to resolve to the same value in gcc and clang on macos, but when I was testing in the fuzzer environment (clang) on my linux machine I was getting values [0,255] which suggested it was dereferencing the pointer.

I'll rerun this when I am back in the office, i hope its not another hole in the C spec like integer promotion.

#include <inttypes.h>
#include <stdio.h>

struct test {
        size_t size;
        uint8_t data[];
};

int main() {
        uint8_t buf[sizeof(struct test) + 1] = {0xff};
        struct test *ptr = (struct test*)buf;

        printf("ptr: %p, ptr->data: %p, &ptr->data: %p\n", ptr, ptr->data, &ptr->data);
        return 0;
}
ptr: 0x7ff7b81260df, ptr->data: 0x7ff7b81260e7, &ptr->data: 0x7ff7b81260e7

@lyakh
Copy link
Collaborator

lyakh commented Sep 17, 2024

Hmm, actually the changes appear to resolve to the same value in gcc and clang on macos, but when I was testing in the fuzzer environment (clang) on my linux machine I was getting values [0,255] which suggested it was dereferencing the pointer.

@cujomalainey I cannot imaging how this can be correct. In your test code - yes, it's correct, because you have an array there. However .data in

const unsigned char *data;
is a pointer. In int x[2]; x and &x will have the same value. But in int *x = &i; they will have different values. x will return its contents, i.e. address of i, whereas &x will return address of the x variable. Same when that pointer is inside a structure. So if you were getting wrong values it means, that ipc_process->data contained an invalid value, not representing a pointer to data.

@cujomalainey
Copy link
Member Author

ah yes you are right i had the types confused, hmm this is interesting as I was certain I traced the path to correct for making sure data was valid. Thanks for catching the error. I will have to debug this further. Let me know if you want to take a look, i can give you the test case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working as expected IPC3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants