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
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
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
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
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:
- test.exe "AFileNameDoesNotExist" on wine it gets errorcode 0x57 on Windows it gets errorcode 0x00, Both get the correct dumpfile
- 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
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 :)
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--
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 :)
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?
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.
-
André Hentschel -
Jianqiu Zhang -
Sebastian Lackner