Applied ZeroMemory to DEVMODE struct in Win32 to prevent Uninitialized…

[anthnich]

… Read.

While working on my game, I ran my executable through Dr.Memory to keep an eye on memory issues. I noticed an Uninitialized Read error being reported where the DEVMODE struct is being used, particularly under the Win32 switchToFullscreen method. I ran the DEVMODE struct through ZeroMemory and it corrected the issue.

Microsoft's DEVMODE documentation states that many of the struct members need to be initialized to something. I browsed the display code of a few other projects, and running DEVMODE through ZeroMemory is common practice.

[eXpl0it3r]

I ran Dr. Memory myself with a modified version of the opengl example, to use fullscreen. But while I see the UNINITIALIZED READ errors go away, I see two new errors popup about the same code.

Before this PR

Error #1: UNINITIALIZED READ: reading 0x00bef364-0x00bef3a8 68 byte(s) within 0x00bef364-0x00bef3ac
# 0 system call NtUserChangeDisplaySettings DEVMODEW through dmDriverExtra
# 1 USER32.dll!ChangeDisplaySettingsExW                                       +0x4d     (0x7617dd0e <USER32.dll+0x7dd0e>)
# 2 USER32.dll!ChangeDisplaySettingsW                                         +0x1f     (0x7617de20 <USER32.dll+0x7de20>)
# 3 sf::priv::WindowImplWin32::switchToFullscreen                              [D:/Programming/C++/Contrib/SFML/src/SFML/Window/Win32/WindowImplWin32.cpp:494]
# 4 sf::priv::WindowImplWin32::WindowImplWin32                                 [D:/Programming/C++/Contrib/SFML/src/SFML/Window/Win32/WindowImplWin32.cpp:234]
# 5 sf::priv::WindowImpl::create                                               [D:/Programming/C++/Contrib/SFML/src/SFML/Window/WindowImpl.cpp:71]
# 6 sf::Window::create                                                         [D:/Programming/C++/Contrib/SFML/src/SFML/Window/Window.cpp:124]
# 7 sf::RenderWindow::RenderWindow                                             [D:/Programming/C++/Contrib/SFML/src/SFML/Graphics/RenderWindow.cpp:45]
# 8 main                                                                       [D:/Programming/C++/Contrib/SFML/examples/opengl/OpenGL.cpp:32]
Note: @0:00:18.419 in thread 11796

Error #2: UNINITIALIZED READ: reading 0x00bef3b0-0x00bef3ca 26 byte(s) within 0x00bef3ac-0x00bef3ca
# 0 system call NtUserChangeDisplaySettings DEVMODEW dmFields through dmCollate
# 1 USER32.dll!ChangeDisplaySettingsExW                                       +0x4d     (0x7617dd0e <USER32.dll+0x7dd0e>)
# 2 USER32.dll!ChangeDisplaySettingsW                                         +0x1f     (0x7617de20 <USER32.dll+0x7de20>)
# 3 sf::priv::WindowImplWin32::switchToFullscreen                              [D:/Programming/C++/Contrib/SFML/src/SFML/Window/Win32/WindowImplWin32.cpp:494]
# 4 sf::priv::WindowImplWin32::WindowImplWin32                                 [D:/Programming/C++/Contrib/SFML/src/SFML/Window/Win32/WindowImplWin32.cpp:234]
# 5 sf::priv::WindowImpl::create                                               [D:/Programming/C++/Contrib/SFML/src/SFML/Window/WindowImpl.cpp:71]
# 6 sf::Window::create                                                         [D:/Programming/C++/Contrib/SFML/src/SFML/Window/Window.cpp:124]
# 7 sf::RenderWindow::RenderWindow                                             [D:/Programming/C++/Contrib/SFML/src/SFML/Graphics/RenderWindow.cpp:45]
# 8 main                                                                       [D:/Programming/C++/Contrib/SFML/examples/opengl/OpenGL.cpp:32]
Note: @0:00:18.420 in thread 11796

Error #3: UNINITIALIZED READ: reading 0x00bef3ca-0x00bef40c 66 byte(s) within 0x00bef3ca-0x00bef440
# 0 system call NtUserChangeDisplaySettings DEVMODEW dmFormName onward
# 1 USER32.dll!ChangeDisplaySettingsExW                                       +0x4d     (0x7617dd0e <USER32.dll+0x7dd0e>)
# 2 USER32.dll!ChangeDisplaySettingsW                                         +0x1f     (0x7617de20 <USER32.dll+0x7de20>)
# 3 sf::priv::WindowImplWin32::switchToFullscreen                              [D:/Programming/C++/Contrib/SFML/src/SFML/Window/Win32/WindowImplWin32.cpp:494]
# 4 sf::priv::WindowImplWin32::WindowImplWin32                                 [D:/Programming/C++/Contrib/SFML/src/SFML/Window/Win32/WindowImplWin32.cpp:234]
# 5 sf::priv::WindowImpl::create                                               [D:/Programming/C++/Contrib/SFML/src/SFML/Window/WindowImpl.cpp:71]
# 6 sf::Window::create                                                         [D:/Programming/C++/Contrib/SFML/src/SFML/Window/Window.cpp:124]
# 7 sf::RenderWindow::RenderWindow                                             [D:/Programming/C++/Contrib/SFML/src/SFML/Graphics/RenderWindow.cpp:45]
# 8 main                                                                       [D:/Programming/C++/Contrib/SFML/examples/opengl/OpenGL.cpp:32]
Note: @0:00:18.422 in thread 11796

After this PR

Error #1: UNADDRESSABLE ACCESS beyond heap bounds: writing 0x03b40188-0x03b4018d 5 byte(s) within 0x03b40180-0x03b4018d
# 0 system call NtUserChangeDisplaySettings parameter #4
# 1 USER32.dll!ChangeDisplaySettingsExW                               +0x4d     (0x7617dd0e <USER32.dll+0x7dd0e>)
# 2 USER32.dll!ChangeDisplaySettingsW                                 +0x1f     (0x7617de20 <USER32.dll+0x7de20>)
# 3 sf::priv::WindowImplWin32::switchToFullscreen                      [D:/Programming/C++/Contrib/SFML/src/SFML/Window/Win32/WindowImplWin32.cpp:495]
# 4 sf::priv::WindowImplWin32::WindowImplWin32                         [D:/Programming/C++/Contrib/SFML/src/SFML/Window/Win32/WindowImplWin32.cpp:234]
# 5 sf::priv::WindowImpl::create                                       [D:/Programming/C++/Contrib/SFML/src/SFML/Window/WindowImpl.cpp:71]
# 6 sf::Window::create                                                 [D:/Programming/C++/Contrib/SFML/src/SFML/Window/Window.cpp:124]
# 7 sf::RenderWindow::RenderWindow                                     [D:/Programming/C++/Contrib/SFML/src/SFML/Graphics/RenderWindow.cpp:45]
# 8 main                                                               [D:/Programming/C++/Contrib/SFML/examples/opengl/OpenGL.cpp:32]
Note: @0:00:16.715 in thread 1648
Note: next higher malloc: 0x03b401a8-0x03b401b5
Note: refers to 0 byte(s) beyond last valid byte in prior malloc
Note: prev lower malloc:  0x03b40180-0x03b40188

Error #2: UNADDRESSABLE ACCESS beyond heap bounds: writing 0x03b40188-0x03b4018d 5 byte(s) within 0x03b40180-0x03b4018d
# 0 system call NtUserChangeDisplaySettings parameter #4
# 1 USER32.dll!ChangeDisplaySettingsExW                               +0x4d     (0x7617dd0e <USER32.dll+0x7dd0e>)
# 2 USER32.dll!ChangeDisplaySettingsW                                 +0x1f     (0x7617de20 <USER32.dll+0x7de20>)
# 3 sf::priv::WindowImplWin32::switchToFullscreen                      [D:/Programming/C++/Contrib/SFML/src/SFML/Window/Win32/WindowImplWin32.cpp:495]
# 4 sf::priv::WindowImplWin32::WindowImplWin32                         [D:/Programming/C++/Contrib/SFML/src/SFML/Window/Win32/WindowImplWin32.cpp:234]
# 5 sf::priv::WindowImpl::create                                       [D:/Programming/C++/Contrib/SFML/src/SFML/Window/WindowImpl.cpp:71]
# 6 sf::Window::create                                                 [D:/Programming/C++/Contrib/SFML/src/SFML/Window/Window.cpp:124]
# 7 sf::RenderWindow::RenderWindow                                     [D:/Programming/C++/Contrib/SFML/src/SFML/Graphics/RenderWindow.cpp:45]
# 8 main                                                               [D:/Programming/C++/Contrib/SFML/examples/opengl/OpenGL.cpp:32]
Note: @0:00:16.726 in thread 1648
Note: next higher malloc: 0x03b401a8-0x03b401b5
Note: refers to 0 byte(s) beyond last valid byte in prior malloc
Note: prev lower malloc:  0x03b40180-0x03b40188

As such, I wonder whether just zero-ing everything is the best option?

Additionally I noticed another uninitialized read for the TRACKMOUSEEVENT struct. Using ZeroMemory on the struct seems to removed the error and doesn't seem to create an additional error.

[binary1248]

For both the EnumDisplaySettings calls, the ZeroMemory is unnecessary. MSDN documentation says that its dmSize has to be set to sizeof(DEVMODE) and dmDriverExtra has to be set to (in our case) 0. Since this is an out parameter, Windows is the one that would zero out the structure's memory, not us. Since EnumDisplaySettings only sets several of the fields as per the documentation, the rest of the memory is left untouched and should not be read by us either.

The same two fields as above also have to be set when calling ChangeDisplaySettings, however, just for consistency reasons, we should probably pass a DEVMODEW structure, since we are calling the ChangeDisplaySettingsW variant of the function. DEVMODE is DEVMODEW in our case anyway, but we can do it just to be sure. As with EnumDisplaySettings calling ZeroMemory is unnecessary here as well as long as all the right fields are set to the right values.

[anthnich]

For both the EnumDisplaySettings calls, the ZeroMemory is unnecessary.

For EnumDisplaySettings and ChangeDisplaySettingsW , I removed ZeroMemory and added devMode.dmDriverExtra = 0; and it yielded UNINITIALIZED READ with these results.

# 0 system call NtUserChangeDisplaySettings DEVMODEW through dmDriverExtra
# 0 system call NtUserChangeDisplaySettings DEVMODEW dmFields through dmCollate
# 0 system call NtUserChangeDisplaySettings DEVMODEW dmFormName onward

I get the UNADDRESSABLE ACCESS regardless if I ZeroMemory DEVMODE or not. Frankly, Dr.Memory could be a stickler with this whole thing.

[binary1248]

From http://drmemory.org/docs/page_uninit.html:

If the application reads from addressable memory that has not been written to since it was allocated, Dr. Memory reports an "uninitialized read" error. In order to avoid false positives, Dr. Memory does not report the use of uninitialized memory until something "meaningful" is done with that memory, such as a comparison or conditional branch or passing it to a system call. Variables or fields smaller than a word are often initialized without their containing word (variables and fields are typically word-aligned) being initialized. When these variables or fields are then copied, the uninitialized portion of each word is technically being read as an uninitialized value, but reporting such reads as errors would result in far too many errors.

When passing data structures to a system call, if the structure is initialized field-by-field then padding bytes may be left uninitialized. Dr. Memory will report errors on these as it does not know whether the kernel or a receipient on the other end might read it. To avoid these errors, memset the entire structure, or use a Dr. Memory error suppression (see Suppressing Errors) to ignore the error.

Because user mode applications (including Dr. Memory) can't peek inside kernel mode execution to determine what is and isn't done with memory, Dr. Memory simply assumes that the entire structure is going to be read. In the case of DEVMODE this simply isn't true. The documentation clearly states that this will lead to false positives, and the only options are to either zero out the memory in user code or add the system call to the list of suppressions. Because assuming system calls will always read entire structures sounds a bit too paranoid for me, I tend towards the latter option.

This however doesn't change the fact that we are still missing setting dmDriverExtra to 0.

[anthnich]

Thanks, that was informative.

Reverted ZeroMemory and added dmDriverExtra where needed.

[eXpl0it3r]

Can you squash the two commits?

[eXpl0it3r]

Merged in 23a3455