Fix #298, fix infinite loop when gz file is truncated

[avan989]

Describe the contribution
Fix issue #298, when cfe_fs_decompress goes into infinite loop when gz file is truncated.

Fix #298

Testing performed

1. Build
2. Pass in truncated file as parameter to cfe_fs_decompress.
   corgi2.jpg.gz
3. Verify the return status:

STATUS CODE = -973078518

4. Gzip good file
   

5. Pass as parameter and verify output.

System(s) tested on:

• Hardware
• Ubuntu 18.04.03
• cFE 6.7.0

Contributor Info
Anh Van, NASA Goddard

Community contributors
You must attach a signed CLA (required for acceptance) or reference one already submitted

[jphickey]

Has this been confirmed to correctly handle a NON-truncated file?

[jphickey]

Another general comment, if we are touching this code I would like to see a couple more changes.

The checks of len in the function should all be simplified to be just:

At line 287:

if (len <= 0) break;

And At line 294:

if (State->insize == 0 || len < 0) 
...

Rather than specifically testing for OS_ERROR only. Any negative value constitutes an error, and it will corrupt the state if not caught (i.e. insize could go negative).

Also note that the EOF macro is not defined by OSAL nor defined anywhere else in CFE. It just happens to not trigger a compiler error because it must be getting the value implicitly via stdio.h , and probably has the value -1. It might be clearer/better to just use -1 here.

[skliper]

Preference is to avoid "magic numbers" like -1. Is there an OSAL/CFE define that is appropriate?

[jphickey]

Preference is to avoid "magic numbers" like -1. Is there an OSAL/CFE define that is appropriate?

Not particularly. The function is supposed to return the first character in the buffer, but in this case the buffer is empty, so there is nothing to return.

Another reasonable option might be the NUL char (0). But that just as magic as -1. Returning 0 won't corrupt the shift register that the values go into, whereas -1 does.

Either way though, this decompress code still violates coding standards in every conceivable way. I only mention it because it might cause a compile failure if things change and EOF no longer happens to be defined. Same with the length check (testing <= 0 vs. testing a specific value).

The real fix for this should be done in #291

[skliper]

Either way though, this decompress code still violates coding standards in every conceivable way. I only mention it because it might cause a compile failure if things change and EOF no longer happens to be defined. Same with the length check (testing <= 0 vs. testing a specific value).

Understood. Let's fix the <=0 now. The EOF builds today and the "-1" solution isn't acceptable, so lets just leave as is for now. Or if you really want to, feel free to propose a solution that doesn't include the use of a "magic number" (create an appropriate #define, identify an appropriate one that already exists, or whatever). But I don't think the EOF change is worth our time right now due to #291.

The real fix for this should be done in #291

Agreed, which is why I'm OK with leaving the EOF as-is, but I'd like to get this fix in first.

[skliper]

Since it's all local to cfe_fs_decompress.c, why not just:

#define CFE_FS_EOF -1

or something? and change EOF to CFE_FS_EOF?

[avan989]

update checks of lens. Tested with truncated file and non truncated file. Truncated file return

STATUS CODE = -973078518

Non-truncated file decompress and return images.

[jphickey]

Looks good.