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.

Sign up for GitHub

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

First try at disconnect/reconnect detection #516

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

First try at disconnect/reconnect detection #516

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
96aab06
First try at disconnect/reconnect detection. No idea if it's going t…
Dec 11, 2017
2a3f94d
Fixed function name
Dec 11, 2017
1ab4fc9
Removed the retrieving of version information on re-connect so we don…
Feb 17, 2018
6e0b2d0
Removed automatic version cache clearing, added [nocache] flag to "hw…
Feb 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion client/cmdhw.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,11 +405,19 @@ int CmdTune(const char *Cmd)

int CmdVersion(const char *Cmd)
{
return CmdVersionC(Cmd, false);
}

clearCommandBuffer();
int CmdVersionC(const char *Cmd, const bool no_cache)
{
UsbCommand c = {CMD_VERSION};
static UsbCommand resp = {0, {0, 0, 0}};

if( no_cache )
memset( &resp, 0, sizeof(resp) );
else
clearCommandBuffer();

if (resp.arg[0] == 0 && resp.arg[1] == 0) { // no cached information available
SendCommand(&c);
if (WaitForResponseTimeout(CMD_ACK,&resp,1000)) {
Expand Down
1 change: 1 addition & 0 deletions client/cmdhw.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,5 +23,6 @@ int CmdSetDivisor(const char *Cmd);
int CmdSetMux(const char *Cmd);
int CmdTune(const char *Cmd);
int CmdVersion(const char *Cmd);
int CmdVersionC(const char *Cmd, const bool no_cache);

#endif
61 changes: 55 additions & 6 deletions client/proxmark3.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
pthread_mutex_t print_lock;

static serial_port sp;
static char* sp_name;
static UsbCommand txcmd;
volatile static bool txcmd_pending = false;

Expand Down Expand Up @@ -70,16 +71,63 @@ byte_t* prx = rx;
static void *uart_receiver(void *targ) {
Copy link
Contributor

@micolous micolous Feb 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Hey, can you please hold up on this until #463 is in? Aside from not wanting to have to go through a merge conflict process again, I address some of the reconnection issues already in that for flasher.

struct receiver_arg *arg = (struct receiver_arg*)targ;
size_t rxlen;
bool need_reconnect = false;
UsbCommand version_cmd = {CMD_VERSION};
static bool request_version = false;

while (arg->run) {
rxlen = 0;
if (uart_receive(sp, prx, sizeof(UsbCommand) - (prx-rx), &rxlen) && rxlen) {
prx += rxlen;
if (prx-rx < sizeof(UsbCommand)) {
if( need_reconnect ) {
sp = uart_open(sp_name);
Copy link
Contributor

@micolous micolous Feb 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This will break on Android. Is there a better way to pass this from within the main() function instead?

See https://github.com/AndProx/AndProx/blob/b662c653d7ba33b570d71e34c949e3a8e7f17d8e/natives/src/main/cpp/natives.c#L82

Currently, this assumes that the "serial port identifier will always be a string". This isn't true if uart_open is replaced.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • For each of uart_win32.c and uart_posix.c, add a char* referring to the serial port path in their platform-specific serial_port structs.

  • In uart.h, add a method bool uart_reconnect(serial_port* sp).

  • uart_reconnect can then be implemented in platform-specific ways, that takes in the serial port identifier stashed away earlier, and will setup the existing serial_port struct to work again. This returns true on success, or false on error.

  • Whenever the application determines the need to reconnect, it calls uart_reconnect, which can operate in-place. uart_receiver can have some retry logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no idea what part of the above code will fail on Android, that link doesn't explain it. The original PM3 code takes main()'s argv[1] and feeds it to uart_open(). sp_name is just a copy of argv[1]. In what cases will argv[1] not be a char* ? If main() is changed to not feed argv[1] into uart_open then obviously the other instances of uart_open() will need to be changed too. Surely replacing "char*" with whatever becomes appropriate at that time isn't that difficult...

The entire point of this patch is to simply detect a disconnection and attempt to reopen the same port. It is not to rewrite the entire uart system.

Copy link
Contributor

Choose a reason for hiding this comment

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

All of it will fail on Android, because the "serial port" is a reference to a bunch of JNI object IDs, which allow it to pass uart_send and uart_receive to Java method calls.

This is implemented in these files:

PM3 doesn't get called by main either, so there is no argv. The link I sent is one of the bootstrap functions which sets up PM3 similar to how main would.

This is done because there is no /dev/ttyACM0 except on rooted devices where one has installed the cdc_acm kernel module. Everything is passed around the Android USB Host API. Requiring root is a major blocker to use on the platform.

Having an API call for "reconnect" means that it doesn't matter how uart is implemented, whether it's a string pointing to a device name, it's wrapped up in some other platform specific code, or it's part of some testing harness.

At least on Android I'd bring "reconnect" up into Java space, hit the USB stack on the head a few times, and then pass it back.

As for precedent: @iceman1001 objected when I proposed removing uart_{get,set}_speed even though that's not actually used in mainline PM3, but was in his branch.

If that went in as-is, even when it was "internal", I would no longer be able to track upstream PM3 for Android until it was replaced with the API extensions I proposed.


if( sp == INVALID_SERIAL_PORT || sp == CLAIMED_SERIAL_PORT )
{
PrintAndLog("Reconnect failed, retrying...");

if( txcmd_pending ) {
PrintAndLog("Cannot send bytes to offline proxmark");
txcmd_pending = false;
}

sleep(2);
continue;
}
UsbCommandReceived((UsbCommand*)rx);

PrintAndLog("Proxmark reconnected!");
need_reconnect = false;
offline = 0;
//CmdVersionW(NULL, true);
clearCommandBuffer();
uart_send(sp, (byte_t*) &version_cmd, sizeof(UsbCommand)); // request it from the HW
Copy link
Contributor

Choose a reason for hiding this comment

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

This would destroy all data in BigBuf[] when reconnecting. I think the major purpose of the possibility to reconnect is to support offline functionality like sniffing some data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As it is now, unplugging causes the CPU to shoot to 100% used and the only way to continue what you're doing is to exit and restart the client. When I'm on my laptop I sometimes need to move USB ports or occasionally it accidentally comes unplugged, and needing to restart is annoying.

At any rate I've seen some odd segfaults in readline with this, so I was thinking about ripping out the "re-get version info" stuff anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, as far as I can tell clearing the command buf doesn't touch BigBuf[]. Not sure if the act of disconnecting/reconnecting the USB cable will do that, though without a battery whatever's in the hardware will be gone.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, clearCommandBuffer() doesn't touch BigBuf[] but querying for the version info does. This was the main reason to cache the version info on client side. I see two possible solutions:

  • cache the version info on device side instead of client side
  • don't query the version info on reconnect

The latter is obviously easier to implement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Newest version still has clearCommandBuffer() but instead of querying the version on re-connect it simply clears CmdVersion()'s cache.

request_version = true;
}

rxlen = 0;
if (uart_receive(sp, prx, sizeof(UsbCommand) - (prx-rx), &rxlen) ) {
if( rxlen ) {
prx += rxlen;
if (prx-rx < sizeof(UsbCommand)) {
continue;
}

UsbCommandReceived((UsbCommand*)rx);

if( request_version && ((UsbCommand*)rx)->cmd == CMD_ACK)
{
request_version = false;
CmdVersionW(NULL, true);
}
}
}
else {
PrintAndLog("Receiving data from proxmark failed, attempting a reconnect");
uart_close(sp);
sp = INVALID_SERIAL_PORT;
offline = 1;
txcmd_pending = false;
need_reconnect = true;
continue;
}

prx = rx;

if(txcmd_pending) {
Expand Down Expand Up @@ -351,6 +399,7 @@ int main(int argc, char* argv[]) {
set_my_executable_path();

// open uart
sp_name = argv[1];
if (!waitCOMPort) {
sp = uart_open(argv[1]);
} else {
Expand Down Expand Up @@ -405,7 +454,7 @@ int main(int argc, char* argv[]) {
#endif

// Clean up the port
if (usb_present) {
if (sp != INVALID_SERIAL_PORT && sp != CLAIMED_SERIAL_PORT) {
uart_close(sp);
}

Expand Down
12 changes: 6 additions & 6 deletions uart/uart_posix.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ bool uart_receive(const serial_port sp, byte_t* pbtRx, size_t pszMaxRxLen, size_

// Reset the output count
*pszRxLen = 0;

do {
// Reset file descriptor
FD_ZERO(&rfds);
Expand All @@ -153,12 +153,12 @@ bool uart_receive(const serial_port sp, byte_t* pbtRx, size_t pszMaxRxLen, size_
if (res < 0) {
return false;
}

// Read time-out
if (res == 0) {
if (*pszRxLen == 0) {
// Error, we received no data
return false;
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this path also return true? Don't you want to know when there is an 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.

Timing out without receiving any data may not be an error if the hardware just doesn't have any data ready for us at the moment.

Copy link
Contributor

Choose a reason for hiding this comment

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

uart_posix.c does some additional and unnecessary re-packaging which is already handled in proxmark3.c uart_receiver():

			if (prx-rx < sizeof(UsbCommand)) {
				continue;
			}

uart_win32.c doesn't do that. Why not aligning/simplyfying the Unix version and make it behave like the Windows version?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah true. I also double checked ReadFile (the win32api function that this is equivalent to), that doesn't return false on timeout.

So that entire if statement can go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Win and *nix APIs are totally different. It's been a while since I looked at uart_posix.c and I really didn't spend a huge amount of time in there, but from what I recall the various return's are to break out of a loop or to handle the various conditions the select() can result in (read error vs "no data available" which isn't an error). If it's rewritten to be like uart_win32.c then it will block forever (or at least until the PM3 is unplugged) and not allow the comms thread to exit on client shutdown due to the "do { ... } while(byteCount);" . The only clean-up I see is the "if(..) return true; else return true;" As per my first comment I have absolutely no clue if the WIN32 routine returns an error on no data or not, or an error on a read error or not. No windoze boxen in this building...

Copy link
Contributor

@micolous micolous Feb 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

FYI, if you want to test stuff on Windows, http://modern.ie/ has time-limited trial Windows virtual machines. They're ostensibly for testing websites in Internet Explorer and Edge, but you can install any software you like on them.

That should be sufficient for getting ProxSpace (the Windows build environment) going.

} else {
// We received some data, but nothing more is available
return true;
Expand All @@ -168,7 +168,7 @@ bool uart_receive(const serial_port sp, byte_t* pbtRx, size_t pszMaxRxLen, size_
// Retrieve the count of the incoming bytes
res = ioctl(((serial_port_unix*)sp)->fd, FIONREAD, &byteCount);
if (res < 0) return false;

// Cap the number of bytes, so we don't overrun the buffer
if (pszMaxRxLen - (*pszRxLen) < byteCount) {
byteCount = pszMaxRxLen - (*pszRxLen);
Expand All @@ -178,8 +178,8 @@ bool uart_receive(const serial_port sp, byte_t* pbtRx, size_t pszMaxRxLen, size_
res = read(((serial_port_unix*)sp)->fd, pbtRx+(*pszRxLen), byteCount);

// Stop if the OS has some troubles reading the data
if (res <= 0) return false;
if (res < 0) return false;

*pszRxLen += res;

if (*pszRxLen == pszMaxRxLen) {
Expand Down