Bug 499

Summary:	PulseView crash caused by get_port_details() in Windows 7
Product:	libserialport	Reporter:	Boris Gjenero <boris.gjenero>
Component:	Port enumeration	Assignee:	Nobody <nobody>
Status:	RESOLVED FIXED
Severity:	critical	CC:	aurel, uwe
Priority:	Normal
Version:	unreleased development snapshot
Target Milestone:	---
Hardware:	x86
OS:	Windows
Attachments:	Initialize usb_path to NULL to avoid calling free() on unintialized pointer in sp_free_port()

Description Boris Gjenero 2014-11-27 17:33:58 CET

PulseView usually crashes for me in Windows 7, but I can sometimes get it to run. It seems the test at line 365 in windows.c is wrong:

(gdb)
356                     if (!(device_key = SetupDiOpenDevRegKey(device_info, &device_info_data,
(gdb)
361                     if (RegQueryValueExA(device_key, "PortName", NULL, &type, (LPBYTE)value,
(gdb) p device_key
$1 = (HKEY) 0xffffffff

According to Microsoft documentation at http://msdn.microsoft.com/en-us/library/windows/hardware/ff552079%28v=vs.85%29.aspx "If the function fails, it returns INVALID_HANDLE_VALUE.", which is -1. Line 356 is instead checking for a non-zero value. Afterwards, a SIGSEGV happens in code called from RegCloseKey(device_key) at line 363.

Comment 1 Aurélien Jacobs 2014-11-27 18:25:42 CET

Fix available here:
https://github.com/aurelj/libserialport/commit/b328a48b0fd8ce51f400b5e06bc1e2ab52e3c9ae

Comment 2 Boris Gjenero 2014-11-28 03:14:04 CET

Thanks for the quick fix. It looks good but I didn't test it yet because I don't have a build environment set up. For now I just binary-patched the relevant jump in the executable, so it jumps if the value is negative instead of if the value is zero.

It seems there is another problem. I'm still getting crashes, but never in gdb. They're RtlFreeHeap() called from free() called in sp_free_port() here:
286 #ifdef _WIN32
287         if (port->usb_path)
288                 free(port->usb_path);
289 #endif

Is port->usb_path being initialized to zero anywhere? I see get_port_details() in windows.c initializes it via "port->usb_path = strdup(usb_path);", inside an "if (port->transport == SP_TRANSPORT_USB) {" block. Maybe that if should have an else with "port->usb_path = NULL"?

My motherboard has a classic non-USB serial port. Those are rare nowadays, so maybe few people run into this issue.

Comment 3 Uwe Hermann 2014-11-29 15:53:53 CET

Thanks, merged as b328a48b0fd8ce51f400b5e06bc1e2ab52e3c9ae.

I'm closing this issue since this specific crash should be fixed. Please try the current git HEAD (with the fix) to see if you experience additional issues and open another bug report for that (if yes). Thanks!

Also, please let us know if there's a way to force/reproduce the crash, I couldn't cause it to crash in my tests (but the bugfix is needed anyway, so that's merged now).

Comment 4 Boris Gjenero 2014-11-29 23:31:05 CET

Created attachment 103 [details]
Initialize usb_path to NULL to avoid calling free() on unintialized pointer in sp_free_port()