September 2023 - Wine-gitlab

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

18 Sep '23

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) 18 Sep '23

18 Sep '23

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) 18 Sep '23

18 Sep '23

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) 18 Sep '23

18 Sep '23

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) 18 Sep '23

18 Sep '23

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) 18 Sep '23

18 Sep '23

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) 18 Sep '23

18 Sep '23

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) 18 Sep '23

18 Sep '23

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

[PATCH v4 0/2] MR3259: Add environment variable to isolate WINEPREFIX at creation
by Fabian Maurer (＠DarkShadow44) 18 Sep '23

18 Sep '23

I'm aware that winecfg allows unlinking of symlinked folders, but this is very annoying when regularly recreating WINEPREFIXES. This change makes it easy to isolate those folders from the start. -- v4: ntdll: Add environment variable to prevent symlinking dosdevices/z: shell32: Add environment variable to prevent symlinking home folders https://gitlab.winehq.org/wine/wine/-/merge_requests/3259

2 2

[PATCH v5 0/2] MR3151: widl: Prevent infinite loop when structure contains array of itself
by Fabian Maurer (＠DarkShadow44) 18 Sep '23

18 Sep '23

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=55115 -- v5: widl: In type_has_full_pointer recurse for pointer types as well widl: Prevent infinite loop when structure contains array of itself https://gitlab.winehq.org/wine/wine/-/merge_requests/3151

2 3