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.

Sign up for GitHub

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

Jump to bottom

digital::fll_band_edge_cc::set_samples_per_symbol() is not thread safe #6466

Closed
boatbod opened this issue Jan 16, 2023 · 5 comments · Fixed by #7127
Closed

digital::fll_band_edge_cc::set_samples_per_symbol() is not thread safe #6466

boatbod opened this issue Jan 16, 2023 · 5 comments · Fixed by #7127
Assignees
@willcode
Labels
Bug digital good first issue Low Issues of low priority.

Comments

@boatbod
Copy link

boatbod commented Jan 16, 2023

What happened?

Calling digital::fll_band_edge_cc::set_samples_per_symbol() in a multi-threaded environment with the flowgraph running may cause a segmentation violation.

set_samples_per_symbol() implementation calls fll_band_edge_cc::design_filter() which may change the size of the d_filter_lower and d_filter_upper vectors which are used in the fll_band_edge_cc::work() method. In a threaded environment it is possible for work() to be running at the same time as a filter update and this may allow previously freed memory to be accessed if the filter vector gets smaller.

Suggest adding gr::thread::scoped_lock(d_mutex) to the fll_band_edge_cc block to prevent this happening.

System Information

OS: Ubuntu 20.04 LTS
GR build from sources on "maint-3.10" branch
Based on code inspection this bug appears to affect multiple releases

GNU Radio Version

3.10 (maint-3.10)

Specific Version

3.10.5.0-1-g480f600c

Steps to Reproduce the Problem

Bug was discovered after much head-scratching by rebuilding and running gnuradio and op25 with an address sanitizer.
While I don't have a specific test-case available, you should be able to reproduce the problem by running a flowgraph at the same time as making calls to set_samples_per_symbol() that increase then decrease the SPS value.

Relevant log output

No response
@willcode
Copy link
Member

Thanks. Agree there should be a mutex there, and also in other setters set_rolloff() and set_filter_size().

@willcode willcode self-assigned this May 5, 2023
@geekyraghav13
Copy link

add a lock to protect the d_filter_lower and d_filter_upper vectors from concurrent access. The proposed code change is to add gr:::scoped_lock(d_mutex) within the fll_band_edge_cc block to ensure that only one thread can access the filter vectors at a time. This will prevent the race condition and potential segmentation violation.

@willcode
Copy link
Member

willcode commented Oct 1, 2023

That's right, and feel free to submit a PR if you're interested. The lock should be in work() and probably all of the setters.

@geekyraghav13
Copy link

That's right, and feel free to submit a PR if you're interested. The lock should be in work() and probably all of the setters.

Actually i dont know about this much and terms like PR and all i am new in github

@marcusmueller
Copy link
Member

@geekyraghav13 well, you proposed what seems to be a reasonable change, so that's why we're encouraging you to send us your proposed patch in the shape of a pull request (PR). But it's alright if you can't fix this bug right now, then someone else has to do it later.

@marcusmueller marcusmueller mentioned this issue Feb 12, 2024
Merged
6 tasks
@marcusmueller marcusmueller added the Low Issues of low priority. label Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@willcode willcode

Labels
Bug digital good first issue Low Issues of low priority.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

FLL bandedge: fix math & thread safety bugs marcusmueller/gnuradio
5 participants
@mbr0wn @marcusmueller @willcode @boatbod @geekyraghav13