[PATCH 0/1] MR6152: msvcrt, winex11, mountmgr: fixed memleaks

[Herman Semenov (＠GermanAizek)]

I think such catches will be useful to you.

[PATCH 1/1] msvcrt, winex11, mountmgr: fixed memleaks

[Herman Semenov]

From: Herman Semenov

---
 dlls/mountmgr.sys/device.c | 5 +++++
 dlls/msvcrt/dir.c          | 4 ++++
 dlls/msvcrt/time.c         | 2 ++
 dlls/winex11.drv/xim.c     | 1 +
 4 files changed, 12 insertions(+)

diff --git a/dlls/mountmgr.sys/device.c b/dlls/mountmgr.sys/device.c
index 48ee8d9189a..1f89f38f7d1 100644
--- a/dlls/mountmgr.sys/device.c
+++ b/dlls/mountmgr.sys/device.c
@@ -1463,7 +1463,12 @@ NTSTATUS query_unix_drive( void *buff, SIZE_T insize, SIZE_T outsize, IO_STATUS_
     LeaveCriticalSection( &device_section );

if (!volume)
+    {
+        free(device);
+        free(mount_point);
+        free(label);
         return STATUS_NO_SUCH_DEVICE;
+    }

switch (device_type)
     {
diff --git a/dlls/msvcrt/dir.c b/dlls/msvcrt/dir.c
index 546e8d49a7b..f3bf58c2f59 100644
--- a/dlls/msvcrt/dir.c
+++ b/dlls/msvcrt/dir.c
@@ -1277,6 +1277,8 @@ wchar_t * CDECL _wfullpath(wchar_t * absPath, const wchar_t* relPath, size_t siz

if (size < 4)
   {
+    if (alloced)
+      free(buffer);
     *_errno() = ERANGE;
     return NULL;
   }
@@ -1331,6 +1333,8 @@ char * CDECL _fullpath(char * absPath, const char* relPath, size_t size)

if (size < 4)
   {
+    if (alloced)
+        free(buffer);
     *_errno() = ERANGE;
     return NULL;
   }
diff --git a/dlls/msvcrt/time.c b/dlls/msvcrt/time.c
index 7a110e53455..044eefbae05 100644
--- a/dlls/msvcrt/time.c
+++ b/dlls/msvcrt/time.c
@@ -1740,6 +1740,7 @@ errno_t CDECL _ctime64_s(char *res, size_t len, const __time64_t *time)

t = _localtime64( time );
     strcpy( res, asctime( t ) );
+    free(t);
     return 0;
 }

@@ -1769,6 +1770,7 @@ errno_t CDECL _ctime32_s(char *res, size_t len, const __time32_t *time)

t = _localtime32( time );
     strcpy( res, asctime( t ) );
+    free(t);
     return 0;
 }

diff --git a/dlls/winex11.drv/xim.c b/dlls/winex11.drv/xim.c
index c6a93eb5e16..2bd8c63969a 100644
--- a/dlls/winex11.drv/xim.c
+++ b/dlls/winex11.drv/xim.c
@@ -99,6 +99,7 @@ static void xim_update_comp_string( UINT offset, UINT old_len, const WCHAR *text
     memmove( ptr + new_len, ptr + old_len, (len - offset - old_len) * sizeof(WCHAR) );
     if (text) memcpy( ptr, text, new_len * sizeof(WCHAR) );
     ime_comp_buf[len + diff] = 0;
+    free(ptr);
 }

void xim_set_result_string( HWND hwnd, const char *str, UINT count )

[PATCH 1/1] msvcrt, winex11, mountmgr: fixed memleaks

[Marvin]

Hi,

It looks like your patch introduced the new failures shown below.
Please investigate and fix them before resubmitting your patch.
If they are not new, fixing them anyway would help a lot. Otherwise
please ask for the known failures list to be updated.

The tests also ran into some preexisting test failures. If you know how
to fix them that would be helpful. See the TestBot job for the details:

The full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=147335

Your paranoid android.

=== debian11b (64 bit WoW report) ===

ddraw:
ddraw7.c:3741: Test failed: Expected message 0x5, but didn't receive it.
ddraw7.c:3743: Test failed: Expected screen size 1024x768, got 0x0.

[Zhiyi Zhang (＠zhiyi)]

Hi, please split the commit by component into three commits.

[Aida Jonikienė (＠DodoGTA)]

Also a nitpick: You should match the coding style of surrounding code (by adding spaces between the brackets on the inside in this case)

[Piotr Caban (＠piotr)]

Piotr Caban (@piotr) commented about dlls/msvcrt/dir.c:
> if (size < 4)
>    {
> - if (alloced)
> -
>  free(buffer);

There's no leak in this function. `size` is set to `MAX_PATH` when `alloced` is set to TRUE. A quick testing shows that the function may be improved though (e.g. we shouldn't ignore `size` parameter when buffer gets allocated). How did you spot this? Is there any compiler that prints a warning on this code?

[Piotr Caban (＠piotr)]

Piotr Caban (@piotr) commented about dlls/msvcrt/time.c:
>  t = _localtime64( time );
>  strcpy( res, asctime( t ) );
> - free(t);

Again, the implementation of `_ctime64_s` is broken (it shouldn't use non thread-safe `_localtime64` function) but there's no leak. Value returned by `_localtime64` should not be freed.

[PATCH 0/1] MR6152: msvcrt, winex11, mountmgr: fixed memleaks - closed

[Piotr Caban (＠piotr)]

This merge request was closed by Piotr Caban.

[Piotr Caban (＠piotr)]

I've looked on remaining changes and mountmgr and winex11 changes are also not needed (in winex11 it's freeing global variable that is supposed to be not freed, in mountmgr - the strings are only allocated if volume is TRUE so there's no leak).

[Piotr Caban (＠piotr)]

On Thu Jul 25 09:11:23 2024 +0000, Piotr Caban wrote:
> Again, the implementation of `_ctime64_s` is broken (it shouldn't use
> non thread-safe `_localtime64` function) but there's no leak. Value
> returned by `_localtime64` should not be freed.

I've created !6155 that addresses _ctime*_s functions not being thread-safe bug.