Long command line "Out of Memory"; Set /E, SET /P bug

[shidel]

This test was performed on FreeCOM running inside DOSBox using the DOSBox kernel.

I'm sorry, I wasn't thinking and did something dumb in a batch file which caused FreeCOM to break... Again.

My intent was to create a simple crazy big text file for some testing. I was doing this using a couple V8Power Tools programs using this batch:

@echo off

set max=50
set line=0
set /e str=vstr /r 235 /c 0xfe
:repeating
if "%line%" == "%max%" goto done
set /e line=vmath %line% + 1
set /e hex=vmath %line% /h
echo %hex% %str%
goto repeating
:done

The vstr /r 235 /c 0xfe is repeat the next option 235 times, the next option being output ASCII 0xfe to StdOut.
The vmath %line% + 1 is add 1 to %line%
The vmath %line% /h is output result as hexadecimal.

The obviously dumb thing I did was repeating the character 235 times. This exceeds the maximum line length of 125 characters in batch files. Running this batch resulted in this output:

All of those errors about the length exceeding the maximum should be expected. However, the "out of memory error." was a surprise. Even more of a surprise is that the error is non-recoverable and required closing DOSBox.

By reducing the number of repeated output characters to a sane number like 40, the batch will happily iterate through the requested lines until the max value is reached without any issues.

If I REM out the echo %hex% %str% and keep the 235 character output, the issue still occurs.

So, I ran a bunch of tests using different repeat counts. Everything worked fine until I used 231 characters for the repeat count. Then the Out of Memory Error occurs.

Interestingly, at 230 characters the batch generates no errors and creates output like this:

I just realized I should perform this test under the latest FreeCOM build created by ECM. It is actually worse. When run it looks like this:

However, after pressing a key. It sometimes causes DOSBox to close. It sometimes generates things like this and freezes:

Like the older version of FreeCOM, the latest build is fine if the repeated character length is 230 or less.

[shidel]

I decided to do a little more testing under FreeCOM 0.85b

Changing the line set /e str=vstr /r 235 /c 0xfe to vstr /r /231 /c 0xfe | set /p str= yields slightly different results after the first "out of memory error" But, it also eventually freezes.

While I do not claim that either VSTR or VMATH are bug free. Both those tools are used extensively by the FreeDOS installer to iterate through large lists and files. Any issue with those basic operations of those programs would have been encountered years ago.

However to eliminate the possibility of some sort of bizarre interaction between VSTR/VMATH and FreeCOM, I created a simple text file containing 231 characters (+ CRLF, 233 bytes) called chars.txt and changed the batch to simply:

@echo off

set hex=0x0000000000000000
type chars.txt | set /p str=
:repeating
echo %hex% %str%
goto repeating

This produces similar results. The batch exited with an "Out of memory error." and returned to the command line. I typed "dir" and hit enter. It output something like the last image in my initial post and froze.

I should mention that for all these tests of FreeCOM, the shell is started in non-permanent mode with a 2k environment. Only 431 bytes of the environment are in use and about 1.5K of the environment table is still free.

[boeckmann]

@shidel the last example you mentioned, shouldn't the type | set line be included in the loop? I changed it accordingly but could not reproduce the error with set /p str= with either the 0.85a release and a Watcom build of 0.85b.

However, deducing from the source there is a memory corruption occuring at

freecom/cmd/set.c

Line 102 in 9ec4779

value[1] = '\0'; /* strip trailing newlines */

under the condition that the data has NO trailing CR and/or NL.

The data is read here:

freecom/cmd/set.c

Line 95 in 9ec4779

len = dos_read(0, promptBuf, promptBuffer);

This reads up to promptBuffer=256 bytes into a 256 byte buffer. So value[1] becomes the 257th byte of the buffer.

[boeckmann]

set hex=0x0000000000000000
:rep
type test.txt | set /p str=
echo %hex% %str%
goto rep

[boeckmann]

"Commandline longer than 125 characters" actually comes from the echo %hex% %str% line. Curiously echo %str% works fine, despite %str% being longer than 125 characters.

[boeckmann]

Update: Can reproduce it. It only took longer...

[boeckmann]

This smells like a memory leak and is probably the cause of the trouble:

freecom/shell/command.c

Lines 688 to 693 in 9ec4779

	if((evar = getEnv(ip)) != 0) {
	if(cp >= parsedMax(strlen(evar)))
	return 0;
	cp = stpcpy(cp, evar);
	free(evar);
	} else if(matchtok(ip, "ERRORLEVEL")) {

If the command is too long, then evar is not freed.

[shidel]

On Jul 5, 2024, at 4:59 PM, Bernd Böckmann ***@***.***> wrote: @shidel the last example you mentioned, shouldn't the type | set line be included in the loop?It did not need to be. Which made it even more interesting. I changed it accordingly but could not reproduce the error with set /p str= with either the 0.85a release and a Watcom build of 0.85b.Interesting you could not reproduce the issue.Initially was under 85Aa. And then I switched to 85b which I think ECM said was compiled with IA. When you used set /p did you feed it a file that had a single line over 230 characters?However it is possible that the bug only appears under the DOSBox kernel and not “pure” FreeDOS.  However, deducing from the source there is a memory corruption occuring at https://github.com/FDOS/freecom/blob/9ec47792953100eea6c9a18b0bed34a4471a9a6a/cmd/set.c#L102 under the condition that the data has NO trailing CR and/or NL. The data is read here: https://github.com/FDOS/freecom/blob/9ec47792953100eea6c9a18b0bed34a4471a9a6a/cmd/set.c#L95 This reads up to promptBuffer=256 bytes into a 256 byte buffer. So value[1] becomes the 257th byte of the buffer.That could do it. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

[shidel]

On Jul 5, 2024, at 5:37 PM, Bernd Böckmann ***@***.***> wrote: Update: Can reproduce it. It only took longer...Ah, ok. :-) —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

[shidel]

Regarding the placement of the lopping... Nope the set /p does not need to be inside the loop. Which makes it much more interesting. :-)

It probably would do this without the %hex% variable as long as the %str% variable is long enough. However, I did not try that. The only reason for the %hex% was to assist me in figuring out what was causing problems in VVIEW.

(Side note: Basically VVIEW would reset to the start of the file after paging down a few dozen times. After some time I narrowed the problem down to the file buffering system. But after staring at that section for a long time, I just was not seeing the issue. However while looking at it, I realized that I was using a completely wrong process. I was thinking too much along the lines of programing in a HLL and not in ASM. It was far more efficient to change that piece entirely. So, I just rewrote that little section and the problem was solved.)

[boeckmann]

@shidel can you confirm that the following binary does not run out of memory anymore?
https://nextcloud.iww.rwth-aachen.de/index.php/s/kdHQtdZi57fnMno

[shidel]

@boeckmann in part, yes. But, not completely and some other severe problems now exist...

I dropped that build into DOSBox in the two places COMMAND.COM exist. C:\ and C:\FREEDOS\BIN.

The default FDAUTO.BAT used by the FreeDOS installer uses for hybrid mode froze during startup and did not return from the MEM /C /N in that file.

So, I renamed FDAUTO.BAT to avoid it starting when DOSBox launched.

I then ran the new build as command.com /e:2048, manually set the PATH C:\FREEDOS\BIN, and ran CRASH2. I could not tell if it was still running or frozen. So, I added a time /t to the 3 different crash test batch files and tried again. Both CRASH2 and CRASH3 seem fine now. However, CRASH1 only outputs the first current time and freezes.

Crash test file contents:

REM CRASH1
@echo off
set max=50
set line=0
type chars.txt | set /p str=
:repeating
time /t
if "%line%" == "%max%" goto done
set /e line=vmath %line% + 1
time /t
set /e hex=vmath %line% /h
time /t
echo %hex% %str%
goto repeating
:done

REM CRASH2
@echo off
set hex=0x0000000000000000
type chars.txt | set /p str=
:repeating
time /t
echo %hex% %str%
goto repeating

REM CRASH3
@echo off
set hex=0x0000000000000000
:repeating
type chars.txt | set /p str=
time /t
echo %hex% %str%
goto repeating

So, I then I went and tried another test. Not running the default FDAUTO.BAT, I relaunched DOSBox and manually loaded the command and set the path.

(Screen shot below)

I ran mem /c/n and it returned to the prompt. I pressed the up arrow key to use the history but nothing happened. So, I typed mem /c/n again. When I pressed return, there was no CRLF performed and it froze. DOSBox entered a race condition and the system fans spun up as more power was drawn. I needed to force quit DOSBox.

[boeckmann]

Interesting. I can reproduce it. This only occurs when spawning an external process. So perhaps the history buffer gets corrupted when spawning a process or something similar is going on. I will try to find out what is going on...

[shidel]

Also, I should have said that "Yes, SET /P seems to be working correctly" but "SET /E is now causing it to freeze immediately." with the build you sent me. :-)

[boeckmann]

I think this was a bad binary. I rebuilt it and now this error does not occur anymore. I can spawn external processes and use the history. Strangely a single byte changed between the files, from 0x00 to 0x18?!?

Can you try the newly uploaded one? I currently do not have the powertools installed in DosBox...

[shidel]

Same URL?

[boeckmann]

Yes: https://nextcloud.iww.rwth-aachen.de/index.php/s/kdHQtdZi57fnMno

[shidel]

Looks good now. :-)

It once again can start the default FDAUTO.BAT for the hybrid install inside DOSBox.

Now, CRASH2 and CRASH3 still work and do not crash.

CRASH1 using V8Power Tools, ran through the 50 iterations without issue. I increased it to 1000. No problem detected.

Ran mem /c/n a bunch of times, sometimes by typing, sometimes from history. A-Okay.

The Long command line "Out of Memory" SET /E and SET /P issue appears to be fixed now.

:-)

[boeckmann]

Great. But I wonder what caused the changed byte. Perhaps some .OBJs of different build modes or languages got mixed up. I aborted some builds by CTRL+C. Will do a clean build to see if it still outputs a working version.

[boeckmann]

Yes, clean build still works. So I will prepare a merge request for this.

[shidel]

There is also testing the build when compiled with IA32.

[boeckmann]

There is also testing the build when compiled with IA32.

You mean GCC-IA16?

[shidel]

Yeah, that one. Don't know why I said 32. at least I didn't say IA256. :-)

[boeckmann]

I added this to my 0.85_fixes branch there is a merge request open which fixes also the quotation of the exe name.
#104