Re: Reject wildcards in directory names
Alexandre Julliard <julliard(a)winehq.org> wrote:
As for the invalid characters, [Create|Remove]File[A|W] only check for '?', '*', which I just copied for [Create|Remove]Directory[A|W]. I take it you will accept a patch that also extends the checks in [Create|Remove]File*?
Yes, and of course it should use a common routine to do the checks, so that it's consistent everywhere. Also the Create and Remove cases should be handled differently IMO, a Remove should succeed if the specified file exists, even if it contains illegal characters.
I would actually agree with Alexandre here, but W2K at least behaves differently. My tests show that RemoveDirectory just as CreateDirectory rejects all paths which contain one of the wildcards ": * ? \" < > |" with error 123 completely independent where that character occurs (not just in the path element which is created). And as far as I can say DeleteFile just as much behaves also this way. So should we attempt to be smarter than Windows in this case? Rolf Kalbermatter
On Friday 21 November 2003 09:43, Rolf Kalbermatter wrote:
I would actually agree with Alexandre here, but W2K at least behaves differently. My tests show that RemoveDirectory just as CreateDirectory rejects all paths which contain one of the wildcards ": * ? \" < > |" with error 123 completely independent where that character occurs (not just in the path element which is created).
I extended the conformance test to check that and yes, RemoveDirectory will fail on W2K (ntfs). To add to your observation, there is also a number of invalid non-readable characters, and I read somewhere that FAT also rejects '^'.
And as far as I can say DeleteFile just as much behaves also this way. So should we attempt to be smarter than Windows in this case?
No, I don't think we should. Some (buggy) application might depend on it. But I do think that RemoveDirectory always succeeding would have been a better implementation. By the way, this points in the direction of a single parameter checking routine that get's called in all API's that take a path or filename. -Hans
"Rolf Kalbermatter" <rolf.kalbermatter(a)citeng.com> writes:
My tests show that RemoveDirectory just as CreateDirectory rejects all paths which contain one of the wildcards ": * ? \" < > |" with error 123 completely independent where that character occurs (not just in the path element which is created). And as far as I can say DeleteFile just as much behaves also this way. So should we attempt to be smarter than Windows in this case?
Yes, because under Unix you can actually create files that contain wildcards, so it should be possible to access them. We just need to prevent the Windows app itself from creating them. -- Alexandre Julliard julliard(a)winehq.com
Yes, because under Unix you can actually create files that contain wildcards, so it should be possible to access them. We just need to prevent the Windows app itself from creating them.
Interesting. What about CopyFile and MoveFile then? CopyFile creates a new file, so following your logic it shouldn't be possible to copy *to* a file with a wildcard name, but it should be be possible to copy *from* a file with wildcards. What if I want to copy over an *existing* file with wildcards, created under Unix? Isn't that a way of "accessing" the existing file? That's not possible in this scenario since CopyFile is implemented using CreateFile. And then there's MoveFile, which is implemented using CopyFile and DeleteFile so it's semantics follows from those. So now it's not possible to, say, move existing wildcard files from one directory to another, which is something you want to be able to do isn't it? Schematically I get this picture: CreateFile file1 OK CreateFile file2* NOT OK DeleteFile file1 OK DeleteFile file2* OK Copyfile file1 file2 OK Copyfile file1* file2 OK Copyfile file1 file2* NOT OK Copyfile file1* file2* NOT OK Movefile file1 file2 OK Movefile file1* file2 OK Movefile file1 file2* NOT OK Movefile file1* file2* NOT OK Going about it this way doesn't seem right to me. Shouldn't we simply allow anything the Unix filesystem allows? And thus strip the existing checks for wildcards from CreateFile* and friends? And remove the corresponding test for [Create|Remove]Directory? Would that break any application? Or simply allow anything Windows allows? Or should CopyFile and MoveFile perhaps become primitives, so we can have other combinations? -Hans
Alexandre Julliard wrote:
"Rolf Kalbermatter" <rolf.kalbermatter(a)citeng.com> writes:
My tests show that RemoveDirectory just as CreateDirectory rejects all paths which contain one of the wildcards ": * ? \" < > |" with error 123 completely independent where that character occurs (not just in the path element which is created). And as far as I can say DeleteFile just as much behaves also this way. So should we attempt to be smarter than Windows in this case?
Yes, because under Unix you can actually create files that contain wildcards, so it should be possible to access them. We just need to prevent the Windows app itself from creating them.
How on earth can a discussion about wildcards be related to unicows??? It appears that applications using unicows will not work in NT mode on wine unless the attached patch is applied. Unicows' init calls "GetFileAttributesW" with "???.???". It expects, if it fails, to get error code 123 (ERROR_INVALID_NAME), otherwise it simply doesn't work. Wine returns 2 (ERROR_FILE_NOT_FOUND). For that horrible sin, the entire application misbehaves. It also misbehaves, in the same way, if GetFileAttributes succeeds for this file. I can't escape the feeling this was meant specifically as a trap for Wine. I am yet to find any other use for this test. Any other explanation will be greatly appretiated. One possible workaround that may satisfy both Alexandre and unicows may be to put the wildcards checking code into the error part of the function. In other words, if we have not found such a file, return ERROR_INVALID_NAME if the name contains wildcards, and ERROR_FILE_NOT_FOUND if not. This way, if the file IS found, it will be handled correctly. Personally, I don't like this workaround. It means that a perfectly legal file name is reported as illegal. I am, however, open to suggestions. Shachar -- Shachar Shemesh Open Source integration & consulting Home page & resume - http://www.shemesh.biz/ Index: files/dos_fs.c =================================================================== RCS file: /home/sun/sources/cvs/wine/files/dos_fs.c,v retrieving revision 1.142 diff -u -r1.142 dos_fs.c --- files/dos_fs.c 15 Nov 2003 00:13:21 -0000 1.142 +++ files/dos_fs.c 25 Nov 2003 13:22:23 -0000 @@ -1047,6 +1047,12 @@ return FALSE; } + if (strchrW(name, '?') || strchrW(name, '*')) + { + SetLastError(ERROR_INVALID_NAME); + return FALSE; + } + if ((full->drive = DOSFS_GetPathDrive( &name )) == -1) return FALSE; flags = DRIVE_GetFlags( full->drive );
Shachar Shemesh <wine-devel(a)shemesh.biz> writes:
It appears that applications using unicows will not work in NT mode on wine unless the attached patch is applied. Unicows' init calls "GetFileAttributesW" with "???.???". It expects, if it fails, to get error code 123 (ERROR_INVALID_NAME), otherwise it simply doesn't work. Wine returns 2 (ERROR_FILE_NOT_FOUND). For that horrible sin, the entire application misbehaves. It also misbehaves, in the same way, if GetFileAttributes succeeds for this file. I can't escape the feeling this was meant specifically as a trap for Wine. I am yet to find any other use for this test. Any other explanation will be greatly appretiated.
It seems to be a way to detect NT/Win95, I suspect Win95 returns a different error code in that case. Using GetVersion() would of course have been too easy...
One possible workaround that may satisfy both Alexandre and unicows may be to put the wildcards checking code into the error part of the function. In other words, if we have not found such a file, return ERROR_INVALID_NAME if the name contains wildcards, and ERROR_FILE_NOT_FOUND if not. This way, if the file IS found, it will be handled correctly.
Well, yes, that was the idea. The normal case is that the file doesn't exist, and in that case we of course need to return the proper error (also taking Windows version into account apparently...) Anyway, I looked into your unicows exe, and I believe the problem is that the home-made GetProcAddress that unicows.lib is using doesn't support forwarded ordinals, so I'm afraid we'll need to export real functions from unicows.dll. -- Alexandre Julliard julliard(a)winehq.com
Alexandre Julliard wrote:
It seems to be a way to detect NT/Win95, I suspect Win95 returns a different error code in that case. Using GetVersion() would of course have been too easy...
And that, after they DO call "GetVersion", and do keep a variable telling whether it's NT or not. Oh well. Would you say that returning the error in the patch only if the platform is NT type will be a correct thing to do? If so, I'll send a patch.
One possible workaround that may satisfy both Alexandre and unicows may be to put the wildcards checking code into the error part of the function. In other words, if we have not found such a file, return ERROR_INVALID_NAME if the name contains wildcards, and ERROR_FILE_NOT_FOUND if not. This way, if the file IS found, it will be handled correctly.
Well, yes, that was the idea. The normal case is that the file doesn't exist, and in that case we of course need to return the proper error (also taking Windows version into account apparently...)
I'll send a patch, then.
Anyway, I looked into your unicows exe, and I believe the problem is that the home-made GetProcAddress that unicows.lib is using doesn't support forwarded ordinals, so I'm afraid we'll need to export real functions from unicows.dll.
I already have a plan for doing that. Unfortunetly, it's not a nice one. Basically, it means changing unicows.spec.c after it's generated. I'll try to create the makefile so that this will still support dynamic changes. -- Shachar Shemesh Open Source integration & consulting Home page & resume - http://www.shemesh.biz/
Shachar Shemesh <wine-devel(a)shemesh.biz> writes:
I'll send a patch, then.
Note that this needs a general solution. Please don't just add a special case in GetFileAttributes.
I already have a plan for doing that. Unfortunetly, it's not a nice one. Basically, it means changing unicows.spec.c after it's generated. I'll try to create the makefile so that this will still support dynamic changes.
Why would you need to do that? -- Alexandre Julliard julliard(a)winehq.com
Alexandre Julliard wrote:
Shachar Shemesh <wine-devel(a)shemesh.biz> writes:
I'll send a patch, then.
Note that this needs a general solution. Please don't just add a special case in GetFileAttributes.
My existing patch is in DOSFS_GetFullName, which is called by GetFileAttributes. Another thing, however, is that I'm begining to doubt whether it is indeed used for what you said it is. It seems that calling "GetFileAttributesW" on Windows 98 returns 120 (function not implemented). This casts a great doubt on whether that is indeed the purpose of calling "GetFileAttributesW". In any case, calling "GetFileAttributesA" on Windows 98 indeed yields "2" (file not found).
I already have a plan for doing that. Unfortunetly, it's not a nice one. Basically, it means changing unicows.spec.c after it's generated. I'll try to create the makefile so that this will still support dynamic changes.
Why would you need to do that?
Because I don't seem to be able to define a function called "GetProcAddress" in a winelib dll. -- Shachar Shemesh Open Source integration & consulting Home page & resume - http://www.shemesh.biz/
Shachar Shemesh <wine-devel(a)shemesh.biz> writes:
My existing patch is in DOSFS_GetFullName, which is called by GetFileAttributes. Another thing, however, is that I'm begining to doubt whether it is indeed used for what you said it is. It seems that calling "GetFileAttributesW" on Windows 98 returns 120 (function not implemented). This casts a great doubt on whether that is indeed the purpose of calling "GetFileAttributesW".
Why? I think it makes even more sense, obviously if the function is not implemented in Win98 then it will never return the proper error code, so it's a good detection mechanism. What other purpose do you think this would serve?
Because I don't seem to be able to define a function called "GetProcAddress" in a winelib dll.
I don't see why not, unless maybe if you are using delayed imports, but you shouldn't need that. How exactly are you defining it? -- Alexandre Julliard julliard(a)winehq.com
Alexandre Julliard wrote:
Shachar Shemesh <wine-devel(a)shemesh.biz> writes:
My existing patch is in DOSFS_GetFullName, which is called by GetFileAttributes. Another thing, however, is that I'm begining to doubt whether it is indeed used for what you said it is. It seems that calling "GetFileAttributesW" on Windows 98 returns 120 (function not implemented). This casts a great doubt on whether that is indeed the purpose of calling "GetFileAttributesW".
Why? I think it makes even more sense, obviously if the function is not implemented in Win98 then it will never return the proper error code, so it's a good detection mechanism. What other purpose do you think this would serve?
I THINK, though I need to test that, that this code is only reached if it is already known that this is not Windows 98. I can test that. In any case, this doesn't really matter.
Because I don't seem to be able to define a function called "GetProcAddress" in a winelib dll.
I don't see why not, unless maybe if you are using delayed imports, but you shouldn't need that. How exactly are you defining it?
What do you mean by "delayed imports". When I defined, proper, the entire spec as actual functions, I got conflicts when I tried to define GetProcAddress. In any case, even then I'm going to need to call GetProcAddress (yes, I guess I can use the same strange macros to do that, or call the native NT function. I'd rather not do that, however). These problems are apparent in the experimental patch I sent (http://www.winehq.org/hypermail/wine-devel/2003/11/0098.html). Shachar -- Shachar Shemesh Open Source integration & consulting Home page & resume - http://www.shemesh.biz/
Shachar Shemesh <wine-devel(a)shemesh.biz> writes:
What do you mean by "delayed imports". When I defined, proper, the entire spec as actual functions, I got conflicts when I tried to define GetProcAddress. In any case, even then I'm going to need to call GetProcAddress (yes, I guess I can use the same strange macros to do that, or call the native NT function. I'd rather not do that, however).
These problems are apparent in the experimental patch I sent (http://www.winehq.org/hypermail/wine-devel/2003/11/0098.html).
That patch builds just fine for me. You know, I'd really like to help you, but this is very frustrating: I'm sure I could give you a solution in 2 minutes if I actually saw the problem; instead of that you waste your time trying to modify the generated .spec.c file, and I waste my time trying to guess what error you are seeing. How about sending me the exact code that fails, with the exact error messages? OK, in the meantime I'll try to continue guessing. I suppose you called your implementation GetProcAddress, which of course makes it impossible to call the kernel32 one. If that's the problem, you should simply use a different name for your function, and do the mapping in the spec file. So you do something like: @ stdcall GetProcAddress(long ptr) UNICOWS_GetProcAddress and implement UNICOWS_GetProcAddress by calling the real GetProcAddress. Next, note that calling GetProcAddress at all is unnecessary; you can let the linker do the work, by implementing the other functions the same way (with a UNICOWS_xxx function that calls the real xxx). -- Alexandre Julliard julliard(a)winehq.com
Hi, On Tue, Nov 25, 2003 at 03:24:02PM -0800, Alexandre Julliard wrote:
Shachar Shemesh <wine-devel(a)shemesh.biz> writes:
What do you mean by "delayed imports". When I defined, proper, the entire spec as actual functions, I got conflicts when I tried to define GetProcAddress. In any case, even then I'm going to need to call GetProcAddress (yes, I guess I can use the same strange macros to do that, or call the native NT function. I'd rather not do that, however).
These problems are apparent in the experimental patch I sent (http://www.winehq.org/hypermail/wine-devel/2003/11/0098.html).
That patch builds just fine for me. You know, I'd really like to help you, but this is very frustrating: I'm sure I could give you a solution in 2 minutes if I actually saw the problem; instead of that you waste your time trying to modify the generated .spec.c file, and I waste my time trying to guess what error you are seeing. How about sending me the exact code that fails, with the exact error messages?
OK, in the meantime I'll try to continue guessing. I suppose you called your implementation GetProcAddress, which of course makes it impossible to call the kernel32 one. If that's the problem, you should simply use a different name for your function, and do the mapping in the spec file. So you do something like: Well, if he has such trouble getting the GetProcAddress call resolved properly, then why not instead not use the normal import mechanism, but getting the pointer to that problematic function manually by doing a pFunc = GetProcAddress(hModKernel32, "GetProcAddress"); ?
Awww, wait... something's borken here ;) Score: -255, insanely funny Andreas Mohr P.S.: you may say what you want, but that WAS my first idea, until I realized certain issues with that... ;) P.P.S.: who the ยง$&/! reenabled mail delivery for my wine-devel account? Oh well... it's about time to join the discussion again anyway :)
participants (5)
-
Alexandre Julliard -
Andreas Mohr -
Hans Leidekker -
Rolf Kalbermatter -
Shachar Shemesh