-
Notifications
You must be signed in to change notification settings - Fork 50
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
Fix crash upon PSDBU report triggered on a multi-buffer frame #240
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole problem makes we wonder if the current URR event queuing model should be changed (but not in this bugfix).
It might be worth having a outstanding URR events queue per session and only queue a trigger to process the URRs to the PFCP server.
That way additional events can be added to the URR queue for a session while the trigger is in the queue. Of course, some mutual exclusion on the per session URR event buffer would be needed.
upf/upf_pfcp_server.c
Outdated
* CP-SEID in the event. | ||
*/ | ||
if (pool_is_free_index (gtm->sessions, ueh->session_idx)) | ||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The continue
skips over the vec_free_h
at the end. Wouldn't this cause a leak of the ueh
entry?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for noticing this problem. Adjusted the code slightly to avoid the leak
47b89ee
to
a05ed7c
Compare
I agree concerning the per-session URR event queue (to be done later) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
upf/upf_pfcp_server.c
Outdated
if (!sx || sx->cp_seid != ueh->cp_seid) | ||
{ | ||
vec_free_h (uev, sizeof (upf_event_urr_hdr_t)); | ||
continue; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those lines are fine, still I would have coded this as:
if (!sx || sx->cp_seid != ueh->cp_seid) | |
{ | |
vec_free_h (uev, sizeof (upf_event_urr_hdr_t)); | |
continue; | |
} | |
if (!sx || sx->cp_seid != ueh->cp_seid) | |
{ | |
goto next_ev_urr; | |
} |
with a the goto
label` place right before the free.
That way it would be harder to accidentally use different cleanups for both cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the code to use goto
, as indeed we might introduce a hard-to-catch bug later with different cleanup code paths
a05ed7c
to
015ad51
Compare
Make sure that the session is not deleted multiple times