kernel32: Don't set unconditionally SECTION_QUERY flag in OpenFileMapping.

[Jacek Caban]

On 03/11/16 14:44, Dmitry Timoshkov wrote:
> Thanks, I have only style related comments, and since I'm an author of
> existing tests I'd prefer to keep an original style if possible.
>
> Jacek Caban <jacek at codeweavers.com> wrote:
>
>> --- a/dlls/advapi32/tests/security.c
>> +++ b/dlls/advapi32/tests/security.c
>> @@ -5560,17 +5560,23 @@ static void test_filemap_security(void)
>>      char temp_path[MAX_PATH];
>>      char file_name[MAX_PATH];
>>      DWORD ret, i, access;
>> -    HANDLE file, mapping, dup;
>> +    HANDLE file, mapping, dup, created_mapping;
>>      static const struct
>>      {
>> -        int generic, mapped;
>> +        DWORD generic, mapped;
> Please leave 'int' type, it changes nothing.
>
>> +        BOOL open_only;
>>      } map[] =
>>      {
>>          { 0, 0 },
>>          { GENERIC_READ, STANDARD_RIGHTS_READ | SECTION_QUERY | SECTION_MAP_READ },
>>          { GENERIC_WRITE, STANDARD_RIGHTS_WRITE | SECTION_MAP_WRITE },
>>          { GENERIC_EXECUTE, STANDARD_RIGHTS_EXECUTE | SECTION_MAP_EXECUTE },
>> -        { GENERIC_ALL, STANDARD_RIGHTS_REQUIRED | SECTION_ALL_ACCESS }
>> +        { GENERIC_ALL, STANDARD_RIGHTS_REQUIRED | SECTION_ALL_ACCESS },
>> +        { SECTION_MAP_READ | SECTION_MAP_WRITE, SECTION_MAP_READ | SECTION_MAP_WRITE},
>> +        { SECTION_MAP_WRITE, SECTION_MAP_WRITE},
>> +        { SECTION_MAP_READ | SECTION_QUERY, SECTION_MAP_READ | SECTION_QUERY},
>> +        { SECTION_QUERY, SECTION_MAP_READ, TRUE },
>> +        { SECTION_QUERY | SECTION_MAP_READ, SECTION_QUERY | SECTION_MAP_READ }

You missed missing space before '}' in your review.

>>      };
>>      static const struct
>>      {
>> @@ -5599,6 +5605,8 @@ static void test_filemap_security(void)
>>  
>>      for (i = 0; i < sizeof(prot_map)/sizeof(prot_map[0]); i++)
>>      {
>> +        if (map[i].open_only) continue;
>> +
>>          SetLastError(0xdeadbeef);
>>          mapping = CreateFileMappingW(file, NULL, prot_map[i].prot, 0, 4096, NULL);
>>          if (prot_map[i].mapped)
>> @@ -5645,6 +5653,8 @@ static void test_filemap_security(void)
>>  
>>      for (i = 0; i < sizeof(map)/sizeof(map[0]); i++)
>>      {
>> +        if (map[i].open_only) continue;
>> +
>>          SetLastError( 0xdeadbeef );
>>          ret = DuplicateHandle(GetCurrentProcess(), mapping, GetCurrentProcess(), &dup,
>>                                map[i].generic, FALSE, 0);
>> @@ -5659,6 +5669,24 @@ static void test_filemap_security(void)
>>      CloseHandle(mapping);
>>      CloseHandle(file);
>>      DeleteFileA(file_name);
>> +
>> +    created_mapping = CreateFileMappingA( INVALID_HANDLE_VALUE, NULL, PAGE_READWRITE, 0, 0x1000,
>> +                                         "Wine Test Open Mapping");
>> +    ok( created_mapping != NULL, "CreateFileMapping failed with error %u\n", GetLastError() );
>> +
>> +    for(i=0; i < sizeof(map)/sizeof(*map); i++)
> Please add spaces around '=' and use sizeof(map[0]) for consistency with existing
> code.
>> +    {
>> +        if (!map[i].generic) continue;
>> +
>> +        mapping = OpenFileMappingA( map[i].generic, FALSE, "Wine Test Open Mapping");
>> +        ok( mapping != NULL, "OpenFileMapping failed with error %d\n", GetLastError());
>> +        access = get_obj_access( mapping );
>> +        ok( access == map[i].mapped, "[%d] unexpected access flags %x, expected %x\n",
>> +            i, access, map[i].mapped );
> Please use "%d:" instead of [%d] and %#x instead of %x for consistency
> with existing code.
>
> P.S.
> That's perfectly fine to completely ignore my comments, just wanted to show
> how it looks like for comparison with d3d-like acceptance policy :)

As far as I'm concerned such reviews are waste of both your and my time,
no matter whom they are from and right now it's you who gave me such
review. I don't know what you wanted to show me, I don't recall myself
giving such a review ever. I will send a "fixed" version with hope that
I can finally go back to fixing real problems.

Cheers,
Jacek