fix: Add talker id to nmea struct (#89)

[tazounet]

Hello,

Changes from #89 introduce a bug. The "type" is no more a null terminated string and the "strcmp" is failing in "minmea_sentence_id".
The correction use "struct minmea_type type" and a "memcmp" like the modification done in "minmea_parse_xxx".

[kosma]

Oops. The type should really be a null-terminated string to prevent more bugs cropping up, and make it easier for the user.

[hraftery]

Good catch! Your correction is exactly as required.

However, @kosma has a point - the root cause here is that the type of xxx in minmea_scan(sentence, "t", xxx) has now changed.

It's similar in effect and implementation to sscanf(string, "%t", xxx), if the type expected by %t were to change. Because these are varargs it's difficult to type check (I think).

There could be other users of minmea_scan that will face a similar issue.

Given that the type changed from char buf[6] to struct { char talker_id[2]; char sentence_id[3]; };, they are very nearly compatible. Using one instead of the other only differs in that the former has room for a null terminator (and is expected to have one). The other salient difference is that the latter is stored with the sentence struct, so the sentence structs all grew by 5 bytes.

So on balance, I think I'd prefer to see the struct change, and grow by a byte, to be backwards compatible. The fix in this PR can remain as an example of proper use, but at least then if there's code out there that doesn't change, it will still work. The struct might become:

struct minmea_type {
    char talker_id[2];
    char sentence_id[3];
    char null_terminator;
};

or even (taking advantage of anonymous structs in C11):

union minmea_type {
  char buf[6];
  struct {
    char talker_id[2];
    char sentence_id[3];
    char null_terminator;
  }
};

That way, sizeof(type.sentence_id) still works as intended, but the whole thing is backwards compatible with char buf[6] and treatment as a null terminated string.

[chmorgan]

Looks like a solid approach to me as long as the destination length is being properly checked when the values or memory is copied into the struct. Good idea for backwards compatibility and ease of parsing.

…

On Fri, Aug 1, 2025 at 7:51 PM Heath Raftery ***@***.***> wrote: *hraftery* left a comment (kosma/minmea#90) <#90 (comment)> Good catch! Your correction is exactly as required. However, @kosma <https://github.com/kosma> has a point - the root cause here is that the type of xxx in minmea_scan(sentence, "t", xxx) has now changed. It's similar in effect and implementation to sscanf(string, "%t", xxx), if the type expected by %t were to change. Because these are varargs it's difficult to type check (I think). There could be other users of minmea_scan that will face a similar issue. Given that the type changed from char buf[6] to struct { char talker_id[2]; char sentence_id[3]; };, they are very nearly compatible. Using one instead of the other only differs in that the former has room for a null terminator (and is expected to have one). The other salient difference is that the latter is stored with the sentence struct, so the sentence structs all grew by 5 bytes. So on balance, I think I'd prefer to see the struct change, and grow by a byte, to be backwards compatible. The fix in this PR can remain as an example of proper use, but at least then if there's code out there that *doesn't* change, it will still work. The struct might become: struct minmea_type { char talker_id[2]; char sentence_id[3]; char null_terminator; }; or even (taking advantage of anonymous structs in C11): union minmea_type { char buf[6]; struct { char talker_id[2]; char sentence_id[3]; char null_terminator; } }; That way, sizeof(type.sentence_id) still works as intended, but the whole thing is backwards compatible with char buf[6] and treatment as a null terminated string. — Reply to this email directly, view it on GitHub <#90 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJH4ADDAITIJD7VKXOO7DD3LP4QDAVCNFSM6AAAAACC46VQRKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTCNBWGAZDENJWGE> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[tazounet]

ok, I will update the PR using an union :)

[chmorgan]

@tazounet you mentioned that strcmp was failing in minmea_sentence_id but the tests are passing. If the tests aren't catching this condition can you add a test that should fail as either the first commit or a separate PR, and then it will be apparent that these changes will fix the test.

[chmorgan]

@tazounet otherwise the code changes look good, just want to improve the tests as it would seem they should have caught this bug in the library.

[tazounet]

@tazounet you mentioned that strcmp was failing in minmea_sentence_id but the tests are passing. If the tests aren't catching this condition can you add a test that should fail as either the first commit or a separate PR, and then it will be apparent that these changes will fix the test.

I looked at the tests and the function is correctly tested. I think the bug depend on the compiler or the libc version (I'm running on a rpi pico). But the code is wrong (you should not call strcmp on a string not null terminated).

[hraftery]

the function is correctly tested. I think the bug depend on the compiler or the libc version

That doesn't sound right. strcmp() fails pretty reliably if one of the strings is not null-terminated. See for example: https://godbolt.org/z/YMno13aGo

#include <stdio.h>
#include <string.h>

int main(int argc, char argv[])
{
    char buf1[4];
    char buf2[4];

    buf1[0] = buf2[0] = 'a';
    buf1[1] = buf2[1] = 'b';
    buf1[2] = buf2[2] = 'c';
    buf1[3] = buf2[3] = '\0';

    buf2[3] = 'd';

    printf("%d\n", strcmp(buf1, buf2));

    return 0;
}

[hraftery]

I've only had a very quick look, but it seems:

1. the project uses libcheck for unit testing
2. minmea_sentence_id() is only checked in test_minmea_usage1.
3. the buffer in question is local to minmea_sentence_id().

I don't know if libcheck has any options to initialise stack variables to catch things like this. You could try manually initialising the (future) stack prior to the call in test_minmea_usage1, but it might be difficult to make that cross-platform. Alternatively you could create a test dedicated to this purpose, where you create a badly initialised buffer yourself with something like char type[] = "123456"; before calling minmea_scan(sentence, "t", type).

[chmorgan]

@hraftery I looked at adding a test for this, and looked in test_minmea_scan_t and it was unclear how to implement. Would you be able to implement it in a way that it will fail without the fixes here?

[chmorgan]

@hraftery ping?

[hraftery]

My interest is primarily as a user. I'll attempt to get a dev environment setup this week, but can't guarantee a turn around time. I'm also not sure which of the options I suggested here would be best, but if it was me I'd probably take the easy (last option) way out.

[chmorgan]

My interest is primarily as a user. I'll attempt to get a dev environment setup this week, but can't guarantee a turn around time. I'm also not sure which of the options I suggested here would be best, but if it was me I'd probably take the easy (last option) way out.

You had helpfully pointed to an approach that should reproduce the issue but due to my lack of experience with the library I wasn’t able to figure out where to put it. Trying to close holes that could end up being a future cve :-) even a code snippet and a pointer to where it might go and I could open the pr and test here. Today check isn’t catching anything so I suspect there isn’t a test for it.

[hraftery]

I stole some time today and got a dev environment going. Was a big lift trying to figure out how to reliably run tests midway through someone else's PR. Anyway, I've had a crack. There's only a few lines to it. The heft is all in inserting it into the PR! The result is (oddly, perhaps) in a(nother) PR on @tazounet 's original branch.

tazounet#1

[tazounet]

Ok, I merged your PR on my branch and the PR (here) is ok (I see your new test "test_minmea_scan_t_str")

[chmorgan]

@hraftery looks like your tests work!!

This is without the structure changes (ex. without the fix)

(base) cmorgan@ubuntu-vm:~/projects/minmea/build$ make test
Running tests...
Test project /home/cmorgan/projects/minmea/build
    Start 1: tests
1/2 Test #1: tests ............................***Failed    0.01 sec
Running suite(s): minmea
97%: Checks: 37, Failures: 1, Errors: 0
/home/cmorgan/projects/minmea/tests.c:306:F:minmea_scan:test_minmea_scan_t_str:0: Assertion 'strcmp(type, "GPRMC") == 0' failed

    Start 2: clang_static_analysis
2/2 Test #2: clang_static_analysis ............   Passed    0.06 sec

50% tests passed, 1 tests failed out of 2

Total Test time (real) =   0.07 sec

The following tests FAILED:
	  1 - tests (Failed)
Errors while running CTest
make: *** [Makefile:71: test] Error 8

and with the fix:

(base) cmorgan@ubuntu-vm:~/projects/minmea/build$ make test
Running tests...
Test project /home/cmorgan/projects/minmea/build
    Start 1: tests
1/2 Test #1: tests ............................   Passed    0.01 sec
    Start 2: clang_static_analysis
2/2 Test #2: clang_static_analysis ............   Passed    0.06 sec

100% tests passed, 0 tests failed out of 2

Total Test time (real) =   0.07 sec

[chmorgan]

Merged with another PR, thanks for the contributions @tazounet, @hraftery