Re: [PATCH] ntdll: round section parameters on 0x200

[Andrey Turkin]

Some comments inside. Sorry for over-quoting :)

Dmitry Timoshkov wrote, on 11/13/06 10:19 MSK:
> "Andrey Turkin"  wrote:
> > > What is the file alignment of the problematic PE file? Is it 512
> > > (0x200) by any chance?
> >
> > Yep. However, I've made some quick tests (that is, I've used PE tools
> > to rebuild some apps with larger file alignment and then tried to
> > change physical offset) and it seems that align cutoff is hard-coded
> > as 0x200.
>
> Well, of course larger alignments are always "aligned" to 0x200.
>
> I took your patch as a base and attached patch make upack 0.39 (latest
> available at its home page http://dwing.51.net/download/WinUpack39.rar)
> executable run under Wine.
>
> diff -up a/dlls/ntdll/virtual.c b/dlls/ntdll/virtual.c
> --- a/dlls/ntdll/virtual.c    2006-11-08 13:55:07.000000000 +0800
> +++ b/dlls/ntdll/virtual.c    2006-11-13 14:57:07.000000000 +0800
> @@ -133,8 +133,10 @@ static UINT_PTR page_mask;
> #define ROUND_ADDR(addr,mask) \
>    ((void *)((UINT_PTR)(addr) & ~(UINT_PTR)(mask)))
>
> -#define ROUND_SIZE(addr,size) \
> - (((UINT)(size) + ((UINT_PTR)(addr) & page_mask) + page_mask) &
>
> ~page_mask)
> +#define ROUND_UP(addr,size,mask) \
> - (((UINT)(size) + ((UINT_PTR)(addr) & (mask)) + (mask)) & ~(mask))
> -
>
> +#define ROUND_SIZE(addr,size) ROUND_UP(addr,size,page_mask)
>
> #define VIRTUAL_DEBUG_DUMP_VIEW(view) \
>     do { if (TRACE_ON(virtual)) VIRTUAL_DumpView(view); } while (0)
> @@ -957,7 +959,7 @@ static NTSTATUS map_image( HANDLE hmappi
>     if (!st.st_size) goto error;
>     header_size = min( header_size, st.st_size );
>     if (map_file_into_view( view, fd, 0, header_size, 0, VPROT_COMMITTED
> | VPROT_READ,
> -
>                        removable ) != STATUS_SUCCESS) goto error;
> -
>                        TRUE ) != STATUS_SUCCESS) goto error;
> dos = (IMAGE_DOS_HEADER *)ptr;
>   nt = (IMAGE_NT_HEADERS *)(ptr + dos->e_lfanew);
>   header_end = ptr + ROUND_SIZE( 0, header_size );
>
> @@ -1031,7 +1033,7 @@ static NTSTATUS map_image( HANDLE hmappi
> for (i = pos = 0; i < nt->FileHeader.NumberOfSections; i++, sec++)
> {
> -
>    SIZE_T map_size, file_size, end;
> -
>    SIZE_T map_size, file_size, start, end;
>
>   if (!sec->Misc.VirtualSize)
>   {
>
> @@ -1045,6 +1047,7 @@ static NTSTATUS map_image( HANDLE hmappi
>         }
>     /* a few sanity checks */
> -
>    start = (SIZE_T) ROUND_ADDR( sec->PointerToRawData,
>
> nt->OptionalHeader.FileAlignment - 1 );

I believe this is incorrect with regard to native Windows loader. As I
already said, I've tried to change PointerToRawData field on some files
with big FileAlignment and they load correctly if I or'ed them with some
value in range 0..0x1FF. Files refuses to load if I or'ed
PointerToRawData with 0x200 or 0x3FF!
>     end = sec->VirtualAddress + ROUND_SIZE( sec->VirtualAddress,
>
> map_size );
>         if (sec->VirtualAddress > total_size || end > total_size || end
> < sec->VirtualAddress)
>         {
> @@ -1095,9 +1098,9 @@ static NTSTATUS map_image( HANDLE hmappi
>         /* Note: if the section is not aligned properly
> map_file_into_view will magically
>          *       fall back to read(), so we don't need to check anything
> here.
>          */
> -
>    end = sec->PointerToRawData + file_size;
> -
>    if (sec->PointerToRawData >= st.st_size || end > st.st_size ||
>
> end < sec->PointerToRawData ||
> -
>        map_file_into_view( view, fd, sec->VirtualAddress,
>
> file_size, sec->PointerToRawData,
> -
>    end = start + file_size;
> -
>    if (start >= st.st_size || end > st.st_size || end < start ||
> -
>        map_file_into_view( view, fd, sec->VirtualAddress,
>
> file_size, start,
>                                 VPROT_COMMITTED | VPROT_READ |
> VPROT_WRITECOPY,
>                                 removable ) != STATUS_SUCCESS)
>         {
> @@ -1107,12 +1110,13 @@ static NTSTATUS map_image( HANDLE hmappi
>     if (file_size & page_mask)
>     {
> -
>        start = ROUND_UP( 0, file_size,
>
> nt->OptionalHeader.FileAlignment - 1 );

I've not checked this, but there's probability that align is 0x200 here too.
>         end = ROUND_SIZE( 0, file_size );
>         if (end > map_size) end = map_size;
>         TRACE_(module)("clearing %p - %p\n",
> -
>                       ptr + sec->VirtualAddress + file_size,
> -
>                       ptr + sec->VirtualAddress + start,
>                      ptr + sec->VirtualAddress + end );
> -
>        memset( ptr + sec->VirtualAddress + file_size, 0, end -
>
> file_size );
> -
>        memset( ptr + sec->VirtualAddress + start, 0, end - start );
>   }
> }
>
> diff -up a/server/mapping.c b/server/mapping.c
> --- a/server/mapping.c    2006-11-03 21:34:24.000000000 +0800
> +++ b/server/mapping.c    2006-11-13 14:34:50.000000000 +0800
> @@ -243,7 +243,7 @@ static int get_image_params( struct mapp
> mapping->size        = ROUND_SIZE( nt.OptionalHeader.SizeOfImage );
> mapping->base        = (void *)nt.OptionalHeader.ImageBase;
> - mapping->header_size = pos + size;
> - mapping->header_size = max( pos + size,
>
> nt.OptionalHeader.SizeOfHeaders );

You're reading my mind (or my hard drive), don't you? :) Ok, but if you
going to change server code here, then you should probably align
PointerToRawData in server code as well (writable shared sections will
be mapped incorrectly otherwise).
> mapping->protect     = VPROT_IMAGE;
>
> /* sanity check */