[go: nahoru, domu]

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

Update g2_gribend / g2c_check_msg for rare condition #438

Open
EricEngle-NOAA opened this issue Oct 3, 2023 · 8 comments
Open

Update g2_gribend / g2c_check_msg for rare condition #438

EricEngle-NOAA opened this issue Oct 3, 2023 · 8 comments
Assignees
Labels
bug Something isn't working

Comments

@EricEngle-NOAA
Copy link
Contributor

I came across a rare condition when addressing a grib2io issue. In short, the user was trying to pack data using simple packing, no decimal or binary scaling, and nbits=0. This results in the packing the data to the nearest integer. Just so happens that the last 4 values of the grid rounded to a value of 55, which resolves to "7" in the ASCII table. In this scenario, g2c thinks the GRIB2 message is complete and gives a non-zero return status.

Therefore, I think g2_gribend and/or g2c_check_msg need to check for this rare condition.

Thinking about this a bit, perhaps g2c_check_msg should check the length of section 7 and if the last 4 bytes are "7777" (assuming section length is correct), then to not trigger the error.

@edwardhartnett edwardhartnett added the bug Something isn't working label Jan 30, 2024
@edwardhartnett
Copy link
Contributor

Maybe we should just cut this code:

/* Check to see if GRIB message is already complete. */
if (cgrib[*lencurr - 4] == seven && cgrib[*lencurr - 3] == seven &&
    cgrib[*lencurr - 2] == seven && cgrib[*lencurr - 1] == seven)
{
    if (verbose)
        printf("GRIB message already complete.  Cannot add new section.\n");
    return G2C_EMSGCOMPLETE;
}

@EricEngle-NOAA
Copy link
Contributor Author

I think the above is needed. I am looking at g2_gribend. With some additional logic, we can check for this. I can work on this.

@edwardhartnett
Copy link
Contributor

If you have a solution, that would be great. I didn't see an easy way to do that...

EricEngle-NOAA added a commit to EricEngle-NOAA/NCEPLIBS-g2c that referenced this issue Jan 31, 2024
This commit fixes a rare condition in which the last 4 bytes of a packed
section 7 could equal "7777" which is the GRIB end of message
terminating string and triggers an erroneous end of message status.

The added logic checks for "7777" at the end of cgrib under the
condition that we have a valid GRIB message through seciton 7.

This commit references NOAA-EMC#438
@EricEngle-NOAA
Copy link
Contributor Author

Lets table this for v1.10

@edwardhartnett
Copy link
Contributor

The next version (v2.0.0) will be released soon. Should this be bumped to the next release?

@EricEngle-NOAA
Copy link
Contributor Author
EricEngle-NOAA commented Jun 18, 2024

I have talked about this rare condition on a few occasions with different groups of people. Someone suggested that if the last 4 bytes of the packed data are [55, 55, 55, 55] (i.e. "7777"), then just make the last byte either 54 or 56. In theory, this byte would only impact a corner of the grid.

Thoughts?

@edwardhartnett
Copy link
Contributor

Don't we know the size of the section anyway? In other words do we need to scan for 7777?

@EricEngle-NOAA
Copy link
Contributor Author

Continuing to look at this... g2_gribend is performing checking actions after the call to g2c_check_msg. So perhaps, we should move some of the checking logic from g2_gribend into g2c_check_msg -- mainly this section:

    /*  Loop through all current sections of the GRIB message to find
     *  the last section number. */
    len = 16;    /* Length of Section 0. */
    for (;;)
    {
        /*    Get number and length of next section. */
        iofst = len * 8;
        gbit(cgrib, &ilen, iofst, 32);
        iofst = iofst + 32;
        gbit(cgrib, &isecnum, iofst, 8);
        len = len + ilen;

        /*    Exit loop if last section reached. */
        if (len == lencurr)
            break;

        /*    If byte count for each section doesn't match current
         *    total length, then there is a problem. */
        if (len > lencurr)
        {
            printf("g2_gribend: Section byte counts don''t add to total.\n");
            printf("g2_gribend: Sum of section byte counts  =  %d\n", (int)len);
            printf("g2_gribend: Total byte count in Section 0 = %d\n", (int)lencurr);
            return G2_BAD_SEC_COUNTS;
        }
    }

If you look at g2c_check_msg, it is only looking at the first and last 4 bytes, but does not check section order and lengths. IMO it should.

And perhaps it should conditionally check for the end section? The modified workflow in g2_gribend would then be:

...input cgrib has all sections and ready to add end...
call g2c_check_msg (conditional arg to not check for end)
if OK
   add end section
   check again (conditional arg to now check for end)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: To do
Development

No branches or pull requests

2 participants