[PATCH v2 0/1] MR4414: server: Fully initialize pe mapping struct to avoid uninitialized memory when...

[Fabian Maurer (＠DarkShadow44)]

When running wineserver under valgrind there is a bunch of uninitialised bytes warnings when sending the req_get_mapping_info reply.
This warning can easily be reproduced by running `valgrind wineserver -f` and then `winecfg`.
Tested using self compiled wine.

Reason is the `pe_image_info_t` struct having bitfields:
```
unsigned char  contains_code : 1;
unsigned char  wine_builtin : 1;
unsigned char  wine_fakedll : 1;
```
The last 5 bits are never initialized. This MR fixes this by zeroing the entire struct before it is filled.

I hope "issues" like this are allowed to be fixed so valgrind has less false positives.

Valgrind message for reference:
```
==628662== 716 errors in context 3 of 3:
==628662== Syscall param writev(vector[1]) points to uninitialised byte(s)
==628662==    at 0x49C3A87: writev (writev.c:26)
==628662==    by 0x14C2F3: send_reply (request.c:268)
==628662==    by 0x14C50F: call_req_handler (request.c:316)
==628662==    by 0x14C5D2: read_request (request.c:339)
==628662==    by 0x158F52: thread_poll_event (thread.c:388)
==628662==    by 0x120FC9: fd_poll_event (fd.c:505)
==628662==    by 0x1212A6: main_loop_epoll (fd.c:599)
==628662==    by 0x121882: main_loop (fd.c:955)
==628662==    by 0x12CBE0: main (main.c:238)
==628662==  Address 0x53ec56e is 62 bytes inside a block of size 88 alloc'd
==628662==    at 0x48416D4: malloc (vg_replace_malloc.c:431)
==628662==    by 0x1348D0: mem_alloc (object.c:119)
==628662==    by 0x14BEB9: set_reply_data_size (request.c:160)
==628662==    by 0x1301C1: req_get_mapping_info (mapping.c:1291)
==628662==    by 0x14C494: call_req_handler (request.c:305)
==628662==    by 0x14C5D2: read_request (request.c:339)
==628662==    by 0x158F52: thread_poll_event (thread.c:388)
==628662==    by 0x120FC9: fd_poll_event (fd.c:505)
==628662==    by 0x1212A6: main_loop_epoll (fd.c:599)
==628662==    by 0x121882: main_loop (fd.c:955)
==628662==    by 0x12CBE0: main (main.c:238)
==628662==  Uninitialised value was created by a client request
==628662==    at 0x134873: mark_block_uninitialized (object.c:110)
==628662==    by 0x1348EA: mem_alloc (object.c:120)
==628662==    by 0x134B87: alloc_object (object.c:199)
==628662==    by 0x134F92: create_named_object (object.c:337)
==628662==    by 0x12F12B: create_mapping (mapping.c:939)
==628662==    by 0x12FFE9: req_create_mapping (mapping.c:1250)
==628662==    by 0x14C494: call_req_handler (request.c:305)
==628662==    by 0x14C5D2: read_request (request.c:339)
==628662==    by 0x158F52: thread_poll_event (thread.c:388)
==628662==    by 0x120FC9: fd_poll_event (fd.c:505)
==628662==    by 0x1212A6: main_loop_epoll (fd.c:599)
==628662==    by 0x121882: main_loop (fd.c:955)
```

--
  v2: server: Fully initialize pe mapping struct to avoid uninitialized memory when sending req_get_mapping_info reply (valgrind)
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/4414

[PATCH v2 1/1] server: Fully initialize pe mapping struct to avoid uninitialized memory when sending req_get_mapping_info reply (valgrind)

[Fabian Maurer]

From: Fabian Maurer <dark.shadow4(a)web.de>

---
 include/wine/server_protocol.h | 1 +
 server/mapping.c               | 1 +
 2 files changed, 2 insertions(+)

diff --git a/include/wine/server_protocol.h b/include/wine/server_protocol.h
index 15156a12353..444cd9c7fa0 100644
--- a/include/wine/server_protocol.h
+++ b/include/wine/server_protocol.h
@@ -844,6 +844,7 @@ typedef struct
     unsigned char  contains_code : 1;
     unsigned char  wine_builtin : 1;
     unsigned char  wine_fakedll : 1;
+    unsigned char  padding : 5; /* to avoid uninitialized memory using valgrind */
     unsigned char  image_flags;
     unsigned int   loader_flags;
     unsigned int   header_size;
diff --git a/server/mapping.c b/server/mapping.c
index a795dc4b38b..b5684c484d2 100644
--- a/server/mapping.c
+++ b/server/mapping.c
@@ -842,6 +842,7 @@ static unsigned int get_image_params( struct mapping *mapping, file_pos_t file_s
         return STATUS_INVALID_IMAGE_FORMAT;
     }
 
+    mapping->image.padding       = 0;
     mapping->image.map_addr      = get_fd_map_address( mapping->fd );
     mapping->image.image_charact = nt.FileHeader.Characteristics;
     mapping->image.machine       = nt.FileHeader.Machine;
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/4414

[Fabian Maurer (＠DarkShadow44)]

On Tue Nov 28 20:13:01 2023 +0000, Fabian Maurer wrote:
> Seemed the cleanest way to me. I mean, there's no way to take the
> address of a bitfield, so how else should I do it?
> Reworked

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/4414#note_54437