Fix read checks in shell file reader by NicknineTheEagle · Pull Request #3076 · dosbox-staging/dosbox-staging

NicknineTheEagle · 2023-10-31T20:08:56Z

Description

This fixes a problem in FileReader::Read where it was not properly checking if DOS_ReadFile has succeeded or not which, in some cases, caused batch script parser to get stuck in infinite loop.
Sample batch script inside archive: d2_19pat.zip

Manual testing

Ran PATCH.BAT from the attached sample. Dosbox hanged at the end of the script without the fix and worked fine with the fix.
Tested benchmark pack from Phil's Computer Lab: https://www.philscomputerlab.com/dos-benchmark-pack.html
Tested RUN.BAT from Bubble Bobble from eXo's pack.
Tested these cases:

Create a .BAT file that's empty (zero-bytes).
Create a .BAT file that contains a single 0 or null characters.
Create a .BAT that contains valid lines, but ends with a 0 or null instead of a carriage return.
Create a .BAT that contains valid lines but that doesn't end with a null or a CR LF (It just ends at the last valid character).
Create a .BAT that ends with CR LF.

No issues noticed.

Checklist

I have:

weirddan455 · 2023-10-31T20:41:09Z

This looks like it may be a more widespread issue. Lots of callers aren't checking the return value.

$ git grep DOS_ReadFile
include/dos_inc.h:bool DOS_ReadFile(uint16_t handle,uint8_t * data,uint16_t * amount, bool fcb = false);
src/dos/dos.cpp:                        DOS_ReadFile(STDIN,&c,&n);
src/dos/dos.cpp:                                DOS_ReadFile(STDIN,&c,&n);
src/dos/dos.cpp:                                DOS_ReadFile (STDIN,&c,&n);
src/dos/dos.cpp:                                DOS_ReadFile(STDIN,&c,&n);
src/dos/dos.cpp:                                        DOS_ReadFile(STDIN,&c,&n);
src/dos/dos.cpp:                        if (DOS_ReadFile(reg_bx,dos_copybuf,&toread)) {
src/dos/dos.cpp:                DOS_ReadFile(STDIN, &code, &count);
src/dos/dos_execute.cpp:        if (!DOS_ReadFile(fhandle,(uint8_t *)&head,&len)) {
src/dos/dos_execute.cpp:                                DOS_ReadFile(fhandle,loadbuf,&dataread);
src/dos/dos_execute.cpp:                                DOS_ReadFile(fhandle,loadbuf,&dataread);
src/dos/dos_execute.cpp:                DOS_ReadFile(fhandle,loadbuf,&readsize);
src/dos/dos_execute.cpp:                        readsize=0x8000;DOS_ReadFile(fhandle,loadbuf,&readsize);
src/dos/dos_execute.cpp:                        readsize=(uint16_t)imagesize;DOS_ReadFile(fhandle,loadbuf,&readsize);
src/dos/dos_execute.cpp:                        readsize=4;DOS_ReadFile(fhandle,(uint8_t *)&relocpt,&readsize);
src/dos/dos_files.cpp:bool DOS_ReadFile(uint16_t entry,uint8_t * data,uint16_t * amount,bool fcb) {
src/dos/dos_files.cpp:  if (!DOS_ReadFile(fhandle,dos_copybuf,&toread,true)) return FCB_READ_NODATA;
src/dos/program_intro.cpp:    DOS_ReadFile (STDIN,&c,&n);
src/dos/program_intro.cpp:    DOS_ReadFile (STDIN,&c,&n);
src/dos/program_intro.cpp:    DOS_ReadFile(STDIN, &c, &n);
src/dos/program_more_output.cpp:                DOS_ReadFile(STDIN, &code, &count);
src/dos/program_more_output.cpp:                DOS_ReadFile(STDIN, &tmp, &count);
src/dos/program_more_output.cpp:        DOS_ReadFile(input_handle, reinterpret_cast<uint8_t *>(&code), &count);
src/hardware/reelmagic/driver.cpp:                      if (!DOS_ReadFile(_pspEntry, data, &transactionAmount))
src/shell/file_reader.cpp:      DOS_ReadFile(handle, &data, &bytes_read);
src/shell/shell_cmds.cpp:                                                               DOS_ReadFile(sourceHandle, buffer, &toread);
src/shell/shell_cmds.cpp:               DOS_ReadFile(handle,&c,&n);
src/shell/shell_cmds.cpp:       DOS_ReadFile(STDIN, &c, &n);
src/shell/shell_cmds.cpp:               DOS_ReadFile(STDIN, &c, &n); // read extended key
src/shell/shell_cmds.cpp:               DOS_ReadFile(STDIN, reinterpret_cast<uint8_t *>(&choice), &bytes_read);
src/shell/shell_cmds.cpp:                               if (!DOS_ReadFile(source_handle, buffer, &bytes_requested)) {
src/shell/shell_misc.cpp:               while (!DOS_ReadFile(input_handle, &data, &byte_count)) {
src/shell/shell_misc.cpp:                       DOS_ReadFile(input_handle, &data, &byte_count);

I wonder if it's safe to modify DOS_ReadFileto set *amount = 0 on error if that's the thing the callers are actually checking. It would probably be useful to go through these and see what's up. In this case bytes_read got initialized to 1 by the caller for some reason.

NicknineTheEagle · 2023-10-31T20:53:11Z

https://github.com/dosbox-staging/dosbox-staging/actions/runs/6711865862/job/18240193833?pr=3076
Why did number of warnings go up by 4 here with none of them being in file_reader.cpp?

NicknineTheEagle · 2023-10-31T20:54:17Z

This looks like it may be a more widespread issue. Lots of callers aren't checking the return value.

$ git grep DOS_ReadFile
include/dos_inc.h:bool DOS_ReadFile(uint16_t handle,uint8_t * data,uint16_t * amount, bool fcb = false);
src/dos/dos.cpp:                        DOS_ReadFile(STDIN,&c,&n);
src/dos/dos.cpp:                                DOS_ReadFile(STDIN,&c,&n);
src/dos/dos.cpp:                                DOS_ReadFile (STDIN,&c,&n);
src/dos/dos.cpp:                                DOS_ReadFile(STDIN,&c,&n);
src/dos/dos.cpp:                                        DOS_ReadFile(STDIN,&c,&n);
src/dos/dos.cpp:                        if (DOS_ReadFile(reg_bx,dos_copybuf,&toread)) {
src/dos/dos.cpp:                DOS_ReadFile(STDIN, &code, &count);
src/dos/dos_execute.cpp:        if (!DOS_ReadFile(fhandle,(uint8_t *)&head,&len)) {
src/dos/dos_execute.cpp:                                DOS_ReadFile(fhandle,loadbuf,&dataread);
src/dos/dos_execute.cpp:                                DOS_ReadFile(fhandle,loadbuf,&dataread);
src/dos/dos_execute.cpp:                DOS_ReadFile(fhandle,loadbuf,&readsize);
src/dos/dos_execute.cpp:                        readsize=0x8000;DOS_ReadFile(fhandle,loadbuf,&readsize);
src/dos/dos_execute.cpp:                        readsize=(uint16_t)imagesize;DOS_ReadFile(fhandle,loadbuf,&readsize);
src/dos/dos_execute.cpp:                        readsize=4;DOS_ReadFile(fhandle,(uint8_t *)&relocpt,&readsize);
src/dos/dos_files.cpp:bool DOS_ReadFile(uint16_t entry,uint8_t * data,uint16_t * amount,bool fcb) {
src/dos/dos_files.cpp:  if (!DOS_ReadFile(fhandle,dos_copybuf,&toread,true)) return FCB_READ_NODATA;
src/dos/program_intro.cpp:    DOS_ReadFile (STDIN,&c,&n);
src/dos/program_intro.cpp:    DOS_ReadFile (STDIN,&c,&n);
src/dos/program_intro.cpp:    DOS_ReadFile(STDIN, &c, &n);
src/dos/program_more_output.cpp:                DOS_ReadFile(STDIN, &code, &count);
src/dos/program_more_output.cpp:                DOS_ReadFile(STDIN, &tmp, &count);
src/dos/program_more_output.cpp:        DOS_ReadFile(input_handle, reinterpret_cast<uint8_t *>(&code), &count);
src/hardware/reelmagic/driver.cpp:                      if (!DOS_ReadFile(_pspEntry, data, &transactionAmount))
src/shell/file_reader.cpp:      DOS_ReadFile(handle, &data, &bytes_read);
src/shell/shell_cmds.cpp:                                                               DOS_ReadFile(sourceHandle, buffer, &toread);
src/shell/shell_cmds.cpp:               DOS_ReadFile(handle,&c,&n);
src/shell/shell_cmds.cpp:       DOS_ReadFile(STDIN, &c, &n);
src/shell/shell_cmds.cpp:               DOS_ReadFile(STDIN, &c, &n); // read extended key
src/shell/shell_cmds.cpp:               DOS_ReadFile(STDIN, reinterpret_cast<uint8_t *>(&choice), &bytes_read);
src/shell/shell_cmds.cpp:                               if (!DOS_ReadFile(source_handle, buffer, &bytes_requested)) {
src/shell/shell_misc.cpp:               while (!DOS_ReadFile(input_handle, &data, &byte_count)) {
src/shell/shell_misc.cpp:                       DOS_ReadFile(input_handle, &data, &byte_count);

I wonder if it's safe to modify DOS_ReadFileto set *amount = 0 on error if that's the thing the callers are actually checking. It would probably be useful to go through these and see what's up. In this case bytes_read got initialized to 1 by the caller for some reason.

Damn, this looks bad. Definitely worth looking into but not sure if this is within the scope of this PR or even the current release.

FeralChild64 · 2023-10-31T20:54:58Z

Sometimes a minor/patch update to MSVC can make it report more warnings than before.

johnnovak · 2023-10-31T21:19:59Z

Damn, this looks bad. Definitely worth looking into but not sure if this is within the scope of this PR or even the current release.

Agreed. Best to address this after 0.81.0 is out. Should be fixed, but then this bug hasn't caused major issues so far.

NicknineTheEagle · 2023-10-31T21:55:27Z

Sometimes a minor/patch update to MSVC can make it report more warnings than before.

So what should I do in this case?

kcgen · 2023-11-01T16:57:18Z

I'm not seeing new warnings from this PR, so hopefully it's just a disconnect versus main.

@NicknineTheEagle , can you pull and rebase against main?

The last main build generated 875 warnings from MSVC:

https://github.com/dosbox-staging/dosbox-staging/actions/runs/6707497783/job/18226174137

kcgen · 2023-11-01T17:00:20Z

This is a useful fix and I think we should try to add it, but the reported testing:

Ran PATCH.BAT from the attached sample. Dosbox hanged at the end of the script without the fix and worked fine with the fix.

Doesn't report any regression testing using other (previously handled) .BAT files.

@NicknineTheEagle -> can you run this PR through a lot more .BAT files and report back?

NicknineTheEagle · 2023-11-01T17:04:02Z

This is a useful fix and I think we should try to add it, but the reported testing:

Ran PATCH.BAT from the attached sample. Dosbox hanged at the end of the script without the fix and worked fine with the fix.

Doesn't report any regression testing using other (previously handled) .BAT files.

@NicknineTheEagle -> can you run this PR through a lot more .BAT files and report back?

I would if I had any or knew which specific cases to test. Do you have a sample pack?

kcgen · 2023-11-01T17:36:13Z

This is a useful fix and I think we should try to add it, but the reported testing:

Ran PATCH.BAT from the attached sample. Dosbox hanged at the end of the script without the fix and worked fine with the fix.

Doesn't report any regression testing using other (previously handled) .BAT files.
@NicknineTheEagle -> can you run this PR through a lot more .BAT files and report back?

I would if I had any or knew which specific cases to test. Do you have a sample pack?

Phil's benchmark pack is launched with dosbench.bat, which is an elaborate batch file.

If you have any of the GNU tools available in WSL, you can use this to list all your batch files and sort them by size:

find . -type f -iname '*.bat' -exec du -h {} + | sort -hr | more

Here's Bubble Bobble from eXo's pack. It uses a decently large .BAT file with file copies, config-changes, pause, printing, and three levels of GOTO:

BubbBobb.zip

I've tested all audio device choices and confirmed they work.

kcgen · 2023-11-01T17:39:17Z

Also try it with your own cretions to exercise this specific code.

Create a .BAT file that's empty (zero-bytes).
Create a .BAT file that contains a single 0 or null characters.
Create a .BAT that contains valid lines, but ends with a 0 or null instead of a carriage return.
Create a .BAT that contains valid lines but that doesn't end with a null or a CR/LF (It just ends at the last valid character).

NicknineTheEagle · 2023-11-01T17:52:31Z

VS build still fails due to warnings. What the heck is happening?

kcgen · 2023-11-01T18:18:03Z

Hmm.. even compared the logged MSVC versions from the main build:

MSBuild version 17.7.2+d6990bcfa for .NET Framework

Versus this PR MSVC version (the same 😮‍💨 ):

MSBuild version 17.7.2+d6990bcfa for .NET Framework

My only guess is that Microsoft has done other updates that aren't reflected in these versions.. or the code lets the compiler "see" and warn about more stuff.

kcgen · 2023-11-01T18:20:47Z

VS build still fails due to warnings. What the heck is happening?

This other PR #3078 is also rebases against latest main and not showing the extra warnings.

So my hunch is there's something about this change that's revealing more warnings in "adjacent" code.

weirddan455 · 2023-11-01T18:25:40Z

It's using a newer version of the runner image for this PR (and strangely only this PR, not the 2 that were submitted after this).

Runner Image
  Image: windows-2022
  Version: 20231029.1.0
  Included Software: https://github.com/actions/runner-images/blob/win22/20231029.1/images/win/Windows2022-Readme.md
  Image Release: https://github.com/actions/runner-images/releases/tag/win22%2F20231029.1

Compared to main and other active PRs:

Runner Image
  Image: windows-2022
  Version: 20231024.1.0
  Included Software: https://github.com/actions/runner-images/blob/win22/20231024.1/images/win/Windows2022-Readme.md
  Image Release: https://github.com/actions/runner-images/releases/tag/win22%2F20231024.1

kcgen · 2023-11-01T18:40:21Z

Great catch with the runner versions, @weirddan455.

GitHub does gradual deployments, so eventually all new CI runs will be moved over to this newer version.

@NicknineTheEagle , suggest bumping up the warning limit in your PR, as your changes are not the culprit.

NicknineTheEagle · 2023-11-01T19:34:47Z

Ran all the suggested batch script tests. No issues noticed. No hangs at the end.

kcgen

Ok, let's merge this with regression tests confirmed.
Thanks, @NicknineTheEagle.

NicknineTheEagle added bug Something isn't working regression We broke something 😊 labels Oct 31, 2023

weirddan455 requested review from FeralChild64, johnnovak and kcgen October 31, 2023 20:41

Fix read checks in shell file reader

829ec79

NicknineTheEagle force-pushed the nn/freader branch from 3020a5f to 829ec79 Compare November 1, 2023 17:03

kcgen reviewed Nov 1, 2023

View reviewed changes

Comment thread src/shell/file_reader.cpp

Raise allowed warnings for MSVC 64-bit build

ef2525b

NicknineTheEagle requested a review from kcgen November 1, 2023 19:34

kcgen approved these changes Nov 1, 2023

View reviewed changes

kcgen assigned NicknineTheEagle Nov 1, 2023

kcgen merged commit c577254 into main Nov 1, 2023

NicknineTheEagle deleted the nn/freader branch November 1, 2023 22:26

Grounded0 mentioned this pull request Nov 17, 2023

DOSBox Staging 0.81 release steering ticket #3091

Closed

4 tasks

johnnovak added the DOS Issues related to DOS integration or DOS commands label Dec 11, 2023

Uh oh!

Conversation

NicknineTheEagle commented Oct 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Manual testing

Checklist

Uh oh!

weirddan455 commented Oct 31, 2023

Uh oh!

NicknineTheEagle commented Oct 31, 2023

Uh oh!

NicknineTheEagle commented Oct 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FeralChild64 commented Oct 31, 2023

Uh oh!

johnnovak commented Oct 31, 2023

Uh oh!

NicknineTheEagle commented Oct 31, 2023

Uh oh!

kcgen commented Nov 1, 2023

Uh oh!

kcgen commented Nov 1, 2023

Uh oh!

NicknineTheEagle commented Nov 1, 2023

Uh oh!

kcgen commented Nov 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kcgen commented Nov 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

NicknineTheEagle commented Nov 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kcgen commented Nov 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kcgen commented Nov 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weirddan455 commented Nov 1, 2023

Uh oh!

kcgen commented Nov 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NicknineTheEagle commented Nov 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kcgen left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

NicknineTheEagle commented Oct 31, 2023 •

edited

Loading

NicknineTheEagle commented Oct 31, 2023 •

edited

Loading

kcgen commented Nov 1, 2023 •

edited

Loading

kcgen commented Nov 1, 2023 •

edited

Loading

NicknineTheEagle commented Nov 1, 2023 •

edited

Loading

kcgen commented Nov 1, 2023 •

edited

Loading

kcgen commented Nov 1, 2023 •

edited

Loading

kcgen commented Nov 1, 2023 •

edited

Loading

NicknineTheEagle commented Nov 1, 2023 •

edited

Loading