September 2023 - Wine-GitLab

Re: [PATCH v4 0/3] MR3870: mspatcha: Add support for 32-bit file patching and normalization
by Jeffrey Smith (＠whydoubt) Sept. 18, 2023

Sept. 18, 2023

On Mon Sep 18 17:52:49 2023 +0000, Zebediah Figura wrote: > Why are we adding one here? That doesn't look right. I think I see what the intent is, but it is unnecessarily 'clever'. It would be clearer to use something like a `BOOL modified`, and ultimately `return modified ? NORMALIZE_RESULT_SUCCESS_MODIFIED : NORMALIZE_RESULT_SUCCESS;` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3870#note_45774

1 0

[PATCH v3 0/3] MR3855: Fixes for ntlea
by Yuxuan Shui (＠yshui) Sept. 18, 2023

Sept. 18, 2023

Make functions ntlea needs to patch hotpatchable. And add a asm wrapper for `GetWindowLongA` to workaround an assumption ntlea made. -- v3: user32: add hotpatchable wrapper for GetWindowLongA https://gitlab.winehq.org/wine/wine/-/merge_requests/3855

3 6

Re: [PATCH v4 0/3] MR3870: mspatcha: Add support for 32-bit file patching and normalization
by Zebediah Figura (＠zfigura) Sept. 18, 2023

Sept. 18, 2023

Zebediah Figura (@zfigura) commented about dlls/mspatcha/pecoff.h: > +ULONG image_rva_to_file_offset( > + IMAGE_NT_HEADERS32 UNALIGNED *nt_headers, ULONG rva, PUCHAR image_base, ULONG image_size); > + > +ULONG image_directory_rva_and_size( > + IMAGE_NT_HEADERS32 UNALIGNED *nt_header, USHORT directory_entry, ULONG *directory_size, > + PUCHAR image_base, ULONG image_size); > + > +ULONG image_directory_offset_and_size( > + IMAGE_NT_HEADERS32 UNALIGNED *nt_header, USHORT directory_entry, ULONG *directory_size, > + PUCHAR image_base, ULONG image_size); > + > +PVOID image_rva_to_mapped_address( > + IMAGE_NT_HEADERS32 UNALIGNED *nt_headers, ULONG rva, PVOID image_base, ULONG image_size); > + > +PVOID image_directory_mapped_address( > + IMAGE_NT_HEADERS32 UNALIGNED *nt_headers, USHORT directory_entry, ULONG *directory_size, I don't think we need another C or header file for these. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3870#note_45765

1 0

Re: [PATCH v4 0/3] MR3870: mspatcha: Add support for 32-bit file patching and normalization
by Zebediah Figura (＠zfigura) Sept. 18, 2023

Sept. 18, 2023

Zebediah Figura (@zfigura) commented about dlls/mspatcha/pa19.c: > + } > + } > + > + for (i = 0; i < retain_range_count; ++i) { > + if (retain_range_array[i].OffsetInOldFile <= old_file_size > + && (retain_range_array[i].OffsetInOldFile + retain_range_array[i].LengthInBytes) <= old_file_size) > + { > + memset(&old_file_buffer[retain_range_array[i].OffsetInOldFile], > + 0, > + retain_range_array[i].LengthInBytes); > + result = NORMALIZE_RESULT_SUCCESS; > + } > + } > + } > + > + return result + 1; Why are we adding one here? That doesn't look right. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3870#note_45764

1 0

Re: [PATCH v4 0/3] MR3870: mspatcha: Add support for 32-bit file patching and normalization
by Zebediah Figura (＠zfigura) Sept. 18, 2023

Sept. 18, 2023

Zebediah Figura (@zfigura) commented about dlls/mspatcha/pa19.c: > + BYTE *old_file_buffer, ULONG old_file_size, > + ULONG option_flags, PATCH_OPTION_DATA *option_data, > + ULONG new_image_base, ULONG new_image_time, > + const PATCH_IGNORE_RANGE *ignore_range_array, ULONG ignore_range_count, > + const PATCH_RETAIN_RANGE *retain_range_array, ULONG retain_range_count) > +{ > + int result = 0; > + ULONG i; > + IMAGE_NT_HEADERS32 UNALIGNED *nt_headers; > + > + UNREFERENCED_PARAMETER(option_data); > + > + TRACE("normalizing image at 0x%p with options 0x%lX, new base 0x%lX, new time %lu", > + old_file_buffer, option_flags, new_image_base, new_image_time); > + > + if (old_file_buffer && old_file_size) Early return would save you a level of indentation here. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3870#note_45763

1 0

Re: [PATCH v4 0/3] MR3870: mspatcha: Add support for 32-bit file patching and normalization
by Zebediah Figura (＠zfigura) Sept. 18, 2023

Sept. 18, 2023

Zebediah Figura (@zfigura) commented about dlls/mspatcha/pa19.c: > + if (size_in_bytes & 1) { > + sum += *buffer; > + } > + > + return (WORD)(HIWORD(sum) + LOWORD(sum)); > +} > + > +/* Normalizes a given 32-bit PE image to render a stream that is common. */ > +int normalize_old_file_image( > + BYTE *old_file_buffer, ULONG old_file_size, > + ULONG option_flags, PATCH_OPTION_DATA *option_data, > + ULONG new_image_base, ULONG new_image_time, > + const PATCH_IGNORE_RANGE *ignore_range_array, ULONG ignore_range_count, > + const PATCH_RETAIN_RANGE *retain_range_array, ULONG retain_range_count) > +{ > + int result = 0; You defined symbolic constants; why not use them here? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3870#note_45762

1 0

Re: [PATCH v4 0/3] MR3870: mspatcha: Add support for 32-bit file patching and normalization
by Zebediah Figura (＠zfigura) Sept. 18, 2023

Sept. 18, 2023

Zebediah Figura (@zfigura) commented about dlls/mspatcha/pa19.c: > + return (WORD)(HIWORD(sum) + LOWORD(sum)); > +} > + > +/* Normalizes a given 32-bit PE image to render a stream that is common. */ > +int normalize_old_file_image( > + BYTE *old_file_buffer, ULONG old_file_size, > + ULONG option_flags, PATCH_OPTION_DATA *option_data, > + ULONG new_image_base, ULONG new_image_time, > + const PATCH_IGNORE_RANGE *ignore_range_array, ULONG ignore_range_count, > + const PATCH_RETAIN_RANGE *retain_range_array, ULONG retain_range_count) > +{ > + int result = 0; > + ULONG i; > + IMAGE_NT_HEADERS32 UNALIGNED *nt_headers; > + > + UNREFERENCED_PARAMETER(option_data); We don't need this. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3870#note_45761

1 0

Re: [PATCH v4 0/3] MR3870: mspatcha: Add support for 32-bit file patching and normalization
by Zebediah Figura (＠zfigura) Sept. 18, 2023

Sept. 18, 2023

Zebediah Figura (@zfigura) commented about dlls/mspatcha/pa19.c: > + ++import_desc; > + } > + } > + > + /* Mark the .idata section as writable */ > + section_table = IMAGE_FIRST_SECTION(nt_headers); > + section_count = nt_headers->FileHeader.NumberOfSections; > + for (ULONG i = 0; i < section_count; i++) > + { > + if ((PUCHAR)&section_table[i] < mapped_image > + || (PUCHAR)&section_table[i] > (mapped_image_end - sizeof(IMAGE_SECTION_HEADER))) { > + throw_pe_fmt_exception(); > + } > + > + /* tolower hack check for ".idata " */ > + if ((*((ULONGLONG UNALIGNED *)&section_table[i].Name) | 0x2020202020202020) == 0x202061746164692E) { Why is this a hack? (Why are we not doing a string comparison?) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3870#note_45760

1 0

Re: [PATCH v4 0/3] MR3870: mspatcha: Add support for 32-bit file patching and normalization
by Zebediah Figura (＠zfigura) Sept. 18, 2023

Sept. 18, 2023

Zebediah Figura (@zfigura) commented about dlls/mspatcha/pa19.c: > + } > + > + nt_headers->OptionalHeader.ImageBase = (DWORD)new_image_base; > + result = TRUE; > + > + reloc_dir_remaining = (LONG)reloc_dir_size; > + while (reloc_dir_remaining > 0) { > + > + if (reloc_block->SizeOfBlock <= (ULONG)reloc_dir_remaining > + && reloc_block->SizeOfBlock > sizeof(IMAGE_BASE_RELOCATION)) > + { > + reloc_block_base = (IMAGE_BASE_RELOCATION UNALIGNED *)(mapped_image_base + > + image_rva_to_file_offset(nt_headers, reloc_block->VirtualAddress, mapped_image_base, mapped_image_size)); > + if (reloc_block_base) > + { > + reloc = (USHORT UNALIGNED *)((PUCHAR)reloc_block + sizeof(IMAGE_BASE_RELOCATION)); I don't think we want UNALIGNED, not least because it doesn't do anything on non-MSVC compilers. If any of these actually can be unaligned (can they?) then I think we should just use memcpy. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3870#note_45759

1 0

Re: [PATCH v4 0/3] MR3870: mspatcha: Add support for 32-bit file patching and normalization
by Zebediah Figura (＠zfigura) Sept. 18, 2023

Sept. 18, 2023

Zebediah Figura (@zfigura) commented about dlls/mspatcha/mspatcha_main.c: > { > - FIXME("stub - %p, %lu, %lx, %p, %lu, %lu, %lu, %p, %lu, %p\n", file_buffer, file_size, flags, options, new_coff_base, > - new_coff_time, ignore_range_count, ignore_range, retain_range_count, retain_range); > - SetLastError(ERROR_CALL_NOT_IMPLEMENTED); > - return 0; > + int result; > + > + result = normalize_old_file_image( > + file_buffer, file_size, > + option_flags, option_data, > + new_coff_base, new_coff_time, > + ignore_range_array, ignore_range_count, > + retain_range_array, retain_range_count); > + > + return result; > } The column limit is larger than 50 characters. No need to reflow to make it shorter. I'd advocate for keeping a trace in any API entry point. The 'result' variable is useless. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3870#note_45758

1 0