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

Jes/uac registrant null pointer dereference #3165

Closed

Conversation

jes
Copy link
Contributor

@jes jes commented Aug 22, 2023

Summary
I was seeing segfaults when trying to make opensips register against a server with a much-too-short interval. I traced the problem back to a uac_registrant null pointer dereference of msg->min_expires.

Details
This commit makes uac_registrant only try to dereference msg->min_expires if it is non-NULL.

Solution
I wrapped the dereference of msg->min_expires in if (msg->min_expires).

Compatibility
It is possible that we would instead want parse_min_expires() to return nonzero if msg->min_expires is NULL. I couldn't quite work out what logic was intended. I think the code in this PR will fix the segfault, but it may not correct the intended behaviour.

Closing issues

@jes jes force-pushed the jes/uac_registrant-null-pointer-dereference branch from fc10bb5 to 5823ab4 Compare August 22, 2023 16:10
@bogdan-iancu
Copy link
Member

Hi @jes , imho the proper fix would be to have parse_min_expires() returning false if there is no hdr or if it failed to parse.

bogdan-iancu added a commit that referenced this pull request Oct 11, 2023
@bogdan-iancu
Copy link
Member

@jes , see me alternative commit - if works for you, I will backport and close here.

@bogdan-iancu bogdan-iancu self-assigned this Oct 11, 2023
@jes
Copy link
Contributor Author

jes commented Oct 12, 2023

@bogdan-iancu looks good to me!

It solves the segfault, and the logic looks fine for the uac_registrant case. I didn't check if anything else is relying on the old behaviour with parse_min_expires() returning 0 with msg->min_expires == NULL, but it seems unlikely and you're more likely to know than me anyway.

bogdan-iancu added a commit that referenced this pull request Oct 12, 2023
bogdan-iancu added a commit that referenced this pull request Oct 12, 2023
bogdan-iancu added a commit that referenced this pull request Oct 12, 2023
@bogdan-iancu
Copy link
Member

Thanks @jes for the confirmation, the backports are in place, closing here

sobomax pushed a commit to sippy/opensips that referenced this pull request Dec 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants