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

Number extraction missing validation. Verified with PGN127489 testcase. #9

Open
asbjorn opened this issue Feb 18, 2022 · 1 comment

Comments

@asbjorn
Copy link
Contributor

asbjorn commented Feb 18, 2022

Hi.

I'm using this library to parse a stream of NMEA2k messages from an NGT-1 device. I really like this library - great work btw 👍 But, periodically I get bad values for some PGN's, especially PGN 127489 (Engine Parameters, Dynamic). I've been trying to debug the source of the problem for some time now, and after managing to extract RAW NMEA2k messages and writing Go tests I was able to pinpoint the problem. The 'maxValue' checking that is commented out in the raw_message.go seems to fix the problem:

// var maxValue uint64

I also got some help by comparing the raw_message.go:extractNumber(..) with the corresponding extractNumberfunction from the canboat project: https://github.com/canboat/canboat/blob/master/analyzer/pgn.c#L253

So my question is why this code block is commented out?

Here is my Go tests which can reproduce the problem raw_message_test.go:

func TestPgn127489NumberExtraction(t *testing.T) {
	var msg = RawMessage{new(can.RawMessage)}
	msg.Data = []byte{0x00, 0xe8, 0x08, 0xff, 0xff, 0x73, 0x7d, 0x31, 0x0b, 0x0c, 0x00, 0x80, 0x99, 0x56, 0x04, 0xff, 0xff, 0x60, 0x02, 0xff, 0x00, 0x00, 0x00, 0x00, 0x09, 0x03}
	msg.Pgn = uint32(127489)

	pgnParsed := ParsePacket(msg.RawMessage)
	_, ok := pgnParsed.Data[6].(uint64)
	if !ok {
		t.Errorf("unable to extract 'total engine hours' from valid RawMessage: %+v\n", pgnParsed.Data[6])
	}

	// This RawMessage is the 'BAD' one with invalid values. Field 6 should be 'nil' with the proper validation.
	msg = RawMessage{new(can.RawMessage)}
	msg.Data = []byte{0x00, 0xe8, 0x08, 0xff, 0xff, 0x73, 0x7d, 0x31, 0x0b, 0x0f, 0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x60, 0x02, 0xff, 0x00, 0x00, 0x00, 0x00, 0x0b, 0x04}
	msg.Pgn = uint32(127489)

	pgnParsed = ParsePacket(msg.RawMessage)
	engineHours, ok := pgnParsed.Data[6].(uint64)
	if ok {
		t.Errorf("the value for the field 'total engine hours' is invalid: (%d). Need to check maximum value during extraction. RawMessage: %+v\n", engineHours, pgnParsed.Data[6])
	}
}

By uncommenting the maxValue validation checking in raw_message.go:extractNumber(..) this test case works.

@asbjorn
Copy link
Contributor Author

asbjorn commented Oct 31, 2022

@timmathews Did you had the chance to review this PR?

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

No branches or pull requests

1 participant