cmd: Don't loop infinitely on NUL.
Merge request reports
Activity
assigned to @ivyl
unassigned @ivyl
Hi. Thanks for the review.
Either I don't fully understand your comment or it doesn't work the way you describe. According to my testing the line containing NUL and every line after it seems to be fully ignored by
FOR /F
. If we would return characters up to NUL that would not be the case.Can you elaborate a bit more?
Hi Arek
I'm not concerned about droping all characters after the NULL (1)
I'm concerned by what's before the NUL character! and basically what should be an marker for end of file
if instead of writing "a b c\n\0f .." into your test file, you just write "a b c\0" (removing the CR at the end of "a b c", I believe that "a b c" should be returned by WCMD_fgets and subsequent WCMD_fgets should return NULL to mark EOF.
(so from a cursory look in pseudo code
if (*p == '\0' && p > bufA) break; /* reposition at '\0' to reread the NULL in next call) */ if (*p == '\0' && p == bufA) return NULL;
)
(1): if you have time, it would be interesting to see what happens if instead of \0, ^Z (0x1A) is used (it's the end of text file marker in the old DOS days). I wouldn't be surprised if it stops also reading the file. (and maybe at some point we'd better use read/write from msvcrt which should handle all this DOS stuff directly, but that's likely a too large change for what you're trying to solve)
since WCMD_fgets is used by most of the input methods, I want to be sure that this doens't break other things
I tested some more on my side, and it's not what I expected :-( https://testbot.winehq.org/JobDetails.pl?Key=117992
these tests need to be extended (cannot tell which one of the two new tests display nothing) (likely the contents1 displays nothing and for contents2 printf doesn't print the ctrl-z character)
anyway, I tried also to run this simple batch on windows:
"echo\necho\0\echo"
and it hangs after printing the first 'echo on' (so \0 isn't a generic EOF marker)so changing WCMD_fgets when running into \0 to return EOF isn't likely the right solution (at least as a generic solution) (and I still don't like in case of "abc\0" not returning the "abc" characters)
more tests are needed :-(
Hi Arek,
After testing a bit more locally, what I get is:
- no support for ctrl-z in for /F handling
- current code (before your patch) is looping endlessly in WCMD_fgets when a stray NUL character is read from a file
- native cmd.exe behavior (tested in W11) is inconsistent
- native cmd stops reading a line with a stray NUL (like your tests show) and throws it away
- native cmd loops endlessly in some cases (like when parsing a .bat/.cmd file) ("echo\necho\0echo" thingie)
So it boils down to what error handling we put in place in case of a NUL character in text file.
Your patch implies:
- never loop endlessly for all callers on NUL character
- NUL is an EOF marker
- throw away previous characters on line (if any)
So I'd rather fix WCMD_fgets with attached patch:
- never loop endlessly for all callers on NUL character (not consistent with native, but we can live with that)
- NUL is not an EOF marker
- just include the NUL in the returned buffer
- so if caller cares (like in 'for /F' handler), he can take appropriate actions
- if caller doesn't care (like the other actual callers in builtin cmd.exe, he will get the characters before the NUL, and continue on next line
But I agree that we likely haven't been hit by the differences above as it would be still be looping endlessly .
Ah. Thanks for testing! I didn't even know about the
^Z
behavior. A lot of the Windows things are fairly foreign to me.I was testing
WCMD_for()
while failing to recognize thatWCMD_fgets()
may be used by other things that behave a bit differently.Your attached patch looks better than my attempt at the fix and solves my problem. Can you make it in a proper patch so we can supersede this MR? I don't feel like claiming authorship on this one.
I did some extra testing myself and there's even more weirdness to
FOR /F
... It handles UTF-16 but only in some cases. When you try to read a UTF-16 file with a BOM it just stop on the first NUL byte, but if the output comes from a command (via USEBACKQ, one example would bewmic
) it seems to handle UTF-16 just fine. So it looks like there's some conversion happening when shelling out. But that looks like a separate problem.Hi Arek
regarding the UTF16 items:
- looks like the handling of file reading and command output (via pipes or similar) isn't fully consistent :-( - we may have to reconsider the unique WCMD_fgets implementation.
- yes separate issue (do you have an app depending on it?). At least this MR will get the case for reading from a file with a BOM right
added 195 commits
-
e2d9bc0e...fefe108e - 194 commits from branch
wine:master
- 4016f728 - cmd: Don't loop infinitely on NUL.
-
e2d9bc0e...fefe108e - 194 commits from branch
Filed bug for the UTF16: https://bugs.winehq.org/show_bug.cgi?id=53386
sorry to get back to this, but looking at the UTF16 bug report likely indicates that this patch (at least what I wrote ) doesn't go in the best direction
I'd recommand waiting until some more tests are done on patch in https://bugs.winehq.org/show_bug.cgi?id=53386
added 413 commits
-
4016f728...c86955d3 - 411 commits from branch
wine:master
- a2b06eb3 - cmd: Use CRT file I/O function inside 'for /F' handling.
- 93236478 - cmd/tests: Make sure for /f doesn't loop infinitely on NULs.
-
4016f728...c86955d3 - 411 commits from branch
added 1504 commits
-
93236478...e43cb422 - 1502 commits from branch
wine:master
- 2569e4a9 - cmd: Use CRT file I/O function inside 'for /F' handling.
- e3752945 - cmd/tests: Make sure for /f doesn't loop infinitely on NULs.
-
93236478...e43cb422 - 1502 commits from branch
assigned to @epo
Hi @ivyl is it just a rebase or did you change other things ? (can't get a diff between the two, gitlab says more that 1000 files changed)