Skip to content

nul-terminate meinberg sw_rev and ascii_msg strings in data_mbg - #30

Open
arib06 wants to merge 1 commit into
ntp-project:stablefrom
arib06:mbg-string-nul-terminate
Open

nul-terminate meinberg sw_rev and ascii_msg strings in data_mbg#30
arib06 wants to merge 1 commit into
ntp-project:stablefrom
arib06:mbg-string-nul-terminate

Conversation

@arib06

@arib06 arib06 commented Jul 10, 2026

Copy link
Copy Markdown

get_mbg_sw_rev and get_mbg_ascii_msg copy the full name[17]/s[23] field out of the serial packet, overwriting the byte the struct reserves as the trailing zero, so the later %s and strlen in refclock_parse read past the field on a non-terminated frame; force the last byte to NUL after each copy.

@arib06

arib06 commented Jul 23, 2026

Copy link
Copy Markdown
Author

any update?

@hart-NTP

Copy link
Copy Markdown
Collaborator

Feedback from Meinberg below. IMO it's belt and suspenders but for a very small audience and protects against a "if it hurts don't do that" situation unlikely to be seen in the real world, so not a bad idea but very low priority.


In fact, the fields mentioned in this report are defined in a way that
they already include a terminating zero.

See the file include/mbg_gps166.h, which contains definitions that were
copied by Frank Kardel from the Meinberg API file gpsdefs.h, which was
originally created in the 1990s, when we built our first own GPS receiver.

The file still exists and is in use for the Meinberg API, see, for example:
https://git.meinbergglobal.com/drivers/mbgtools-lx.git/tree/mbglib/common/gpsdefs.h

A huge number of definitions were added in the mean time, but the
definitions that existed about 30 years ago are still unchanged, and the
firmware of our GPS receivers transmits the data accordingly, with
properly terminated strings.

On the other hand, if the program that reads these data structures from
a device ensures that the strings are indeed properly terminated, it
doesn't hurt and makes sure that no bad things can happen if the
received data has been messed up for some reason.

So IMO it would be good to accept these patches.

@arib06

arib06 commented Jul 29, 2026

Copy link
Copy Markdown
Author

That matches the intent. Valid firmware already terminates these, so the assignment is a no-op on well-formed packets and only does anything on a corrupted or truncated frame, where it stops the later %s and strlen from reading past the field. Cheap guard, no behavior change for good data, so low priority is fair.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants