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

Fixed connection hang, segfault, and unexpected warnings issues after internet connection loss. #60

Merged
merged 41 commits into from
Mar 29, 2024

Conversation

adamshapiro0
Copy link
Contributor

@adamshapiro0 adamshapiro0 commented Mar 26, 2024

Changes

  • Updated to the most recent release of BoringSSL (2024/3/25)
  • Treat all remote socket disconnections as unexpected

Fixes

  • Reset the retry count on success to avoid unexpected reauthentication from a single failure after a period of stable connection
  • Don't reauthenticate on socket errors vs data/authentication errors
  • Fixed enabling of trace prints in polaris_client.cc
  • Fixed unexpected "connection terminated" and "no data" warnings on user-requested disconnect
  • Updated "no data" logic to expect RTCM 1029 messages, now sent on authentication failures
  • Check for IPv6 returns from gethostname() (IPv6 not currently supported)
  • Fixed socket error checks and printout on FreeRTOS
  • Fixed possible segfault after an error (internet connection loss, etc.) while awaiting an HTTP authentication response
  • Fixed possible loss of data after a temporary internet outage caused by missed socket timeout error codes
  • Fixed Polaris_Send*() position send function segfault if SSL context is not connected

This counter is intended to detect possible authentication issues (e.g.,
incorrect key). Socket errors, typically some form of "internet connection not
available," should not count against that.
…ng cases.

The Polaris network now sends an RTCM 1029 text message in response to an
authentication failure or similar error. The client no longer receives 0 bytes
when an authentication token is rejected.

This change avoids misleading warnings when authentication is valid but position
is not supplied, etc. We now return POLARIS_CONNECTION_CLOSED when the
connection is closed without an authentication error, including if we do not
receive any data from the network, and only return POLARIS_FORBIDDEN if we
suspect an authentication issue.
The P1_SetAddress() does not currently support IPv6. This may change in the
future.
@adamshapiro0 adamshapiro0 self-assigned this Mar 26, 2024
Otherwise, ASAN needs to be manually loaded via LD_PRELOAD.
… out."

We cannot remove these. Internally, polaris.c accesses them if POLARIS_USE_TLS
is enabled _when polaris.c is compiled_. But if the user application does not
set POLARIS_USE_TLS when they compile and instantiate PolarisContext_s, the
context they create will be smaller than polaris.c is expecting and will cause a
buffer overflow.
@adamshapiro0 adamshapiro0 changed the title Fixed unexpected warnings on disconnect and reauthentication after single connection failure. Fixed connection hang, segfault, and unexpected warnings issues after internet connection loss. Mar 27, 2024
Copy link
Member

@anathan anathan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor stuff but looks good

c/src/point_one/polaris/polaris.c Outdated Show resolved Hide resolved
else if (context->total_bytes_received > 0) {
P1_Print(
"Warning: Polaris connection closed with an error response. Is "
"your authentication token valid?\n");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we print the error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not without adding an RTCM parser. If we hit an errno it will be printed above before we get here, but if we get an error notification from the server it comes as an RTCM 1029 message. We could add functionality to parse it and verify the CRC, but we don't have that in this library currently.

c/src/point_one/polaris/polaris.c Outdated Show resolved Hide resolved
examples/simple_polaris_client.cc Outdated Show resolved Hide resolved
@adamshapiro0 adamshapiro0 merged commit 2271a69 into master Mar 29, 2024
9 checks passed
@adamshapiro0 adamshapiro0 deleted the failed-reconnect branch March 29, 2024 23:35
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