errant xlink error checking, deref null ptr, string/memory out of bounds, memory leak, etc.

diablodale commented 2 years ago

XLink has various unsafe usage found by the MSVC code analyzer. Some are code defects and can lead to abnormal program flow and/or crash.

My intention is to fix code defects, and to decrease noise so that important warnings/errors can be seen. I have a PR ready that addresses all issues except one in the CONCERN section below

Setup

Microsoft Windows [Version 10.0.19044.1415]
MSVC v16.11.8, with code analyzer enabled, and CppCoreCheck.dll, yet disabling several warnings
XLink master eef5a51c608fef8e326c740f120b90a543e55b27

Repro

config for x64, debug, static lib
and with msvc compile flags to include /analyze:plugin EspXEngine.dll /wd26440 /wd26496 /wd26462 /wd26460 /wd26481 /wd26493 /wd26446 /wd26482 /wd26485 /wd26812 /wd26476 /wd26490 /wd26472 /wd26495 /wd26432 /wd26457 /wd6031

Set env var for the analyzer...

esp.extensions=CppCoreCheck.dll
esp.annotationbuildlevel=ignore

build Xlink

Result

I read the below compiler+analyzer output and investigated the code. Almost all the issues the analyzer found are legitimate bugs (not style issues).

[main] Building folder: depthai-core XLink
[build] Starting build
[proc] Executing command: "C:\Program Files\CMake\bin\cmake.exe" --build c:/njs/depthai-core/build --config Debug --target XLink -j 14 --
[build] [14/24   4% :: 1.621] Building C object XLink\CMakeFiles\XLink.dir\src\pc\protocols\usb_mx_id.c.obj
[build] [15/24   8% :: 1.855] Building C object XLink\CMakeFiles\XLink.dir\src\shared\XLinkLog.c.obj
[build] [16/24  12% :: 1.883] Building C object XLink\CMakeFiles\XLink.dir\src\shared\XLinkSemaphore.c.obj
[build] [17/24  16% :: 1.941] Building C object XLink\CMakeFiles\XLink.dir\src\shared\XLinkStream.c.obj
[build] [18/24  20% :: 1.953] Building C object XLink\CMakeFiles\XLink.dir\src\shared\XLinkPrivateFields.c.obj
[build] [19/24  25% :: 1.960] Building C object XLink\CMakeFiles\XLink.dir\src\shared\XLinkDispatcherImpl.c.obj
[build] C:\njs\XLink\src\shared\XLinkDispatcherImpl.c(346) : warning C6246: Local declaration of 'stream' hides declaration of the same name in outer scope. For additional information, see previous declaration at line '254' of 'c:\njs\xlink\src\shared\xlinkdispatcherimpl.c'.: Lines: 254
[build] C:\njs\XLink\src\shared\XLinkDispatcherImpl.c(417) : warning C6246: Local declaration of 'stream' hides declaration of the same name in outer scope. For additional information, see previous declaration at line '254' of 'c:\njs\xlink\src\shared\xlinkdispatcherimpl.c'.: Lines: 254
[build] C:\njs\XLink\src\shared\XLinkDispatcherImpl.c(546) : warning C6011: Dereferencing NULL pointer 'ret'. : Lines: 542, 543, 545, 546
[build] [20/24  29% :: 1.970] Building C object XLink\CMakeFiles\XLink.dir\src\shared\XLinkPrivateDefines.c.obj
[build] [21/24  33% :: 1.989] Building C object XLink\CMakeFiles\XLink.dir\src\shared\XLinkDeprecated.c.obj
[build] [22/24  37% :: 2.005] Building C object XLink\CMakeFiles\XLink.dir\src\pc\PlatformDeviceSearch.c.obj
[build] [23/24  41% :: 2.020] Building C object XLink\CMakeFiles\XLink.dir\src\pc\PlatformData.c.obj
[build] [23/24  45% :: 2.045] Building C object XLink\CMakeFiles\XLink.dir\src\pc\PlatformDeviceControl.c.obj
[build] C:\njs\XLink\src\pc\PlatformDeviceControl.c(660) : warning C6387: 'devPathWriteBuff' could be '0':  this does not adhere to the specification for the function 'strncpy'. : Lines: 643, 644, 656, 658, 659, 660
[build] C:\njs\XLink\src\pc\PlatformDeviceControl.c(662) : warning C6053: The prior call to 'strncpy' might not zero-terminate string 'devPathWriteBuff'.: Lines: 643, 644, 656, 658, 659, 660, 662
[build] [23/24  50% :: 2.058] Building C object XLink\CMakeFiles\XLink.dir\src\shared\XLinkDevice.c.obj
[build] C:\njs\XLink\src\shared\XLinkDevice.c(331): warning C4244: '+=': conversion from 'int64_t' to 'long', possible loss of data
[build] C:\njs\XLink\src\shared\XLinkDevice.c(333): warning C4244: '-=': conversion from 'int64_t' to 'long', possible loss of data
[build] C:\njs\XLink\src\shared\XLinkDevice.c(325): warning C4101: 'end': unreferenced local variable
[build] [23/24  54% :: 2.097] Building C object XLink\CMakeFiles\XLink.dir\src\pc\protocols\tcpip_host.c.obj
[build] C:\njs\XLink\src\pc\protocols\tcpip_host.c(393): warning C4133: 'function': incompatible types - from 'tcpipHostCommand_t *' to 'const char *'
[build] C:\njs\XLink\src\pc\protocols\tcpip_host.c(169) : warning C6001: Using uninitialized memory 'size'.: Lines: 166, 167, 169
[build] C:\njs\XLink\src\pc\protocols\tcpip_host.c(186) : warning C6011: Dereferencing NULL pointer 'ipaddrtable'. : Lines: 166, 167, 169, 170, 173, 175, 176, 181, 182, 183, 184, 185, 186
[build] [23/24  58% :: 2.184] Building C object XLink\CMakeFiles\XLink.dir\src\shared\XLinkStringUtils.c.obj
[build] [23/24  62% :: 3.113] Building C object XLink\CMakeFiles\XLink.dir\src\pc\Win\src\win_time.c.obj
[build] [23/24  66% :: 3.202] Building C object XLink\CMakeFiles\XLink.dir\src\pc\Win\src\win_synchapi.c.obj
[build] [23/24  70% :: 3.236] Building C object XLink\CMakeFiles\XLink.dir\src\pc\protocols\usb_boot.c.obj
[build] C:\njs\XLink\src\pc\protocols\usb_boot.c(807) : warning C6244: Local declaration of 'bulk_chunklen' hides previous declaration at line '48' of 'c:\njs\xlink\src\pc\protocols\usb_boot.c'.: Lines: 48
[build] [23/24  75% :: 3.295] Building C object XLink\CMakeFiles\XLink.dir\src\pc\Win\src\win_pthread.c.obj
[build] C:\njs\XLink\src\pc\Win\src\win_pthread.c(184) : warning C6387: 'Temp_value_#19' could be '0':  this does not adhere to the specification for the function 'GetProcAddress'. : Lines: 184, 181, 182, 184
[build] C:\njs\XLink\src\pc\Win\src\win_pthread.c(205) : warning C6387: 'Temp_value_#33' could be '0':  this does not adhere to the specification for the function 'GetProcAddress'. : Lines: 205, 203, 205
[build] [23/24  79% :: 3.300] Building C object XLink\CMakeFiles\XLink.dir\src\pc\protocols\pcie_host.c.obj
[build] [23/24  83% :: 3.343] Building C object XLink\CMakeFiles\XLink.dir\src\pc\Win\src\win_semaphore.c.obj
[build] [23/24  87% :: 3.441] Building C object XLink\CMakeFiles\XLink.dir\src\shared\XLinkData.c.obj
[build] C:\njs\XLink\src\shared\XLinkData.c(463): warning C4244: '+=': conversion from 'int64_t' to 'long', possible loss of data
[build] C:\njs\XLink\src\shared\XLinkData.c(465): warning C4244: '-=': conversion from 'int64_t' to 'long', possible loss of data
[build] [23/24  91% :: 3.545] Building C object XLink\CMakeFiles\XLink.dir\src\shared\XLinkDispatcher.c.obj
[build] [23/24  95% :: 3.830] Building C object XLink\CMakeFiles\XLink.dir\src\pc\Win\src\win_usb.c.obj
[build] C:\njs\XLink\src\pc\Win\src\win_usb.c(432): warning C4090: 'function': different 'const' qualifiers
[build] C:\njs\XLink\src\pc\Win\src\win_usb.c(432): warning C4267: 'function': conversion from 'size_t' to 'ULONG', possible loss of data
[build] C:\njs\XLink\src\pc\Win\src\win_usb.c(423): warning C4267: 'initializing': conversion from 'size_t' to 'USHORT', possible loss of data
[build] C:\njs\XLink\src\pc\Win\src\win_usb.c(917): warning C4996: 'strdup': The POSIX name for this item is deprecated. Instead, use the ISO C and C++ conformant name: _strdup. See online help for details.
[build] C:\njs\XLink\src\pc\Win\src\win_usb.c(152) : warning C6255: _alloca indicates failure by raising a stack overflow exception.  Consider using _malloca instead.
[build] C:\njs\XLink\src\pc\Win\src\win_usb.c(563) : warning C28182: Dereferencing NULL pointer. 'deviceInterfaceDetailData' contains the same NULL value as 'LocalAlloc()`560' did. : Lines: 545, 546, 548, 549, 550, 553, 554, 558, 559, 560, 563
[build] C:\njs\XLink\src\pc\Win\src\win_usb.c(718) : warning C26451: Arithmetic overflow: Using operator '*' on a 4 byte value and then casting the result to a 8 byte value. Cast the value to the wider type before calling operator '*' to avoid overflow (io.2).
[build] C:\njs\XLink\src\pc\Win\src\win_usb.c(828) : warning C6011: Dereferencing NULL pointer 'pDevList->infos'. : Lines: 808, 810, 811, 812, 814, 815, 817, 818, 824, 825, 827, 828
[build] C:\njs\XLink\src\pc\Win\src\win_usb.c(832) : warning C6387: 'pDevList->infos+i' could be '0':  this does not adhere to the specification for the function 'SetupDiEnumDeviceInfo'. See line 828 for an earlier location where this can occur: Lines: 808, 810, 811, 812, 814, 815, 817, 818, 824, 825, 827, 828, 827, 828, 827, 832
[build] C:\njs\XLink\src\pc\Win\src\win_usb.c(833) : warning C6387: 'pDevList->infos+i' could be '0':  this does not adhere to the specification for the function 'SetupDiGetDeviceRegistryPropertyA'. See line 828 for an earlier location where this can occur: Lines: 808, 810, 811, 812, 814, 815, 817, 818, 824, 825, 827, 828, 827, 828, 827, 832, 833
[build] C:\njs\XLink\src\pc\Win\src\win_usb.c(841) : warning C6011: Dereferencing NULL pointer 'pDevList->vidpids'. : Lines: 808, 810, 811, 812, 814, 815, 817, 818, 824, 825, 827, 828, 827, 828, 827, 832, 833, 836, 837, 841
[build] [24/24 100% :: 3.873] Linking C static library XLink\XLinkd.lib
[build] Build finished with exit code 0

Concern -- need feedback

src\pc\protocols\usb_boot.c there are two vars same name bulk_chunklen. One is global static. The other is function local

Problem 1 -- same name
Problem 2 -- look at how the static is used. It is set (in non Windows codepath) but never read by any codepath. The local var is set/read. Like 823-827 makes me suspect that it should be reading the global bulk_chunklen as it might have had its value restricted by line 706 -- where libusb declares the max packet size for this usb endpoint. This is suspicious.

I suspect a code defect concerning these bulk_chunklen vars. I don't have the USB experience or the hardware-level visibility to test this. 🤔 What is your view on this topic? I want to update the PR to address this (or open a separate bug for your USB team member to investigate).

FYI

In win_usb.c the two call sites for SetupDiGetDeviceInterfaceDetail() have inconsistent code, no error handling, memory leak due to missing LocalFree(), and use of problematic Windows apis like _alloca(). The call site get_mx_id_device_path() is definitely used. I didn't readily find a client-side codepath that would call the other retreive_dev_path().

_alloca() raises a Windows SEH exception on error and the existing code will crash the main program. SEH exceptions are Windows-only and require Windows specific wrapping via __try __except __finally or the program crashes. https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/alloca?view=msvc-170 https://docs.microsoft.com/en-us/cpp/cpp/structured-exception-handling-c-cpp?view=msvc-170

I resolved difference in the two call sites, handled leaks and errors, and changed to use malloc(). There is no need for the other inconsistent memory alloc functions.

If you want to use _alloca() at one of the call sites, the adjusted code should be similar to... 😐

__try
{
    detData = (PSP_DEVICE_INTERFACE_DETAIL_DATA)_alloca(reqLen);
}
__except (GetExceptionCode() == STATUS_STACK_OVERFLOW)
{
    // https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/alloca
    // when stack overflows, attempt stack restore
    if (!_resetstkoflw())
        mvLog(MVLOG_FATAL, "Stack corrupt after _alloca() failure for SetupDiEnumDeviceInterfaces()\n");
    SetupDiDestroyDeviceInfoList(devInfo);
    return USB_DEV_NONE;
}

themarpe commented 2 years ago

Hi @diablodale

Regarding Concern section: I'd leave local variable and remove the global static and write to global static. libusb should handle that, so not really sure what the intention of setting it to wMaxPacketSize was.

Regarding FYI section: Thanks for addressing these - for the future, IMO best to go with device_search_improvements_libusb way, as they remove the hard to deal with WinUSB code

diablodale commented 2 years ago

OK, will update PR.

I get the desire for a single usb API. libusb will introduce LGPL 2.1 licensing which is very particular and legal in how libusb can be linked into code that uses it. As I understand, LGPL requires libusb can never be compiled into other code. Otherwise, the LGPL license is forced onto that using code. Instead, libusb must always be dynamically/shared linked. Someone (who?) has to compile libusb into a DLL and host the license and code. And then that independent DLL must be only dynamically linked to xlink and depthai-core; and the libusb.DLL distributed alongside the using code.

This then adds libusb.DLL (and other OS equals) to all your prebuild distributions and cmake needs related compile/install changes. Because if you compile libusb into using code, then the depthai license changes from MIT -> LGPL.

themarpe commented 2 years ago

Yeah, LGPL isn't the most open licence but will work okay as long as we note a couple of things.

There are 2 ways to go:

make libusb a dll (as is already on macos and Linux platforms). Windows might make this a tad harder because of the lack of rpath
- compile depthai-core as dll with libusb static

The third option of having static libusb and static core, comes with a caveat that consuming application must then either be: provided in an object form to be able to relink another libusb or be opensource. This case should be documented so people aren't introducing licencing incompatible code into their project.

Note, we'll default to build libusb as a dll, so the change shouldn't affect anyone (with exception of Windows users regarding installing the libusb dll along their app)

https://softwareengineering.stackexchange.com/questions/312758/does-providing-object-files-satisfy-lgpl-relink-clause#312759

Core doesn't have to become LGPL licenced as there is the exemption of being able to relink with another version of libusb, which an opensource library consuming it fits into (in dll case).

Disclaimer, I'm am not a lawyer - this is my understanding from reading through various sources.

diablodale commented 2 years ago

I'm in alignment with what you wrote (as both non-lawyers). I've opened issue #37 to discuss independent of this issue.

luxonis / XLink