Memory leak in VR_LoadHmdSystemInternal()

[mann1x]

The function Path_IsDirectory (pathtools_public.cpp) is leaking memory (at least on Windows).

Fixed changing this code:

    if ( _wstat( wsFixedPath.c_str(), &buf ) == -1 )
    {
        return false;
    }

    return false;

    return (buf.st_mode & _S_IFDIR) != 0;

to this:

    std::wstring wsFixedPath = UTF8to16( sFixedPath.c_str() );

    if (_wstat(wsFixedPath.c_str(), &buf) == 0 && (buf.st_mode & _S_IFDIR)) return true;

    return false;

[okawo80085]

Very important question, on Windows does it leak when compiled with CMake or VisualStudio?

Reasoning being: when you do a pure cmake build even if it uses visualstudio it behaves differently, e.g. wstring are treated differently in some cases

[mann1x]

VisualStudio

[okawo80085]

That explains how you were able to compile it

The master branch of openvr has a broken CMake build, see #1594

And ironically enough it should be compiled with CMake, so there are 2 ways to further test this 1) You fix the CMake build in your tree and test it when built with CMake 2) You test it with my fork which already has the CMake build fixes, can be found here

[mann1x]

I'm a Sunday's developer, coding for fun... Anyway, since I have a Debian host that I use as NAS will try when I have some spare time.

But wouldn't be just easier to use something that works both with Visual Studio and CMake? Or do you mean it should be tested on CMake as well cause it could not work as expected with it?

I'm pretty sure 99% of hobbyists developers will re-compile the library with Visual Studio anyway :)

[okawo80085]

You are definitely right about hobbyists, however testing with both CMake and VIsualStudio is important

And even if people will use VisualStudio that doesnt mean they shouldn't use CMake, because VisualStudio can build using CMake and one can just install CMake on Windows and build with it from the command prompt (and imo its way easier to use then visualstudio)

[mann1x]

Ok, I was not really up to date. Thought I had to install CMake on a Linux/WSL host. Let me check.

[mann1x]

Also on your fork there's the CleanupInternalInterfaces issue:

openvr_api_public.obj : error LNK2019: unresolved external symbol "void __cdecl vr::CleanupInternalInterfaces(void)" (?
CleanupInternalInterfaces@vr@@YAXXZ) referenced in function VR_ShutdownInternal 

What's the best workaround? I have commented it for my build.

[okawo80085]

Oh thats a new one, did you use the CMakeLists.txt from the root of the repo?

Because Valve use CMake subdirectories in their CMakeLists.txt, so it matters which one you actually run

[mann1x]

I assume so, from "cmake --build ." output:

Building Custom Rule C:/Users/manni/source/repos/openvrokawo/src/CMakeLists.txt

[okawo80085]

I don't remember the exact output from the top of my head, I'm not near my pc rn, when I am I'll post the full output from a build

[mann1x]

I've just commented it as I did before. Took me a while to confirm; yes, while compiling with CMake the original code doesn't leak.

I've stumbled on another and more insidious memory leak which I'm not sure it's related to the C# binding or not. My guess is not but I'm not exactly an expert in the field. From what I recall it was the same with Python.

[okawo80085]

Huh weird, first cuz im not getting the same link error on build

Here is a literal dump of a successful build from my fork's master, the worst thing im getting is string function warnings

D:\coding_n_shit\openvr\build>cmake -DBUILD_SHARED=1 ..
-- Building for: Visual Studio 16 2019
CMake Deprecation Warning at CMakeLists.txt:2 (cmake_minimum_required):
  Compatibility with CMake < 2.8.12 will be removed from a future version of
  CMake.

  Update the VERSION argument <min> value or use a ...<max> suffix to tell
  CMake that the project does not need compatibility with older versions.

-- The C compiler identification is MSVC 19.29.30040.0
-- The CXX compiler identification is MSVC 19.29.30040.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: D:/vs_shid/ide/VC/Tools/MSVC/14.29.30037/bin/Hostx64/x64/cl.exe - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: D:/vs_shid/ide/VC/Tools/MSVC/14.29.30037/bin/Hostx64/x64/cl.exe - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Configuring done
-- Generating done
-- Build files have been written to: D:/coding_n_shit/openvr/build

D:\coding_n_shit\openvr\build>cmake --build .
Microsoft (R) Build Engine version 16.10.2+857e5a733 for .NET Framework
Copyright (C) Microsoft Corporation. All rights reserved.

  Checking Build System
  Building Custom Rule D:/coding_n_shit/openvr/src/CMakeLists.txt
  openvr_api_public.cpp
  jsoncpp.cpp
  dirtools_public.cpp
D:\coding_n_shit\openvr\src\vrcommon\dirtools_public.cpp(35,2): warning C4996: 'strcpy': This function or variable may
be unsafe. Consider using strcpy_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for de
tails. [D:\coding_n_shit\openvr\build\src\openvr_api64.vcxproj]
  envvartools_public.cpp
  pathtools_public.cpp
D:\coding_n_shit\openvr\src\vrcommon\pathtools_public.cpp(422,10): warning C4996: 'stricmp': The POSIX name for this it
em is deprecated. Instead, use the ISO C and C++ conformant name: _stricmp. See online help for details. [D:\coding_n_s
hit\openvr\build\src\openvr_api64.vcxproj]
D:\coding_n_shit\openvr\src\vrcommon\pathtools_public.cpp(549,18): warning C4996: 'stricmp': The POSIX name for this it
em is deprecated. Instead, use the ISO C and C++ conformant name: _stricmp. See online help for details. [D:\coding_n_s
hit\openvr\build\src\openvr_api64.vcxproj]
D:\coding_n_shit\openvr\src\vrcommon\pathtools_public.cpp(557,19): warning C4996: 'stricmp': The POSIX name for this it
em is deprecated. Instead, use the ISO C and C++ conformant name: _stricmp. See online help for details. [D:\coding_n_s
hit\openvr\build\src\openvr_api64.vcxproj]
D:\coding_n_shit\openvr\src\vrcommon\pathtools_public.cpp(869,8): warning C4996: 'strnicmp': The POSIX name for this it
em is deprecated. Instead, use the ISO C and C++ conformant name: _strnicmp. See online help for details. [D:\coding_n_
shit\openvr\build\src\openvr_api64.vcxproj]
  sharedlibtools_public.cpp
  hmderrors_public.cpp
D:\coding_n_shit\openvr\src\vrcommon\hmderrors_public.cpp(333,4): warning C4996: 'sprintf': This function or variable m
ay be unsafe. Consider using sprintf_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help fo
r details. [D:\coding_n_shit\openvr\build\src\openvr_api64.vcxproj]
  vrpathregistry_public.cpp
  strtools_public.cpp
D:\coding_n_shit\openvr\src\vrcommon\strtools_public.cpp(42,14): warning C4996: 'strnicmp': The POSIX name for this ite
m is deprecated. Instead, use the ISO C and C++ conformant name: _strnicmp. See online help for details. [D:\coding_n_s
hit\openvr\build\src\openvr_api64.vcxproj]
D:\coding_n_shit\openvr\src\vrcommon\strtools_public.cpp(61,14): warning C4996: 'stricmp': The POSIX name for this item
 is deprecated. Instead, use the ISO C and C++ conformant name: _stricmp. See online help for details. [D:\coding_n_shi
t\openvr\build\src\openvr_api64.vcxproj]
D:\coding_n_shit\openvr\src\vrcommon\strtools_public.cpp(187,2): warning C4996: 'strncpy': This function or variable ma
y be unsafe. Consider using strncpy_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for
 details. [D:\coding_n_shit\openvr\build\src\openvr_api64.vcxproj]
  Generating Code...
     Creating library D:/coding_n_shit/openvr/lib/win64/Debug/openvr_api64.lib and object D:/coding_n_shit/openvr/lib/w
  in64/Debug/openvr_api64.exp
  openvr_api64.vcxproj -> D:\coding_n_shit\openvr\bin\win64\Debug\openvr_api64.dll
  Building Custom Rule D:/coding_n_shit/openvr/CMakeLists.txt

And if you're getting a new leak please share

[mann1x]

It was not what I thought. After a series of uncoherent results I've found out that Trace.WriteLine is randomly leaking memory. Now I'm back to no leaks until the vrclient DLL is loaded/unloaded and leaking memory and handles like a boss.

[okawo80085]

Weird, very weird

@JoeLudwig Any thoughts? This is a serious issue that can't be solved by "just don't use it"

[mann1x]

I guess this is just the Pandora's vase lid of what's causing this eternal instability of SteamVR closing and re-opening and becoming so unstable to require a full system reboot.

It's unacceptable to use it from anything that is not killed afterwards; 6-7 MB of memory and 50 handles leaked every time OpenVR is opened/closed. Surprisingly to me, there's a lot of people that doesn't reboot the system for days/weeks and is launching hundreds and hundreds of times VR games/apps in the period (in my previous tool in Phyton the BT library was messing with the Windows stack). Even before leaking gigabytes of memory my tool would die from the hundreds thousands handles leaked.

Despite my little proficiency with c++, I decided to go for the nuclear option. Scrap altogether OpenVR integration and develop my own custom driver. At least this mess will be self-contained on SteamVR without infecting my process. But it'd be nice if after five years of struggling we could get a more reliable SteamVR!