Skip to content
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

suspicious UNINITIALIZED READ warning when calling GetKeyNameTextW #1819

Closed
mistmist opened this issue Nov 18, 2015 · 4 comments
Closed

suspicious UNINITIALIZED READ warning when calling GetKeyNameTextW #1819

mistmist opened this issue Nov 18, 2015 · 4 comments

Comments

@mistmist
Copy link

Not sure if this is a bug in DrMemory, but i'm getting this surprising warning: according to MSDN the GetKeyNameTextW should write a null terminator at buf[ret] but DrMemory 1.9.0-4 (32-bit) says that there's a UNINITIALIZED READ on the "if (buf[ret])". (The key in the example is "CTRL")

https://msdn.microsoft.com/en-us/library/windows/desktop/ms646300%28v=vs.120%29.aspx

Build the following program with Visual Studio 2013:

cl.exe -Zi foo.cc user32.lib

#include <windows.h>

int main()
{
  WCHAR buf[350];
  int ret = GetKeyNameTextW(35454976, buf, 350);
  if (buf[ret]) // HERE
    return 1;
  else
    return 2;
}

Running it with "drmemory.exe foo.exe" on Windows 7:

Dr. Memory version 1.9.0 build 4 built on Oct  2 2015 13:13:14
Dr. Memory results for pid 4056: "foo.exe"
Application cmdline: "foo.exe"
Recorded 116 suppression(s) from default C:\Users\ms\DrMemory-Windows-1.9.0-4\bin\suppress-default.txt

Error #1: UNINITIALIZED READ: reading register edx
#0 main               [c:\cygwin\tmp\foo.cc:10]
Note: @0:00:01.291 in thread 3764
Note: instruction: test   %edx %edx

Running it with "-light" reports no error.
"-debug -dr_debug -pause_at_assert" does not print anything interesting.

@zhaoqin
Copy link
Contributor

zhaoqin commented Nov 18, 2015

From MSDN: "If the function fails, the return value is zero. "
did you check if ret is 0,

On Wed, Nov 18, 2015 at 6:05 PM, mistmist notifications@github.com wrote:

Not sure if this is a bug in DrMemory, but i'm getting this surprising
warning: according to MSDN the GetKeyNameTextW should write a null
terminator at buf[ret] but DrMemory 1.9.0-4 (32-bit) says that there's a
UNINITIALIZED READ on the "if (buf[ret])". (The key in the example is
"CTRL")

https://msdn.microsoft.com/en-us/library/windows/desktop/ms646300%28v=vs.120%29.aspx

Build the following program with Visual Studio 2013:

cl.exe -Zi foo.cc user32.lib

#include

int main()
{
WCHAR buf[350];
int ret = GetKeyNameTextW(35454976, buf, 350);
if (buf[ret]) // HERE
return 1;
else
return 2;
}

Running it with "drmemory.exe foo.exe" on Windows 7:

Dr. Memory version 1.9.0 build 4 built on Oct 2 2015 13:13:14
Dr. Memory results for pid 4056: "foo.exe"
Application cmdline: "foo.exe"
Recorded 116 suppression(s) from default
C:\Users\ms\DrMemory-Windows-1.9.0-4\bin\suppress-default.txt

Error #1 #1: UNINITIALIZED
READ: reading register edx
0 main [c:\cygwin\tmp\foo.cc:10]

Note: @0 https://github.com/0:00:01.291 in thread 3764
Note: instruction: test %edx %edx

Running it with "-light" reports no error.
"-debug -dr_debug -pause_at_assert" does not print anything interesting.


Reply to this email directly or view it on GitHub
#1819.

@mistmist
Copy link
Author

On my system return value is 4 and the buffer contains "CTRL" plus a null character (as the "return 2" branch is taken).

Hope that the key code does not depend on the Windows keyboard layout setting, for the record here it's En-IE but if i switch to En-US layout i get the same.

ghost pushed a commit to LibreOffice/core that referenced this issue Nov 19, 2015
DrMemory 1.9.0-4 32-bit complains about UNINITIALIZED READ on the
instruction "test %ecx %ecx" corresponding to the *pW test in the while
loop, on the last iteration.

GetKeyNameTextW() is documented to null-terminate the buffer, so either
it has a bug and doesn't do that, or DrMemory has a bug and falsely
reports an error.

Either way it can be avoided by checking the end position first.

DynamoRIO/drmemory#1819

Change-Id: I546d2057d39865d527c1d7c436f690669b214d75
@derekbruening derekbruening self-assigned this Nov 20, 2015
@derekbruening
Copy link
Contributor

The entry seems to not include the null, explaining the false positive:

    {{0,0},"NtUserGetKeyNameText", OK, SYSARG_TYPE_UINT32, 3,
     {
         {0, sizeof(LONG), SYSARG_INLINED, DRSYS_TYPE_SIGNED_INT},
         {1, -2, W|SYSARG_SIZE_IN_ELEMENTS, sizeof(wchar_t)},
         {1, RET, W|SYSARG_SIZE_IN_ELEMENTS, sizeof(wchar_t)},
         {2, sizeof(int), SYSARG_INLINED, DRSYS_TYPE_SIGNED_INT},
     }
    },

@derekbruening
Copy link
Contributor

The kernel writes terminating null but returns only the non-null count.
I checked the corner case: if the buffer is size 4 and there are 4 chars of data, it returns 3 and writes the null.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants