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

app_rpt.c, rpt_cli.c, rpt_manager.c: Add missing mutex_lock() #512

Merged
merged 1 commit into from
Feb 27, 2025

Conversation

mkmer
Copy link
Collaborator

@mkmer mkmer commented Feb 24, 2025

It appears these calls should be mutex_locked.

@mkmer
Copy link
Collaborator Author

mkmer commented Feb 24, 2025

While reviewing code that generated this PR, I notice this:

void rpt_qwrite(struct rpt_link *l, struct ast_frame *f)
{
struct ast_frame *f1;
if (!l->chan)
return;
f1 = ast_frdup(f);
memset(&f1->frame_list, 0, sizeof(f1->frame_list));
AST_LIST_INSERT_TAIL(&l->textq, f1, frame_list);
return;
}

I'm not positive, but I think we should be locked when adding to the list? Or maybe when calling rpt_qwrite()?

@mkmer mkmer added the code quality Improvments around code quality without functional changes label Feb 24, 2025
@mkmer mkmer self-assigned this Feb 24, 2025
@InterLinked1
Copy link
Member

While reviewing code that generated this PR, I notice this:

void rpt_qwrite(struct rpt_link *l, struct ast_frame *f)
{
struct ast_frame *f1;
if (!l->chan)
return;
f1 = ast_frdup(f);
memset(&f1->frame_list, 0, sizeof(f1->frame_list));
AST_LIST_INSERT_TAIL(&l->textq, f1, frame_list);
return;
}

I'm not positive, but I think we should be locked when adding to the list? Or maybe when calling rpt_qwrite()?

Ah, that's a good point. It looks like it's using a linked list without a lock so some kind of locking does seem needed.

@mkmer mkmer force-pushed the mutex_lock-around-__mklinklist branch 4 times, most recently from 2ce910a to 2bee58f Compare February 25, 2025 00:31
@mkmer mkmer force-pushed the mutex_lock-around-__mklinklist branch 4 times, most recently from 50db7c8 to 6cd6bc3 Compare February 25, 2025 01:58
@mkmer mkmer force-pushed the mutex_lock-around-__mklinklist branch from 6cd6bc3 to 7cb7bfb Compare February 25, 2025 02:08
Copy link
Collaborator

@Allan-N Allan-N left a comment

Choose a reason for hiding this comment

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

I'm approving this change as an incremental improvement. But, we really need to take a step back and review/analyze/update all of the logic around myrpt and the associated links.

Why? If you look at the call to __mklnklist inside the periodic_process_links() function you will see we are passing myrpt and l. Even if we lock myrpt one can/should ask whether l is still valid as it is captured from myrpt->links.next at the top of the function and advanced at the end of the while() loop without holding any locks.

@Allan-N Allan-N merged commit deb6be5 into AllStarLink:master Feb 27, 2025
2 checks passed
@mkmer mkmer deleted the mutex_lock-around-__mklinklist branch February 28, 2025 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality Improvments around code quality without functional changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants