malxau / yori

Yori is a CMD replacement shell that supports backquotes, job control, and improves tab completion, file matching, aliases, command history, and more.
http://www.malsmith.net/yori/
MIT License
1.24k stars 31 forks source link

YoriInit.ys1 script %__APPDIR__% and %__CD__% environment variables on Windows XP #118

Closed aleaksunder closed 9 months ago

aleaksunder commented 1 year ago

Hi!

%APPDIR% and %CD% environment variables description can be found here: https://ss64.com/nt/syntax-variables.html in "Undocumented Dynamic variables (read only)" section or here: https://ss64.org/viewtopic.php?t=338

I've noticed some strange behavior on Windows XP 32-bit, didn't tested 64-bit, but i guess this affects Windows 2003 Server too.

if I place this script in yori.exe directory:

echo %__APPDIR__%
echo %__CD__%
echo %CD%

this commands works well in yori on Vista+ systems: screenshot 3 lines of proper output

but on Windows XP it gives me this: screenshot the output is 4 lines

and if I double every line:

echo %__APPDIR__%
echo %__APPDIR__%
echo %__CD__%
echo %__CD__%
echo %CD%
echo %CD%

on Windows Vista it's ok but on XP I'm getting this: screenshot only 3 lines off some weird stuff

And I've noticed that it simply breaks after the 1st %__CD__% usage, so this:

echo %__APPDIR__%
echo %__APPDIR__%
echo %__APPDIR__%
echo %__CD__%
echo %__CD__%
echo %__CD__%
echo %CD%
echo %CD%
echo %CD%

on XP gives this: screenshot

This is really very strange behavior since this:

echo %__CD__%
echo %__CD__%
echo %__CD__%
echo %__APPDIR__%
echo %__APPDIR__%
echo %__APPDIR__%
echo %CD%
echo %CD%
echo %CD%

gives this: screenshot

this:

echo %CD%
echo %CD%
echo %CD%
echo %__CD__%
echo %__CD__%
echo %__CD__%
echo %__APPDIR__%
echo %__APPDIR__%
echo %__APPDIR__%

gives: screenshot

and this:

echo %CD%
echo %CD%
echo %CD%
echo %__APPDIR__%
echo %__APPDIR__%
echo %__APPDIR__%
echo %__CD__%
echo %__CD__%
echo %__CD__%

gives this: screenshot

malxau commented 1 year ago

Thanks, this looks like a(nother) Windows bug.

On Windows XP, the function that expands these looks like:

*lengthReturned = sizeOfNewString;
if (bufferSize >= sizeOfNewString) {
    memcpy(buffer, newString, sizeOfNewString);
}
return SUCCESS;

The "return success" part is a bug because the contract was to fail with an error indicating an insufficient buffer. This causes the function to complete back to the caller with uninitialized data.

But there's a second, worse bug, that looks like

if (InnerFunction(&lengthReturned) == SUCCESS) {
    buffer[lengthReturned] = '\0';
}

So in addition to leaving the buffer uninitialized, it will write to invalid memory. This one is harder to work around, and causes this to crash immediately when using a debug version of Yori (that ensures the next page is marked invalid.) So on XP, I think GetEnvironmentVariable either has to ensure these variables are not used, or the caller needs to supply a 64Kb buffer (being the largest representable string) to ensure the function can't fail due to insufficient buffer space.

I think the best fix is to detect these variables and handle them internally. Yori already handled %CD% so adding %__CD__% seems straightforward. %__APPDIR__% is very similar to ~appdir which Yori also already expands. (Frustratingly, %__APPDIR__% would be implemented with GetModuleFileName, which also can't indicate how much buffer it needs, and other uses of this function just use a pessimistic buffer size.)

aleaksunder commented 1 year ago

It's perfect this can be fixed someway. These variables sometimes have drastic value! At least for now i'm happy with ~appdir in .ys1 files