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

[André Hentschel]

Hi,

Sorry for the delay, but I haven't noticed that I got assigned for reviewing.
Could you please tell me the name of the application so I can do some testing?
Further reviewing inlined below:

Am 07.01.2016 um 09:49 schrieb Jianqiu Zhang:
> +
> +pcap_dumper_t* CDECL wine_pcap_dump_open(pcap_t *p, const char *fname)
> +{
> +    return pcap_dump_open(p, fname);
> +}

This can't work, the wpcap function might be called with fname set to "C:\dump.pcap" and the native function will be confused by that.
> +void CDECL wine_pcap_dump(u_char *user, const struct pcap_pkthdr *h, const u_char *sp)
> +{
> +    return pcap_dump(user, h, sp);
> +}

This should work, but where are pcap_dump_close and friends?
> diff --git a/dlls/wpcap/wpcap.spec b/dlls/wpcap/wpcap.spec
> index 0e1e208..d7efd5e 100644
> --- a/dlls/wpcap/wpcap.spec
> +++ b/dlls/wpcap/wpcap.spec
> @@ -16,12 +16,12 @@
> @ cdecl pcap_datalink_val_to_description(long) wine_pcap_datalink_val_to_description
> @ cdecl pcap_datalink_val_to_name(long) wine_pcap_datalink_val_to_name
> @ cdecl pcap_dispatch(ptr long ptr ptr) wine_pcap_dispatch
> -@ stub pcap_dump
> +@ cdecl pcap_dump(str ptr str) wine_pcap_dump

Should be (ptr ptr ptr), there are no strings to be displayed

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

[Jianqiu Zhang]

Hi André
Thanks for your review
> Sorry for the delay, but I haven't noticed that I got assigned for reviewing.
> Could you please tell me the name of the application so I can do some testing?
> Further reviewing inlined below:

It's just a small demo program using pcap_dump , I will provide the program and source code in the attachment
> Am 07.01.2016 um 09:49 schrieb Jianqiu Zhang:
> > +
> > +pcap_dumper_t* CDECL wine_pcap_dump_open(pcap_t *p, const char *fname)
> > +{
> > +    return pcap_dump_open(p, fname);
> > +}
>
> This can't work, the wpcap function might be called with fname set to "C:\dump.pcap" and the native function will be confused by that.

Ah, I see, I didn't take this scheme in to consideration, I will try to use a helper function to convert the fname to UNIX fname
Do you think it's a solution?
> > +void CDECL wine_pcap_dump(u_char *user, const struct pcap_pkthdr *h, const u_char *sp)
> > +{
> > +    return pcap_dump(user, h, sp);
> > +}
>
> This should work, but where are pcap_dump_close and friends?

I just implement the stubs in that small program, and it doesn't contain pcap_dump_close and others
Could I ask you how to find the friends of a certain function? I will implement them then
> > +@ cdecl pcap_dump(str ptr str) wine_pcap_dump
>
> Should be (ptr ptr ptr), there are no strings to be displayed

I will correct this in my next try :)

Jianqiu Zhang

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

[André Hentschel]

Hi,

Am 26.01.2016 um 03:54 schrieb Jianqiu Zhang:
> Hi André
> Thanks for your review
> > Sorry for the delay, but I haven't noticed that I got assigned for reviewing.
> > Could you please tell me the name of the application so I can do some testing?
> > Further reviewing inlined below:
>
> It's just a small demo program using pcap_dump , I will provide the program and source code in the attachment

Thx
> > Am 07.01.2016 um 09:49 schrieb Jianqiu Zhang:
> > > +
> > > +pcap_dumper_t* CDECL wine_pcap_dump_open(pcap_t *p, const char *fname)
> > > +{
> > > +    return pcap_dump_open(p, fname);
> > > +}
> >
> > This can't work, the wpcap function might be called with fname set to "C:\dump.pcap" and the native function will be confused by that.
>
> Ah, I see, I didn't take this scheme in to consideration, I will try to use a helper function to convert the fname to UNIX fname
> Do you think it's a solution?

Wine needs to do that in other functions, have a look for wine_nt_to_unix_file_name
> > > +void CDECL wine_pcap_dump(u_char *user, const struct pcap_pkthdr *h, const u_char *sp)
> > > +{
> > > +    return pcap_dump(user, h, sp);
> > > +}
> >
> > This should work, but where are pcap_dump_close and friends?
>
> I just implement the stubs in that small program, and it doesn't contain pcap_dump_close and others
> Could I ask you how to find the friends of a certain function? I will implement them then

I mean:
pcap_dump_ftell
pcap_dump_file
pcap_dump_flush

But I think those should go into a separate patch, but pcap_dump_close should go in this one...
> > > +@ cdecl pcap_dump(str ptr str) wine_pcap_dump
> >
> > Should be (ptr ptr ptr), there are no strings to be displayed
>
> I will correct this in my next try :)
>
> Jianqiu Zhang

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

[Jianqiu Zhang]

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;
    fname_unix.Buffer = NULL;
    RtlInitAnsiString(&fname_dos, fname);
    RtlInitAnsiString(&fname_unix, NULL);
    res = RtlAnsiStringToUnicodeString(&dospathW, &fname_dos, TRUE);
    res = RtlDosPathNameToNtPathName_U(dospathW.Buffer, &nt_name, NULL, NULL);
    if(!res)
    {
        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)
    {
        SetLastError(0xB7);
        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);
    //HeapFree(GetProcessHeap(), 0, fname_dos.Buffer);
    printf("#4 ERRCODE is %X\n", GetLastError());
    //SetLastError(0xB7);
    return pcap_dump_open(p, fname_unix.Buffer);
}

--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

[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

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

[Jianqiu Zhang]

Hi, Slackner

Thanks for your so detailed reply and suggestions

I am working on error handling part of the code and I am a bit confused
I looked up RtlAnsiStringToUnicodeString function , and found it would return
3 different ERROR STATUS,  given below: 
--snip--

 * RETURNS
 *  Success: STATUS_SUCCESS. uni contains the converted string
 *  Failure: STATUS_BUFFER_OVERFLOW, if doalloc is FALSE and ansi is too small.
 *           STATUS_NO_MEMORY, if doalloc is TRUE and the allocation fails.
 *           STATUS_INVALID_PARAMETER_2, if the unicode string would be larger than 65535.
 *

--snip--

For what error code will STATUS_INVALID_PARAMETER_2 Give , I can test by manually set the filename to a very long string, but I cannot test What ERROR code will be given
when STATUS_NO_MEMORY returned in Windows, I try to allocate the memory until no more memory can be allocated with while(malloc(1)); but I get the same error code as STATUS_INVALID_PARAMETER_2 give

And what's more, I found something maybe interesting ,or strange

* On wine When the filename length is 65536 , the RtlAnsiStringToUnicodeString will pass successfully
  and it will fail on RtlDosPathNameToNtPathName_U when the filename length is a bit shorter or equal 65535, it will fail on RtlAnsiStringToUnicodeString , why a longer string can pass the RtlAnsiStringToUnicodeString function, What I guess is overflow, but I haven't dig into it yet

* And another strange thing is On windows DumpFile_Test fail when the filename_length is longer than 218 chars(218 works well), On wine no errors happen until the filename length is longer than 255 (255 works well)

If no NTSTATUS ERRORCODE return and RtlDosPahNameToNtPathName_U return TRUE, I cannot set the errorcode , so when the filename length is between 218 to 255 , the pcap_dump_open will behave differently on win and wine, Should I use strlen to compare the string length of filename and set a errorcode when the filename length is longer than 218?



And By the way , I haven't correct my Memory leak problem, I will correct when send my patch :)

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

[Jianqiu Zhang]

I forgot to provide my code in the last post, Here is the code

--wpcap.c--

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;
    NTSTATUS res;
    RtlInitAnsiString(&fname_dos, fname);
    res = RtlAnsiStringToUnicodeString(&dospathW, &fname_dos, TRUE);
    printf("RtlAnsiStringToUnicodeString retval = %X\n", res);
    if(res == STATUS_INVALID_PARAMETER_2)
    {
        SetLastError(ERROR_FILENAME_EXCED_RANGE);
        return NULL;
    }
    if(!RtlDosPathNameToNtPathName_U(dospathW.Buffer, &nt_name, NULL, NULL))
    {
        printf("RtlDosPathNameToNtPathName_U retval = %X\n", res);
        RtlFreeUnicodeString(&dospathW);
        SetLastError(ERROR_FILENAME_EXCED_RANGE);
        return NULL;
    }
    res  = wine_nt_to_unix_file_name(&nt_name, &fname_unix, FILE_OPEN_IF, FALSE);
    printf("VOID_DEBUG: Nt FileName is %s\n", wine_dbgstr_w(nt_name.Buffer));
    printf("wine_nt_to_unix_file_name retval = %X\n", res);
    if(res == STATUS_NO_SUCH_FILE)
    {
        SetLastError(ERROR_SUCCESS);
    }
    else if(res == STATUS_OBJECT_NAME_INVALID)
    {
        RtlFreeUnicodeString(&dospathW);
        SetLastError(ERROR_INVALID_NAME);
        return NULL;
    }
    else if(res == STATUS_OBJECT_NAME_NOT_FOUND || res == STATUS_OBJECT_PATH_NOT_FOUND)
    {
        RtlFreeUnicodeString(&dospathW);
        SetLastError(ERROR_PATH_NOT_FOUND);
        return NULL;
    }
    else
    {
        SetLastError(ERROR_FILE_EXISTS);
    }
    RtlFreeUnicodeString(&nt_name);
    RtlFreeUnicodeString(&dospathW);
    return pcap_dump_open(p, fname_unix.Buffer);
}


--wpcap.c--

--test.c--
//#define HAVE_REMOTE
#include <pcap.h>
#include <string.h>
#define PCAP_SRC_IF_STRING "rpcap://"
#define PCAP_OPENFLAG_PROMISCUOUS 1
/* 回调函数原型 */
void packet_handler(u_char *param, const struct pcap_pkthdr *header, const u_char *pkt_data);
void packet_handler_1(u_char *dumpfile, const struct pcap_pkthdr *header, const u_char *pkt_data);


int main(int argc, char **argv)
{
    pcap_if_t *alldevs;
    pcap_if_t *d;
    int inum;
    int i=0;
    pcap_t *adhandle;
    char errbuf[PCAP_ERRBUF_SIZE];
    pcap_dumper_t *dumpfile;



    /* 检查程序输入参数 */
    if(argc != 2)
    {
        printf("usage: %s filename", argv[0]);
        return -1;
    }

    /* 获取本机设备列表 */
    if (pcap_findalldevs(&alldevs, errbuf) == -1)
    {
        fprintf(stderr,"Error in pcap_findalldevs: %s\n", errbuf);
        exit(1);
    }

    /* 打印列表 */
    for(d=alldevs; d; d=d->next)
    {
        printf("%d. %s", ++i, d->name);
        if (d->description)
            printf(" (%s)\n", d->description);
        else
            printf(" (No description available)\n");
    }

    if(i==0)
    {
        printf("\nNo interfaces found! Make sure WinPcap is installed.\n");
        return -1;
    }

    printf("Enter the interface number (1-%d):",i);
    scanf("%d", &inum);

    if(inum < 1 || inum > i)
    {
        printf("\nInterface number out of range.\n");
        /* 释放列表 */
        pcap_freealldevs(alldevs);
        return -1;
    }

    /* 跳转到选中的适配器 */
    for(d=alldevs, i=0; i< inum-1 ; d=d->next, i++);


    /* 打开适配器 */
    if ( (adhandle= pcap_open_live(d->name,          // 设备名
                              65536,            // 要捕捉的数据包的部分
                              // 65535保证能捕获到不同数据链路层上的每个数据包的全部内容
                              1,    // 混杂模式
                              1000,             // 读取超时时间
                              errbuf            // 错误缓冲池
                             ) ) == NULL)
    {
        fprintf(stderr,"\nUnable to open the adapter. %s is not supported by WinPcap\n", d->name);
        /* 释放设备列表 */
        pcap_freealldevs(alldevs);
        return -1;
    }

    /* 打开堆文件 */
    //Change the last byte to a not NULL character

    char* str = argv[1];
    char *p = str;
   // while(*(str++) != '\0');
     //   str--;
       // *str = 'H';
    printf("Path is now become %s\n", p);
        //printf("ERRCODE %X\n", GetLastError());
    //while(malloc(10));
    dumpfile = pcap_dump_open(adhandle, str);
    printf("ERRCODE %X\n", GetLastError());

    if(dumpfile==NULL)
    {
        fprintf(stderr,"\nError opening output file\n");
        return -1;
    }

    printf("\nlistening on %s... Press Ctrl+C to stop...\n", d->description);

    /* 释放设备列表 */
    pcap_freealldevs(alldevs);

    /* 开始捕获 */
    printf("Catch Loop\n");
    pcap_loop(adhandle, 0, packet_handler, (unsigned char *)dumpfile);

    return 0;
}

/* 回调函数，用来处理数据包 */
void packet_handler(u_char *dumpfile, const struct pcap_pkthdr *header, const u_char *pkt_data)
{
    /* 保存数据包到堆文件 */
    pcap_dump(dumpfile, header, pkt_data);
}


--test.c--

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

[Sebastian Lackner]

On 11.02.2016 05:44, Jianqiu Zhang wrote:
> Hi, Slackner
>
> Thanks for your so detailed reply and suggestions
>
> I am working on error handling part of the code and I am a bit confused
> I looked up RtlAnsiStringToUnicodeString function , and found it would return
> 3 different ERROR STATUS,  given below: 
> --snip--
>
> * RETURNS
>  *  Success: STATUS_SUCCESS. uni contains the converted string
>  *  Failure: STATUS_BUFFER_OVERFLOW, if doalloc is FALSE and ansi is too small.
>  *           STATUS_NO_MEMORY, if doalloc is TRUE and the allocation fails.
>  *           STATUS_INVALID_PARAMETER_2, if the unicode string would be larger than 65535.
>  *
>
> --snip--
>
> For what error code will STATUS_INVALID_PARAMETER_2 Give , I can test by manually set the filename to a very long string, but I cannot test What ERROR code will be given
> when STATUS_NO_MEMORY returned in Windows, I try to allocate the memory until no more memory can be allocated with while(malloc(1)); but I get the same error code as STATUS_INVALID_PARAMETER_2 give

You do not have to create your own translation tables. Just translate the NTSTATUS into
an error code, similar to how its done in kernel32:

SetLastError(RtlNtStatusToDosError(status));
> And what's more, I found something maybe interesting ,or strange
>
> * On wine When the filename length is 65536 , the RtlAnsiStringToUnicodeString will pass successfully
>   and it will fail on RtlDosPathNameToNtPathName_U when the filename length is a bit shorter or equal 65535, it will fail on RtlAnsiStringToUnicodeString , why a longer string can pass the RtlAnsiStringToUnicodeString function, What I guess is overflow, but I haven't dig into it yet

That sounds like a separate issue. You can ignore such corner cases because the fix would
be unrelated to your implementation in wpcap.
> * And another strange thing is On windows DumpFile_Test fail when the filename_length is longer than 218 chars(218 works well), On wine no errors happen until the filename length is longer than 255 (255 works well)

Such minor differences are known, but in this case there are no plans to be 100% "bug compatible" with Windows.
> If no NTSTATUS ERRORCODE return and RtlDosPahNameToNtPathName_U return TRUE, I cannot set the errorcode , so when the filename length is between 218 to 255 , the pcap_dump_open will behave differently on win and wine, Should I use strlen to compare the string length of filename and set a errorcode when the filename length is longer than 218?

I don't think we really care about such minor differences, unless applications depend on it.
You can ignore them for now.
> And By the way , I haven't correct my Memory leak problem, I will correct when send my patch :)

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

[Jianqiu Zhang]

> You do not have to create your own translation tables. Just translate the NTSTATUS into
> an error code, similar to how its done in kernel32:
>
> SetLastError(RtlNtStatusToDosError(status));

I found SetLastError(RltNtStatusToDosError(status)) behaved not correct , when the file length is very long , It give the error code ERROR_INVALID_PARAMETER

but Windows give the ERROR_FILENAME_EXCED_RANGE

should I omit this and just use SetLastError(RtlNtStatusToDosError(status)) or maullay set the error code to ERROR_FILENAME_EXCED_RANGE?

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

[Sebastian Lackner]

On 11.02.2016 06:14, Jianqiu Zhang wrote:
> > You do not have to create your own translation tables. Just translate the NTSTATUS into
> > an error code, similar to how its done in kernel32:
> >
> > SetLastError(RtlNtStatusToDosError(status));
>
> I found SetLastError(RltNtStatusToDosError(status)) behaved not correct , when the file length is very long , It give the error code ERROR_INVALID_PARAMETER
>
> but Windows give the ERROR_FILENAME_EXCED_RANGE
>
> should I omit this and just use SetLastError(RtlNtStatusToDosError(status)) or maullay set the error code to ERROR_FILENAME_EXCED_RANGE?

I would suggest to omit it. The correct place to fix that is most likely
somewhere else, and not in the part you are currently working on.