[Bug 38558] New: cmd.exe bundled with Windows XP messed up when using FOR /F
https://bugs.winehq.org/show_bug.cgi?id=38558 Bug ID: 38558 Summary: cmd.exe bundled with Windows XP messed up when using FOR /F Product: Wine Version: 1.7.42 Hardware: x86-64 OS: Linux Status: UNCONFIRMED Severity: normal Priority: P2 Component: -unknown Assignee: wine-bugs(a)winehq.org Reporter: katsunori.kumatani(a)gmail.com Distribution: Mint I found some pretty old batch scripts I had laying around, so I decided to test if wine could run them fine. I'm using latest version (1.7.42) as of this time, on Linux Mint 17.1 Rebecca, and using the cmd.exe from a Windows XP SP2 old installation media. cmd has the ability to scan through files (and separating each line into tokens) using the "FOR /F" method. For example, to reproduce this, create a simple text file (test.txt) and put ascending numbers on each line (up to 16 for now): 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 Then use this command in the cmd.exe: FOR /F "tokens=*" %A IN (test.txt) DO @echo %A It SHOULD display all 16 lines in turn, but it's only displaying until 10. If you take out the last two lines (15 and 16), then it will display ONLY until 5, i.e: 1 2 3 4 5 This is a serious bug IF it is within wine because it obviously is something that affects data integrity and not just a glitch. I am positive that it replaces the last bytes of a file with some junk/overflows or something like that (making it a security issue), which is why it stops "early" since it finds a NULL character, and windows processor stops at a null. I'm sure of that because AkelPad 4.1.2 has a similar bug: there's non-sensical binary data on every file you open with it! (i.e you open a file and re-save it, and it's messed up at the end in the output...). But the latest version of it, AkelPad 4.2.3, seems to not have this issue, which makes me think if it's really a wine bug or not. I would still appreciate if you had a small look at the file functions (and eof) though. Note, I couldn't find out how to use wineconsole or wine's built-in cmd at all, it doesn't seem to work at all with FOR /F or at least not in any way properly? Unfortunately I can't attach a log or something since using the cmd.exe puts it in the shell from where I invoke it... so wine's messages are not visible, unless I am missing something? -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=38558 Austin English <austinenglish(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |download Component|-unknown |cmd -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=38558 winetest(a)luukku.com changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |winetest(a)luukku.com --- Comment #1 from winetest(a)luukku.com --- Unless I did something wrong the command doesnt list any of the numbers... wine 1.9.24 and staging 1.9.23. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=38558 --- Comment #2 from winetest(a)luukku.com --- (In reply to winetest from comment #1)
Unless I did something wrong the command doesnt list any of the numbers...
wine 1.9.24 and staging 1.9.23.
To be more presice I didnt fetch any external cmd.exe. Tried the wine's inbuild. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=38558 katsunori.kumatani(a)gmail.com changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katsunori.kumatani(a)gmail.co | |m --- Comment #3 from katsunori.kumatani(a)gmail.com --- Hi, sorry for the very late reply, for some reason I received no email and just manually looked through my open bugs today... Yes, it's probably the same bug, not showing anything because it "truncates" all of it early. Sometimes you get no output because it truncates all of it. Adding a lot of "empty lines" at the end of the input will make it work even with XP's cmd.exe -- I suspect because of the buffer being large enough in that case. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=38558 Jason Edmeades <us(a)edmeades.me.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |us(a)edmeades.me.uk --- Comment #4 from Jason Edmeades <us(a)edmeades.me.uk> --- Looking at this bug, I've found a few related issues in the inbuild wine cmd processing for 'for' loops (It doesnt handle tokens=* syntax at all, using variable 'a' or 'A' with it was dodgy, and it didnt handle * matching the end of line). Whilst it wouldnt help particularly with the reported bug (which is that the microsoft xp cmd.exe has problems under wine), it may help avoid the need to use it. I've got a patch coded, will put a link to the patch in here when done - dont know if you are still able to test it using wine's command shell. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=38558 --- Comment #5 from katsunori.kumatani(a)gmail.com --- It would certainly be a great thing if the builtin cmd would work as well as the one supplied by Microsoft! At this point, another possibility for this bug is more like some buffer overflow in cmd.exe (i.e. bug in Microsoft's cmd) that Windows just silently ignores by coincidence or something... Btw, maybe a bit esoteric, but don't forget that in batch for loops, ? is a valid name for a variable too (i.e. %%? instead of %%a or %%b and so on). :) -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=38558 --- Comment #6 from Jason Edmeades <us(a)edmeades.me.uk> --- Created attachment 61794 --> https://bugs.winehq.org/attachment.cgi?id=61794 Simple patch to do byte by byte data conversion -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=38558 --- Comment #7 from Jason Edmeades <us(a)edmeades.me.uk> --- A short investigation shows the problem here is simply the way wine does data conversion from single to doublebyte - it does 16 bytes at a time, but this ends up corrupting the buffer if the src overlaps the dest. A simple patch is applied, which gets things going, but I have no ideas if it would be acceptable to wine as potentially there's a performance hit here. Re the %? - Given its such an edge case I'll wait until someone reports a need for it before trying to add it to wine, as the code is specific for a-z, A-Z at the moment. No idea if there's any others either! Either way my only priority at the moment is to get wine's cmd running for these kind of test cases. I've found a problem as a result of this bug report for which I'll submit a patch, but the issue of getting windows XP's cmd.exe running is outside my scope. Either way, its definitely not a 'cmd' component problem, assigning to kernel32 which is where I think the converions would come under -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=38558 Jason Edmeades <us(a)edmeades.me.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Component|cmd |kernel32 -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=38558 --- Comment #8 from katsunori.kumatani(a)gmail.com --- Thanks, I'll test the patch tomorrow. About the performance degradation: is there a reason you can't just do an initial check to see if the buffers overlap (distance between them < 16 bytes) and in that case use this slow but correct method? A single branch shouldn't really hurt performance and will yield correct results in all cases (even if it's not this bug itself it's still a bug). -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=38558 --- Comment #9 from katsunori.kumatani(a)gmail.com --- I can confirm now: this patch fixes the bug. Good job locating it! If I'm not wrong, a "fast" check that picks the "slow" case (1 byte at a time) would be something like this: if((UINT_PTR)((unsigned char*)(dst+dstlen) - src) < srclen) { /* slow path */ } It looks ugly but it has only one branch (relies on two's complement, which Windows runs of anyway), I tried to keep the overhead minimal for most cases before. But this is probably too conservative? A more normal / simpler check can obviously be done, if it's better. Just offering some ideas. Anyway, the bug gets fixed so definitely something has to be done about it. I think the above won't impact performance in most cases, except those which were wrong before (but correct results are more important than performance IMO). -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=38558 --- Comment #10 from katsunori.kumatani(a)gmail.com --- Nevermind the check above: it doesn't handle some corner cases I messed around with. A proper one would be something like: if((UINT_PTR)(src - (unsigned char*)dst) < dstlen * sizeof(*dst)) This handles all cases where dst is "behind" src and touches src at a point (within dstlen), again relying on 2's complement to have efficient check (only one branch) in case performance is a worry for small strings. It doesn't handle cases where they still overlap but with dst "in front" of src, but in those cases, I don't think the fast path corrupts anything more than doing it 1 byte at a time... so we can omit that case, but maybe I'm wrong? -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=38558 --- Comment #11 from katsunori.kumatani(a)gmail.com --- Created attachment 61807 --> https://bugs.winehq.org/attachment.cgi?id=61807 Be conservative when dst overlaps into src This solves the bug when dst overlaps into src from a lower address. The opposite case, when src overlaps into dst from a lower address, is not an issue with the former code path (16 bytes at a time). Minimal performance impact of only one branch before the loop, should not affect small strings much. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=38558 --- Comment #12 from katsunori.kumatani(a)gmail.com --- I've tested the new patch, and it works fine without the performance degradation of the former (I also added comments to explain why). Feel free to submit the patch for me or even under your name, if doing otherwise requires some hurdles I have to go through (I've never submitted a patch before FWIW). I don't mind and it's pretty trivial / you found it anyway, would be good to have the bug fixed. Thanks again Jason. :) -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=38558 Gabriel Ivăncescu <gabrielopcode(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |gabrielopcode(a)gmail.com --- Comment #13 from Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- Just stumbled upon this bug in practice, and I see it hasn't gone anywhere. Do you mind if I take over this bug and submit a revised patch for it? -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=38558 --- Comment #14 from Jason Edmeades <us(a)edmeades.me.uk> --- Fine by me -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=38558 Aaron Simmons <paleozogt(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |paleozogt(a)gmail.com -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=38558 helloworld <echekanskiy(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |echekanskiy(a)gmail.com --- Comment #15 from helloworld <echekanskiy(a)gmail.com> --- Can this bug affect code like this? "FOR /F "tokens=2* delims= " %%A IN ('REG QUERY "%RegKeyPath%" /v FrameworkDir32') DO SET FrameworkDir32=%%B" This is part of windows sdk 7.1, and this bug makes sdk unusable. I will try patch later and see if it will work. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=38558 --- Comment #16 from Jason Edmeades <us(a)edmeades.me.uk> --- @helloworld - This bug is for the windows version of cmd.exe, not the wine version. If you are seeing problems running that script on up to date wine using builtin cmd.exe, please raise a separate bug (your description sounds similar to something fixed recently so please make sure you try a new version). If you are using windows cmd.exe, then this could be your issue, but also please consider why you would be using windows cmd.exe and ensure there are bugs for any issues handling batch programs that necessity can be removed. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=38558 Gabriel Ivăncescu <gabrielopcode(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED Fixed by SHA1| |e84c26cec6d4a3b5009eb6cc895 | |031c6673a67dc Status|UNCONFIRMED |RESOLVED --- Comment #17 from Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- Fixed by e84c26cec6d4a3b5009eb6cc895031c6673a67dc -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=38558 Alexandre Julliard <julliard(a)winehq.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED --- Comment #18 from Alexandre Julliard <julliard(a)winehq.org> --- Closing bugs fixed in 4.1. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=38558 Michael Stefaniuc <mstefani(a)winehq.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Target Milestone|--- |4.0.x -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=38558 Michael Stefaniuc <mstefani(a)winehq.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Target Milestone|4.0.x |--- --- Comment #19 from Michael Stefaniuc <mstefani(a)winehq.org> --- Removing the 4.0.x milestone from bug fixes included in 4.0.1. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
participants (1)
-
wine-bugs＠winehq.org