USB comms: part 4 towards @micolous PR #463

[pwpiwi]

• make uart_communication() static in comms.c
• move receiver thread creation and respective mutexes to comms.c
• add mutex and signal for tx buffer
• use comms.c for flasher as well
• remove comm functions from client/proxmark3.h
• this completes isolating all USB communication related functions in comms.c

[pwpiwi]

@micolous: you will find most changes familiar because your PR #463 was the starting point. However, I moved even more stuff into comms.c and solved the flasher timing regression differently. From my perspective this was just a refactoring. Please have a look if this fulfills the requirements for your Android port as well.

[micolous]

OK, let me try to point the Android code at this branch and see how cleanly this goes. Thanks for pushing this forward!

[micolous]

I wrote a patch which makes AndProx use this different API as an experiment. I also wrote some test cases with Mockito to build a virtual PM3 device which can be served from Java-land.

I notice that OpenProxmark is there as a convenience function. While this is nice (it simplifies some of my API calls), it assumes that the serial port is a string. So I have worked around this a little:

bool OpenProxmarkAndroid(JNIEnv* env, JavaVM* vm, jobject nsw) {
    serial_port* sp = uart_open_android(env, vm, nsw);
    return OpenProxmark((char *)sp, /* waitCOMPort */ false, /* timeout */ 0, /* flash_mode */ false);
}

serial_port uart_open(const char* pcPortName) {
    return (serial_port)pcPortName;
}

This workaround leaves pointers to my serial_port_android struct in both sp and serial_port_name, which is workable, but all that pointer casting worries me.

I still need to pass multiple Java object references into uart_open_android. I can't pass a string here, because Android doesn't let you connect to devices by path.

However when CloseProxmark is called, it will call uart_close (wherein I free all the pointers I made in uart_open_android), and then it will unlink that for me (because Android has that function). At this point, serial_port_name is memory that I just free'd, unlink will try to read from that address, and have a use-after-free. :\

Even if I did pass around references to USB device paths, this would run afoul of SELinux policies on most Android devices. I'd also be concerned about actually breaking something on rooted devices.

I worked around this and got a working build with your branch by removing the unlink call in CloseProxmark.

If that unlink call is really needed, then I think would sort this is "yet another" parameter to OpenProxmark, which will make it treat that path as a pointer to a serial_port struct, set serial_port_name = NULL, then CloseProxmark can wrap that unlink call in an if (serial_port_name != NULL).

This would be useful on non-Android platforms as well, because then tests can be written that simulate PM3 hardware interaction.

[pwpiwi]

The general idea was to have a comms.c for each Operating System (or use #ifdef). This means that you would implement your own Android specific versions of OpenProxmark(), CloseProxmark(), etc. (everything defined in comms.h). If you don't need the unlink in your version of CloseProxmark() then don't implement it. If you don't need a serial_port_name then don't implement it. It is all static in comms.c now and therefore up to you or anyone else who wants to implement another communication to the device.

You made a good point regarding portname in OpenProxmakr(). I propose that I change bool OpenProxmark(char *portname, bool waitCOMPort, int timeout, bool flash_mode) to OpenProxmark(void *port, bool waitCOMPort, int timeout, bool flash_mode). You can then use a pointer to whatever structure you require (probably a combination of JNIEnv* env, JavaVM* vm, jobject nsw) in your Android version of OpenProxmark() without the casts.

[micolous]

I never intended for comms.c to be OS specific, and I still actually build that file in the Android version. For reference, CMakeLists.txt shows which things I'm building on Android.

The intent of the changes in the first place was to pull all the communication related functionality away from proxmark3.c, so that I didn't need to include that file. That's also why there were things like SetSerialPort, and I didn't move the full startup process into comms.c.

Having the full startup process there is better than what I had.

Also, uart_open_android already constructs a struct with the arguments it got from JNI in there:

serial_port uart_open_android(JNIEnv* env, JavaVM* vm, jobject nsw)
{
    serial_port_android* sp = malloc(sizeof(serial_port_android));
    sp->javaVM = vm;
    sp->nativeSerialWrapper = (*env)->NewGlobalRef(env, nsw);
    return sp;
}

It already has a working connection at this point, and there's very little that needs to be done from the C side of things aside from getting an exclusive reference for it. There's no "setup" at all. ;)

I'd really like an OpenProxmark that I can just stuff a serial_port* into, have PM3 assume it is OK, and then not do the unlink on. I would pull that onto my side of things, but very little of the global state is exposed.

I wrote PR #611 which should address an issue with uart_ methods having incorrect signatures. From there, apply your patch, but:

https://github.com/pwpiwi/proxmark3/blob/823eadfa5b982bc6346ec1f8b8852593bcf7daef/client/comms.c#L27-L29

Initialise these explicitly to NULL.

https://github.com/pwpiwi/proxmark3/blob/823eadfa5b982bc6346ec1f8b8852593bcf7daef/client/comms.c#L301

Make this assign a local variable instead.

https://github.com/pwpiwi/proxmark3/blob/823eadfa5b982bc6346ec1f8b8852593bcf7daef/client/comms.c#L325-L328

Move those into a new method, StartProxmark, and pass in the local serial_port* variable from OpenProxmark. Assign serial_port_name (line 329) before you call StartProxmark.

https://github.com/pwpiwi/proxmark3/blob/823eadfa5b982bc6346ec1f8b8852593bcf7daef/client/comms.c#L339-L340

Wrap this in if (serial_port_name == NULL) { ....

And then at the end of the function (out of the if-block), add:

serial_port_name = NULL;
sp = NULL;

That should be sufficient to decouple the serial port from being an actual file.

I think we should still figure out why unlink was needed, because there is probably a better way to do that. Ideally, it should move into uart_posix.c, and then I don't need to care about it.

[pwpiwi]

Partly included your proposals into last commit. From here I would add reconnections and try to eliminate SetOffline() (this should not be needed outside comms.c)

[micolous]

Yup, ignore #611, I'll have a look over this when I've got a clearer head. Thanks!

[micolous]

That latest version addresses the unlink issue for non-Linux POSIX platforms, and the intent of my other PR about the NULLs. However, it still assumes path is a string on Linux (because Android still trips those defines).

I see why you want to put this function here -- it is communication code that is common to PM3 usages.

My intent here is not to burden PM3 with Android-specific code, but to give it some nicely shaped interfaces that it can fit in to. I should be able to ship an Android client with approximately 0 modifications to PM3's core code, and PM3 should be able to ship with no Android-specific code.

comms.c with this PR very nearly lets me do this! :)

What I think would be best is if I write a patch based on this one that incorporates the change that I'm actually looking for.

[micolous]

micolous@4d59ec2 contains my extra suggested changes (branch: pwpiwi-607-fix).

• StartProxmark split from OpenProxmark, to make it easier to start up the USB communication thread and set the serial port, without also have PM3 try to open the serial port for us.
• OpenProxmark now doesn't mutate global state until it has 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.
• main now calls CloseProxmark once.

The double free surfaced on OSX on exit as:

proxmark3(73181,0x7fffb74de380) malloc: *** error for object 0x100a020e0: pointer being freed was not allocated

PS: I also changed OpenProxmark back to taking a char* argument, because that properly reflects the semantics of serial_port_name. If StartProxmark takes a serial_port and doesn't touch serial_port_name, then it doesn't really matter what OpenProxmark does.