[PATCH 1/2] wpcap: Implement pcap_dump_open and pcap_dump

[Sebastian Lackner]

You get ERROR_INVALID_PARAMETER because you are trying to free a string not allocated
with heap functions. The RtlFreeAnsiString you wrote below is unnecessary. This is not
the only mistake though - I'm aware that you are still working on it, nevertheless I
thought it might be useful for you to point them out, to make you more familiar with
the (sometimes a bit strict) Wine coding guidelines. As a general remark, I would
suggest to look at other examples how some API functions are used, to get more familiar
with them, and to look at the example I provided on IRC yesterday:
http://pastebin.com/raw/m43tkLdT

Below are some inline comments:

On 10.02.2016 14:59, Jianqiu Zhang wrote:
> 
> Hi, sorry for the late reply
> 
> I have met some problem when implementing wine_pcap_dump_open 
> My wine_pcap_dump_open code goes like this
> 
> --snip--
> 
> pcap_dumper_t* CDECL wine_pcap_dump_open(pcap_t *p, const char *fname)
> {
>     UNICODE_STRING nt_name, dospathW;
>     ANSI_STRING fname_dos;
>     ANSI_STRING fname_unix;
>     int res;

Please always use variables with the proper type (NTSTATUS or BOOL or ...).

>     fname_unix.Buffer = NULL;
>     RtlInitAnsiString(&fname_dos, fname);
>     RtlInitAnsiString(&fname_unix, NULL);

The line above is not necessary.

>     res = RtlAnsiStringToUnicodeString(&dospathW, &fname_dos, TRUE);

There should be an error check here and you should set last error if appropriate.

>     res = RtlDosPathNameToNtPathName_U(dospathW.Buffer, &nt_name, NULL, NULL);
>     if(!res)
>     {

You should release the dospathW unicode string here and set an error.

>         printf("RtlDosPathNameToNtPathName_U failed\n");
>         return NULL;
>     }
>     res  = wine_nt_to_unix_file_name(&nt_name, &fname_unix, FILE_OPEN_IF, FALSE);
>     printf("#1 ERRCODE is %X\n", GetLastError());
>     printf("VOID_DEBUG: Nt FileName is %s\n", wine_dbgstr_w(nt_name.Buffer));
>     if(res != 0 && res != 0xC000000F)

Please always use STATUS_* variables instead of hardcoded numbers.

>     {
>         SetLastError(0xB7);

Same here. Code to release allocated memory is also missing.

>         printf("wine_nt_to_unix_file_name failed\n");
>         printf("#2 ERRCODE is %X\n", GetLastError());
>         return NULL;
>     }
>     RtlFreeUnicodeString(&nt_name);
>     printf("#3 ERRCODE is %X\n", GetLastError());
>     RtlFreeAnsiString(&fname_dos);

RtlInitAnsiString does not duplicate the string. You get an ERROR_INVALID_PARAMETER
error here because you are trying to free a string not allocated with heap functions.

>     //HeapFree(GetProcessHeap(), 0, fname_dos.Buffer);
>     printf("#4 ERRCODE is %X\n", GetLastError());
>     //SetLastError(0xB7);
>     return pcap_dump_open(p, fname_unix.Buffer);

You are leaking memory of fname_unix here. In contrast to
fname_dos, it is allocated on the heap.

> }
> 
> --snip--
> 
> And I modify the test program source code , the source is attached in the attachment(test.c)
> 
> And I run the same test both on win32(XP) and wine , Below are my tests:
> 
> 1. test.exe "AFileNameDoesNotExist" on wine it gets errorcode 0x57 on Windows it gets errorcode 0x00, Both get the correct dumpfile
> 2. test.exe "NameThatExists" on wine it gets errorcode 0x57, on windows it gets errorcode 0xB7, both overwrite the previous file and get the correct dumpfile
> 
> And I dig into the code found RtlFreeAnsiString calls two functions RtlFreeHeap and RtlZeroMemory both give the errorcode 0x57(ERROR_INVALID_PARAMETER) , I wonder why this happens
> If it's a undefined behavior, can I use SetLastError(0) to Manually reset the error code? Thanks in advance
> 
> 
> 
>