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

Protect PFB Clock Sync from Bad Input #1554

Open
willcode opened this issue Jan 21, 2018 · 3 comments
Open

Protect PFB Clock Sync from Bad Input #1554

willcode opened this issue Jan 21, 2018 · 3 comments
Labels

Comments

@willcode
Copy link
Member

willcode commented Jan 21, 2018

Daniel Estévez (@daniestevez) reports a problem where unchecked bad input causes a stack overflow with difficult to debug results.

Problem solved. The cause was that before the PFB I had an AGC block
that when feeding it a stream of 0's, it was performing 0/0 and so
feeding a stream of nan's into the PFB.

This makes all the loop calculations propagate the nan and all loop
variables end up being nan, including d_k. Then, the line 449 in
pfb_clock_sync_ccf_impl.cc, does

d_filtnum = (int)floor(d_k);

and since d_filtnum is an integer and after this line it is adjusted to
the range [0, d_nfilters], modifying the count in the process, this
causes count to become some weird value.

So the problem was not with the PFB but with my previous block. However,
I think that the PFB should have some protection against this.

@mbr0wn mbr0wn added the Bug label Feb 4, 2018
@marcusmueller
Copy link
Member

Hm, as much as I like the idea of sanitizing inputs: do we really need to be save against NaNs anywhere? The real issue here is imho that the AGC used generates "unphysical" values out of physically viable ones.

@daniestevez
Copy link
Member

daniestevez commented Feb 11, 2018

While in this particular case with the AGC, I agree with @marcusmueller, NaNs might make sense in some special contexts. For instance, an IQ recording where some of the samples have been "lost" and replaced by NaNs.

The real question is how much of the existing GNU Radio stuff would break in this case and whether it makes any sense to defend from NaNs, since they would only happen in very special situations.

That being said, I do not think that segfaulting (which is what happens with PFB clock) is an acceptable outcome. I would be much happier if PFB clock outputted only NaNs or complete garbage in this case.

@marcusmueller
Copy link
Member

There's much truth in your comment, @daniestevez. The segfault is in any case unacceptable, indeed.

Your question "how much other GR stuff breaks if someone feeds in NaN" touches a larger topic:

Now, you and me and @mbr0wn would all see that feeding in NaN to things like DFTs would invalidate all the data, and would appropriately sanitize before if foreseeable (and I'd argue that should ideally be controllable by a flag in a potential source of NaNs); but that's not necessarily correct for all users of GR. In essence: how much performance are we willing to sacrifice for the sake of safety of use?

And that, in turn, breaks down to the question: Who's the target audience of the blocks we need to make NaN-proof? What's GR's target audience, at all?

I'd say this might turn out to be something too large to be discussed here. We should probably make this an early GREP, @mbr0wn?

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

No branches or pull requests

4 participants