Skip to content
Snippets Groups Projects

cmd: Don't loop infinitely on NUL.

Merged Arek Hiler requested to merge ivyl/wine:cmd-fix-infinite into master

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • assigned to @ivyl

  • unassigned @ivyl

  • the changes to fgets don't look fully right you should return the characters in bufA until the NUL character (as done for \n) if there are some, and NULL in case there are none... (it works in your test case as the string 'a b c' is terminated by the new line)

  • Author Developer

    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 .

  • Author Developer

    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 that WCMD_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 be wmic) 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
  • Arek Hiler added 195 commits

    added 195 commits

    Compare with previous version

  • Author Developer

    Hi. I've updated the MR with your changes and the sign-off.

    Took me a while but I think I might have found something requires the UTF16 support, I'll check and follow up on it separately / file a bug.

  • Author Developer
  • Arek Hiler approved this merge request

    approved this merge request

  • 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

  • Arek Hiler added 413 commits

    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.

    Compare with previous version

  • Arek Hiler added 1504 commits

    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.

    Compare with previous version

  • 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)

  • Author Developer

    Just a rebase because it was sitting here for quite a while. I've included your changes from the linked bug the previous time.

  • eric pouech approved this merge request

    approved this merge request

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
Please register or sign in to reply
Loading