Fix Open/CloseProxmark issues in PR#607

[micolous]

Since #463 has been closed with #607 billed as it's replacement, and it doesn't fully address my concerns, this is a new PR to fix issues that impact making Android support work from Proxmark master:

• StartProxmark split from OpenProxmark, to make it easier to start up the USB communication thread and set the serial port, without also having PM3 try to open the serial port for us. This is useful when PM3 doesn't have a path to a real device, such as with tests with a simulated device, and on Android).

• OpenProxmark now doesn't mutate global state until it has successfully finished.

• CloseProxmark now puts PM3 in offline mode, and clears global state.

• CloseProxmark now checks for a non-null serial_port before calling uart_close, to avoid unintentional double-free'ing serial_port. This fixes a crash-on-exit on OSX.

Things that are not required or used for Android support, but that are related and included in this PR:

• main now calls CloseProxmark once. This fixes a crash-on-exit on OSX.

• OpenProxmark now takes a char* argument again, because CloseProxmark treats serial_port_name as a char*.

As I've mentioned before, I'm not looking to replace comms.c on Android, and this was never my intent when first submitting the file. Several pieces of important global state (such as sp, offline, and uart_communication) are stored in this file.

At present, CloseProxmark still blindly calls unlink on Android, because #ifdef _LINUX_ still triggers on Android, and this is the only way to shut down the USB communication thread.

[pwpiwi]

Hi @micolous, can we please split this up into fixes and "library API" changes?

Agreed to commit these:

• OpenProxmark now doesn't mutate global state until it has successfully finished.
• CloseProxmark now puts PM3 in offline mode, and clears global state.
• CloseProxmark now checks for a non-null serial_port before calling uart_close, to avoid unintentional double-free'ing serial_port. This fixes a crash-on-exit on OSX.
• main now calls CloseProxmark once. This fixes a crash-on-exit on OSX.

(although I wouldn't call it "global state" - the state is not visible outside of comms.c except via calling IsOffline() )

However, I would like to discuss these first:

StartProxmark split from OpenProxmark, to make it easier to start up the USB communication thread and set the serial port, without also having PM3 try to open the serial port for us. This is useful when PM3 doesn't have a path to a real device, such as with tests with a simulated device, and on Android).

This would add a function to the comms API which requires a serial_port as parameter - an OS and device dependent structure which I think should remain hidden inside comms.c. Isn't there a way to start the thread via OpenProxmark(*whatever_type_is_used_on_Android_for_a_commdevice_or_simulated_device)?

As I've mentioned before, I'm not looking to replace comms.c on Android, and this was never my intent when first submitting the file. Several pieces of important global state (such as sp, offline, and uart_communication) are stored in this file.

If you don't want to replace it, then let's try to modify it with some #ifdef __ANDROID__. You may use another type for sp, you may replace the uart_ calls, and OpenProxmark() can be modified to use Android devices and simulated devices.

At present, CloseProxmark still blindly calls unlink on Android, because #ifdef LINUX still triggers on Android, and this is the only way to shut down the USB communication thread.

Maybe adding && !defined(__ANDROID__) would help? I am not sure if this unlink is still required. I think it is/was there to avoid uart_open() to succeed when the Proxmark is rebooted and temporarily not connected during flashing. I would agree with you that deleting a /dev/* file is quite crude and should be avoided.

[micolous]

can we please split this up into fixes and "library API" changes?

Double-free issues are now in #617. I'll rebase on that once that's merged.

This would add a function to the comms API which requires a serial_port as parameter - an OS and device dependent structure which I think should remain hidden inside comms.c. Isn't there a way to start the thread via OpenProxmark(*whatever_type_is_used_on_Android_for_a_commdevice_or_simulated_device)?

But serial_port is already a void* as far as comms.c is concerned -- an opaque pointer. PM3's uart_open and uart_close even take care of memory allocation. comms.c shouldn't be concerned with what that implementation actually is, as long as there's uart_* functions with the right signature.

My issue is that AndProx's signature for uart_open is different, and so can't get away with a char*.

I mean, we could have uart_* be a struct with a function table... this would allow you to have multiple implementations of uart_* in one binary. And then have some function like uart_has_device_path which makes comms.c treat uart_open's argument as a void*. That would be useful if we ever decide to offer a different USB mode.

I think PM3-internal tests should be their own special binary, and then have a uart implementation that allows some expect-like script.

I presently use the same PM3 binary for simulated and real devices, and use a Java mocking library to provide a different interface, because there's not much point in implementing uart_ more than once in JNI.

I'll probably eventually switch to using Java interfaces to fix some other issue with rooted devices, so even my JNI code will have an opaque jobject and a bunch of method IDs. ;)

If you don't want to replace it, then let's try to modify it with some #ifdef ANDROID. You may use another type for sp, you may replace the uart_ calls, and OpenProxmark() can be modified to use Android devices and simulated devices.

We could do something like #506, and have a define like EXTERNALISE_CONNECTIONS to prevent comms.c from calling uart_open and uart_close, and changes the signature of OpenProxmark to take an opaque pointer.

But I liked just splitting up the function instead, between "bit that is important for PM3 CLI tools" and "bit that is important for using PM3-client as a protocol".

I already replaced uart_* (except uart_open) on Android, they just do JNI calls, and I never build PM3's uart_*.c files. With the exception of OpenProxmark, comms.c seems to be fine treating sp as an opaque pointer.

[micolous]

Rebased on current master (with #617), please take a look.

[pwpiwi]

I already replaced uart_* (except uart_open) on Android

Please replace uart_open() as well. This is the correct way to implement different USB communications (see uart/README.md). There should be no need to fiddle with OpenProxmark() which should be independent on hardware and OS.

If you find a way to move the weird unlink() into uart_* without preventing upcoming reconnect , this would be welcome. Otherwise just add a #ifndef ANDPROX and #endifaround the unlink() for the time being.

[pwpiwi]

btw: char* portname is good. This should be the "human readable" description of the port to open, no matter what OS or hardware.

[micolous]

I wrote uart/README.md... 😅 That applies in the case that you're launching PM3 client from the command line. But I'm not launching through the command line, so my entry point is different.

There's another alternative (for me)... with #607, the unlink() I was concerned about is wrapped in a NULL-check. Calling OpenProxmark(NULL, ...) would mean unlink(serial_port_name) is never executed.

But it means stashing the parameters for uart_open_android in another global variable for uart_open to pick up later, which would effectively result in duplication of the internal sp variable. There's also the risk that if there's an error check later added to OpenProxmark to assert that port_name != NULL, the code would fail.

What I want is a way to inject a value into sp, and start the communication thread. Is there a better way to achieve that, without resorting to adding a codepath that mainline PM3 doesn't use, or supplying a patched version of this file?

[pwpiwi]

You would simply call e.g. OpenProxmark("Android USB Port"). Your open_uart() would return whatever struct you need for sp and the communication thread would be started as expected. That's it.

[micolous]

Replaced by #625.