[PATCH 0/1] MR1637: server: Fix buffer overrun in map_view handler.
Because of padding at the end of the struct, sizeof(*view) is greater than offsetof(struct memory_view, name[0]). Change the allocation to overallocate slightly instead of underallocating slightly. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1637
From: Alex Henrie <alexhenrie24(a)gmail.com> Because of padding at the end of the struct, sizeof(*view) is greater than offsetof(struct memory_view, name[0]). Change the allocation to overallocate slightly instead of underallocating slightly. --- server/mapping.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/mapping.c b/server/mapping.c index 8d4332d240f..ed81a718bbe 100644 --- a/server/mapping.c +++ b/server/mapping.c @@ -1212,7 +1212,7 @@ DECL_HANDLER(map_view) if (!req->mapping) /* image mapping for a .so dll */ { if (get_req_data_size() > sizeof(view->image)) namelen = get_req_data_size() - sizeof(view->image); - if (!(view = mem_alloc( offsetof( struct memory_view, name[namelen] )))) return; + if (!(view = mem_alloc( sizeof(struct memory_view) + namelen * sizeof(WCHAR) ))) return; memset( view, 0, sizeof(*view) ); view->base = req->base; view->size = req->size; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1637
What about using flexible array member? We've fixed similar issues elsewhere that way. Adding a `C_ASSERT( sizeof(struct memory_view) == offsetof(struct memory_view, name[0]) );` next to the struct definition also makes sure it is correctly alignmed. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1637#note_18266
I didn't know about C_ASSERT, thanks for pointing it out to me! I tried changing name[1] to name[] or name[0], but either way the C_ASSERT still failed because MinGW still added four bytes of padding at the end. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1637#note_18288
participants (3)
-
Alex Henrie -
Alex Henrie (@alexhenrie) -
Rémi Bernon