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

Fix crash on download from Navin/ZNEX miniHomer #352

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@kgraefe
Copy link

commented May 13, 2019

This adds the options gps-utc-offset and gps-week-rollover to the
miniHomer driver. By adding them solely to the skytraq driver they were
initialized with nullptr for the miniHomer ultimately leading to a
crash.

This fixes #351.

Fix crash on download from Navin/ZNEX miniHomer
This adds the options gps-utc-offset and gps-week-rollover to the
miniHomer driver. By adding them solely to the skytraq driver they were
initialized with nullptr for the miniHomer ultimately leading to a
crash.

Signed-off-by: Konrad Gräfe <konradgraefe@aol.com>
@tsteven4

This comment has been minimized.

Copy link
Collaborator

commented May 13, 2019

Nice.

Did our current skytraq test fail on your architecture? Our basic test is invoked with the testo script. It reads scripts in the testo.d directory, e.g. skytraq.test. I would have hoped the test using skytraq-miniHomer2_8.bin would have been capable of finding this bug, at least if run on ARM.
Without your fix and using your architecture please try "./testo skytraq". Then try it with your fix on your architecture.

If the test fails w/o the fix, and passes with it, then we can blame our finite test environment (various flavors of ubuntu + windows + macos). But if the test doesn't fail w/o the fix, then it would be desirable to enhance the test.

@kgraefe

This comment has been minimized.

Copy link
Author

commented May 13, 2019

The test works well without the patch, but from what I see that's because it by design uses arglist_t skytraq_fargs[] instead of arglist_t miniHomer_args[] and that already had both options.

I don't see an easy way to test for that particular issue.

@tsteven4

This comment has been minimized.

Copy link
Collaborator

commented May 14, 2019

Either
char* p = nullptr; int i = atoi(p);
or
char* p = nullptr; int i = (int) strtol(p, (char **)NULL, 10);
will segfault on Ubuntu bionic. This has nothing to do with ARM and everything to do with de-referencing a null pointer.

ubsan can find these if we execute the code:
test.cc:7:22: runtime error: null pointer passed as argument 1, which is declared to never be null
/usr/include/stdlib.h:178:14: note: nonnull attribute specified here
UndefinedBehaviorSanitizer:DEADLYSIGNAL
==13709==ERROR: UndefinedBehaviorSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f7a5da1b1b0 bp 0x000000000000 sp 0x7ffee04dcde0 T13709)

but, as @kgraefe points out, we only execute the problematic code when we actually have the serial device connected.

I don't see an easy way to improve the test. One might consider:

  • Some formats use gbser_is_serial instead of duplicating the argument vectors, e.g. magproto.

  • converting argument passing to use QStrings.

but these are longer term projects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.