-
Notifications
You must be signed in to change notification settings - Fork 92
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
Segmentation fault #9
Comments
Ok so first the Ubuntu code is super old. It hasn't been synced with the rest of the changes I/we have made for awhile. I think I might have broken something with d848813. Try reverting that and see what happens. |
Note: I did compile it myself so it was using the latest code in commit 9c0fd2b. I'll poke around at that later; I've saved a core dump of when the seg fault was hit. It seems like the buffer that overflowed was allocated inside of hu_aap.c though, so it would be weird if the Ubuntu specific code would affect overflowing that buffer especially since the Ubuntu specific code only handles the h264 output side of things (it crashed in the decryption portion of the data stream). The hu_aap_recv_process() function increments buf from the it's starting place as it processes data and it trusts the output in the encoded stream as to not overflowing its own buffer (enc_len) as it decrypts the data stream. E.g. it decoded a length of "16149" when it was already at offset 1032 in the incoming data stream, and as such iaap_recv_dec_process() went beyond the bounds of the 16384 buffer. |
Okay, I added in the debug output from the program (undefining NDEBUG). It looks like I got a LIBUSB error which trickled down into the decryption code. Maybe this won't happen on the head unit? Although, I'd suspect that if it can happen on a desktop with a theoretically mature USB stack... Here's the end of the log before a seg fault:
|
I added the following in there: diff --git a/jni/hu_aap.c b/jni/hu_aap.c
index 00cab99..3773712 100644
--- a/jni/hu_aap.c
+++ b/jni/hu_aap.c
@@ -1392,6 +1392,9 @@ http://www.cisco.com/c/en/us/support/docs/security-vpn/secure-socket-layer-ssl/1
if (transport_type != 2 || rx_len != min_size_hdr) // If NOT wifi...
logd ("have_len: %d < enc_len: %d need_len: %d", have_len, enc_len, need_len);
+ logd ("Buf = rx_buf[%i]; hu_aap_tra_recv(&buf[%i], %i, -1)",
+ buf - rx_buf, have_len, need_len);
+
int need_ret = hu_aap_tra_recv (& buf [have_len], need_len, -1);// Get Rx packet from Transport. Use -1 instead of iaap_tra_recv_tmo to indicate need to get need_len bytes
// Length remaining for all sub-packets plus 4/8 byte headers
if (need_ret != need_len) { // If we didn't get precisely the number of bytes we need... I get the following in the routine:
Looks like the "more than one message" code needs to be revised. ;) Here's a quick hack that "fixes" the issue on my PC: diff --git a/jni/hu_aap.c b/jni/hu_aap.c
index 00cab99..e8bc403 100644
--- a/jni/hu_aap.c
+++ b/jni/hu_aap.c
@@ -1392,6 +1392,10 @@ http://www.cisco.com/c/en/us/support/docs/security-vpn/secure-socket-layer-ssl/1
if (transport_type != 2 || rx_len != min_size_hdr) // If NOT wifi...
logd ("have_len: %d < enc_len: %d need_len: %d", have_len, enc_len, need_len);
+ // Move the buffer back to the start
+ memmove(rx_buf, buf, have_len);
+ buf = rx_buf;
+
int need_ret = hu_aap_tra_recv (& buf [have_len], need_len, -1);// Get Rx packet from Transport. Use -1 instead of iaap_tra_recv_tmo to indicate need to get need_len bytes
// Length remaining for all sub-packets plus 4/8 byte headers
if (need_ret != need_len) { // If we didn't get precisely the number of bytes we need... However, that being said, it could be improved by detecting if it would overflow with the hu_aap_tra_recv and deciding only then to do the memory move. Also, the code shouldn't trust the length within the transport stream as being within bounds of the buffer in the program. |
Hmmm... interesting.... On the Android version, I have increased the DEFBUF size to match the value used in the DHU and the number of buffer overruns have significantly decreased, to be noted I'm using Wifi and TCP protocol instead of USB which can also add some overheads. Like your fix through... will try it on Android version, hoping to fix the annoying random disconnection... |
I haven't taken the time to debug this, but it is possible for rx_buf to overflow. This was observed with the Ubuntu folder variant on a 64-bit machine. iaap_recv_dec_process received a rx_buf at offset of 1032, with a len of 16149. These two combined exceed the 16384 buffer DEFBUF and overflows the buffer. (Something somewhere isn't paying enough attention to the boundaries as such!)
Glancing at the codebase, it looks like it processes multiple chunks of data from the remote at once moving through the buffer each time it grabs a segment. It should possibly move data back to the start of buf for each sub-packet if it detects that the remaining length exceeds the receive buffer.
Here's the full backtrace from gdb:
Note: This goes away if I increase the DEFBUF constant. I can observe this just by interacting with the Android Auto window for a few minutes.
The text was updated successfully, but these errors were encountered: