Le 03/08/2011 18:07, Francois Gouget a écrit :
WriteFile() checks if the handle corresponds to the console and if it does invokes WriteConsoleA(). This barely escaped an infinite loop but messed up the string encoding as CP_UNIXCP is usually different from CP_ACP.
I noticed this issue while testing the net.exe tool. After making it use WineConsole() I was getting the following outputs:
$ wine net stop Spécifie le service à arrêter. $ ./wine net stop | cat Sp├®cifie le service ├á arr├¬ter.
It turns out that it's because we only get through the WineConsoleW() -> WriteFile() -> WriteConsoleA() loop in the second case.
could you elaborate a bit more. in case of wine ... | cat, the standard output should be a pipe not a console so writeconsoleW is supposed to fail so it would make more sense to simply let WriteConsoleW fail when is_console_handle(hConsoleOutput) is FALSE ? (similar functions as WriteConsoleA, ReadConsoleA/W should be protected the same way)
A+
On Tue, 23 Aug 2011, Eric Pouech wrote:
Le 03/08/2011 18:07, Francois Gouget a écrit :
WriteFile() checks if the handle corresponds to the console and if it does invokes WriteConsoleA(). This barely escaped an infinite loop but messed up the string encoding as CP_UNIXCP is usually different from CP_ACP.
I noticed this issue while testing the net.exe tool. After making it use WineConsole() I was getting the following outputs:
$ wine net stop Spécifie le service à arrêter. $ ./wine net stop | cat Sp├®cifie le service ├á arr├¬ter.
It turns out that it's because we only get through the WineConsoleW() -> WriteFile() -> WriteConsoleA() loop in the second case.
could you elaborate a bit more. in case of wine ... | cat, the standard output should be a pipe not a console
* net.c is calling GetStdHandle(STD_OUTPUT_HANDLE). * It then passes that to WriteConsoleW() and get_console_bare_fd() does return an fd for it (no idea if that's right or wrong). * So we end up in the WriteFile case and give it wine_server_ptr_handle(console_handle_unmap(hConsoleOutput)). Frankly I don't know what that's supposed to do but when WriteFile() calls is_console_handle() on it it gets TRUE.
so writeconsoleW is supposed to fail
Yes. It does fail when output is a pipe on Windows.
so it would make more sense to simply let WriteConsoleW fail when is_console_handle(hConsoleOutput) is FALSE ? (similar functions as WriteConsoleA, ReadConsoleA/W should be protected the same way)
Currently only three APIs in console.c check is_console_handle(): VerifyConsoleIoHandle(), DuplicateConsoleHandle() and CloseConsoleHandle(). That could certainly be extended to others and things even seems to work if I add a simple 'if (!is_console_handle(hConsoleOutput)) return FALSE;' at the start of WriteConsoleW().
One could then rip out the code in the WriteFile() branch but I'm not sure what the consequences of all this would be or why the WriteFile() case was added in the first place. The author of that code would probably know better than me<g>.
so it would make more sense to simply let WriteConsoleW fail when is_console_handle(hConsoleOutput) is FALSE ? (similar functions as WriteConsoleA, ReadConsoleA/W should be protected the same way)
Currently only three APIs in console.c check is_console_handle(): VerifyConsoleIoHandle(), DuplicateConsoleHandle() and CloseConsoleHandle(). That could certainly be extended to others and things even seems to work if I add a simple 'if (!is_console_handle(hConsoleOutput)) return FALSE;' at the start of WriteConsoleW().
then that's the correct way to fix it
One could then rip out the code in the WriteFile() branch but I'm not sure what the consequences of all this would be or why the WriteFile() case was added in the first place. The author of that code would probably know better than me<g>.
no, the WriteFile stuff must be kept
basically, we need to handle three cases (h being the handle passed to WriteConsole) C1/ h isn't a console handle (ie lower bits not set) => fail (return FALSE) C2/ h is a console handle, and removing the lower bits we have a handle to a console object in wine server => call wineserver C3/ h is a console handle, and removing the lower bits we have a handle to a file object in wine server => call WriteFile => ...
you get : - C1 when output is redirected to a file/pipe... - C2 when running wineconsole net.exe - C3 when running wine net.exe
note that we're playing with handles, and hidding behind console handles (with lower bit sets) handles to other objects... (in fact, from your first post, there is no infinite loop as the calls are made on different objects)
you can safely provide the patches with protecting WriteConsole and friends with a if (!is_console_handle(???)) return FALSE; line
A+
On Wed, 24 Aug 2011, Eric Pouech wrote: [...]
you can safely provide the patches with protecting WriteConsole and friends with a if (!is_console_handle(???)) return FALSE; line
Would something like the attached patch be ok? As far as I can see all the other APIs pretty much immediately pass the handle to wineserver which I expect will complain if it's not a console handle.
Further notes: * Performing the 'console handle check' locally would likely be faster but, except for WriteConsole maybe, this should usually be the case and is probably not performance critical anyway. * There are a few cases where we check for invalid parameters and return an error before the handle is checked by wineserver. It's possible that in such cases Windows checks the handle first. But unless we know of applications that need the right error code in these corner cases it does not justify adding an explicit check.
Le 01/09/2011 16:22, Francois Gouget a écrit :
On Wed, 24 Aug 2011, Eric Pouech wrote: [...]
you can safely provide the patches with protecting WriteConsole and friends with a if (!is_console_handle(???)) return FALSE; line
Would something like the attached patch be ok? As far as I can see all the other APIs pretty much immediately pass the handle to wineserver which I expect will complain if it's not a console handle.
Further notes:
- Performing the 'console handle check' locally would likely be faster but, except for WriteConsole maybe, this should usually be the case and is probably not performance critical anyway.
- There are a few cases where we check for invalid parameters and return an error before the handle is checked by wineserver. It's possible that in such cases Windows checks the handle first. But unless we know of applications that need the right error code in these corner cases it does not justify adding an explicit check.
actually, a couple of API support the "bare" handles and those should be protected (what your patch does) otherwise, wineserver iwill fail on them this patch looks ok, but you have to reverse the previous one as well A+
-
Eric Pouech -
Francois Gouget