Skip to content

FIX: crash on Bionic libc if CloseProxmark is called twice.#672

Merged
pwpiwi merged 1 commit intoProxmark:masterfrom
micolous:bionic-pthread-crash
Oct 6, 2018
Merged

FIX: crash on Bionic libc if CloseProxmark is called twice.#672
pwpiwi merged 1 commit intoProxmark:masterfrom
micolous:bionic-pthread-crash

Conversation

@micolous
Copy link
Contributor

@micolous micolous commented Sep 15, 2018

In Android O and later, if an invalid (or defunct) pthread_t is passed to pthread_join, it calls fatal():

https://github.com/aosp-mirror/platform_bionic/blob/ed16b344e75f422fb36fbfd91fb30de339475880/libc/bionic/pthread_internal.cpp#L116-L128

This patch addresses it by:

  1. Always memset(0) on USB_communications_thread at the end of CloseProxmark.

  2. On Bionic, only call pthread_join on USB_communications_thread if it is not equal to 0.

This addresses AndProx/AndProx#23, an issue similar to #617.

Tested:

  • Android SDK 21
  • Android SDK 27
  • Linux x86_64

In Android O and later, if an invalid pthread_t is passed to pthread_join,
it calls fatal().

https://github.com/aosp-mirror/platform_bionic/blob/ed16b344e75f422fb36fbfd91fb30de339475880/libc/bionic/pthread_internal.cpp#L116-L128

This patch addresses it by:

1. Always memset(0) on USB_communications_thread at the end of
   CloseProxmark.

2. On Bionic, only call pthread_join on USB_communications_thread if it is
   not equal to 0.
RfidResearchGroup pushed a commit to RfidResearchGroup/proxmark3 that referenced this pull request Sep 15, 2018
@pwpiwi
Copy link
Contributor

pwpiwi commented Sep 15, 2018

Why and when would this be needed? CloseProxmark() is only called once before exiting the program.

@micolous
Copy link
Contributor Author

Because comms.c is in charge of the management of USB_communications_thread (which is private to other modules).

This is nearly the same issue as what was fixed in #617, where defence in depth was added to CloseProxmark to mitigate against further issues (in addition to fixing the double call).

It's not possible for a caller to know with absolute certainty the state of USB_communications_thread, other than keeping track of success of OpenProxmark and hoping nothing has come in and changed your state under you (eg: proposed reconnect logic).

Copy link
Contributor

@pwpiwi pwpiwi left a comment

Choose a reason for hiding this comment

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

Would you please surround the memset() with #ifdef as well?

@pwpiwi pwpiwi merged commit 7b2cd97 into Proxmark:master Oct 6, 2018
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