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

Buffer overrun in qdetector #351

Open
johnfb opened this issue Dec 19, 2023 · 1 comment
Open

Buffer overrun in qdetector #351

johnfb opened this issue Dec 19, 2023 · 1 comment

Comments

@johnfb
Copy link

johnfb commented Dec 19, 2023

I was using the qdetector and my tooling said I had a buffer overrun inside it for one of my data sets. I managed to track it down to rxy_index being zero in qdetector_execute_seek. When rxy_index is zero, _q->counter will be _q->nfft on the next call, which results in writing one past the end in the function qdetector_execute_align. There is a todo comment here

// TODO: check for edge case where rxy_index is zero (signal already aligned)
about dealing with this case.

The most straight forward way I can think to address this would be to split qdetector_execute_align into two functions and if rxy_index in qdetector_execute_seek is zero for a detection call the second half of the old qdetector_execute_align.

Here is a diff of what I think the change might be:

diff --git a/src/framing/src/qdetector.proto.c b/src/framing/src/qdetector.proto.c
index 5c13f45e..dd99a7b5 100644
--- a/src/framing/src/qdetector.proto.c
+++ b/src/framing/src/qdetector.proto.c
@@ -39,6 +39,7 @@ int QDETECTOR(_execute_seek)(QDETECTOR() _q, TI _x);
 
 // align signal in time, compute offset estimates
 int QDETECTOR(_execute_align)(QDETECTOR() _q, TI _x);
+int QDETECTOR(_execute_do_align)(QDETECTOR() _q);
 
 // main object definition
 struct QDETECTOR(_s) {
@@ -549,7 +550,9 @@ int QDETECTOR(_execute_seek)(QDETECTOR() _q, TI _x)
         _q->state = QDETECTOR_STATE_ALIGN;
         _q->offset = rxy_offset;
         _q->rxy    = rxy_peak; // note that this is a coarse estimate
-        // TODO: check for edge case where rxy_index is zero (signal already aligned)
+        if (0 == rxy_index) {
+            return QDETECTOR(_execute_do_align)(_q);
+        }
 
         // copy last part of fft input buffer to front
         memmove(_q->buf_time_0, _q->buf_time_0 + rxy_index, (_q->nfft - rxy_index)*sizeof(TI));
@@ -578,7 +581,11 @@ int QDETECTOR(_execute_align)(QDETECTOR() _q, TI _x)
 
     if (_q->counter < _q->nfft)
         return LIQUID_OK;
+    return QDETECTOR(_execute_do_align)(_q);
+}
 
+int QDETECTOR(_execute_do_align)(QDETECTOR() _q)
+{
     //printf("signal is aligned!\n");
 
     // estimate timing offset

As an aside, this library is great, thank you so much for writing it and providing it for our use.

@luzpaz
Copy link

luzpaz commented Feb 10, 2024

cc @jgaeddert

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants