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

Test condition needs verification - cFE_FS_Decompress - insize temporary become 0 #368

Closed
avan989 opened this issue Oct 7, 2019 · 3 comments
Labels

Comments

@avan989
Copy link
Contributor

avan989 commented Oct 7, 2019

Is your feature request related to a problem? Please describe.
Test cases where insize might temporarily hit 0 during a normal decompression. reference issue 298.

Requester Info
Anh Van, NASA Goddard.

@skliper
Copy link
Contributor

skliper commented Oct 8, 2019

From #298:

Recommend investigation if insize might temporarily hit 0 during a normal decompression.

Need to analyze code if this could ever happen. Since bug fix in #367 causes this to now return an error.

@jphickey
Copy link
Contributor

jphickey commented Oct 8, 2019

I have reviewed the code and I'm fairly convinced that FS_gz_fill_inbuf is only ever called from the NEXTBYTE() macro, which is only invoked where it has already determined that it needs more data (usually as part of the NEEDBITS() macro).

Therefore I think it is OK to assume that if insize is 0 after the read loop, that it constitutes an error.

NOTE:

In my opinion this is more evidence that we need to move away from the embedded decompression routine, and externalize/modularize it entirely (see #291).

The code in this module is overall very poor/obfuscated and generally doesn't meet FSW coding standards. It's been grandfathered in for years but it is buggy (as evidenced here) and will probably continue to be a source of problems. Furthermore, general purpose data decompression is arguably outside the scope of the CFE, so this code is probably not a wise investment in CCB resources to maintain this code.

Externalizing this algorithm would give missions much more flexibility and efficiency in how they handle data compression and avoid this type of issue.

@skliper
Copy link
Contributor

skliper commented Oct 10, 2019

Closing as-is per @jphickey investigation and the fact its due to be externalized per #291.

@skliper skliper closed this as completed Oct 10, 2019
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

3 participants