Prevent ~/.ash_history from being saved

[neodyne]

As a way to prevent ash history from being saved, for example when busybox.exe shell is run from a flash drive, could busybox sh -l and busybox sh -s -c ". X:/path/to/.profile" support:

~ $ cat ~/.profile
export HISTFILE=/dev/null # or =NUL ?

Currently the shell after sourcing .profile just closes, after typing any command. export HISTFILE=X:/valid/path/to/file works, but obviously creates the file.

It seems the only other alternative would be something like ln -s /dev/null ~/.ash_history, so MKLINK.EXE "%USERPROFILE%\.ash_history" NUL: , which requires elevation and is system-specific.

(or to recompile with changed history-related config options)

[avih]

When running from sh:

HISTFILE=/dev/nul sh

then the 1st command results in sh exit with the following error:

sh: lseek(0, 2): Invalid seek

The only lseek I can see in ash.c is added at commit 6fb17897d620572290b1f604430c06c261145aad .

It's possible that this fails when trying to open/append to NUL HISTFILE, but I didn't try to confirm that.

It's not guaranteed to be the failed lseek, and code-wise that lseek seems to only be used from the ash-internal redirect, but I wouldn't guess that opening HISTFILE ends up using this function.

[rmyorston]

To prevent history from being recorded in a file set HISTFILE to an empty value in .profile:

export HISTFILE=

History will still be kept in memory, but not stored to a file. This also works in upstream BusyBox.

[rmyorston]

@avih I expect the lseek() you're seeing is the one in libbb/lineedit.c.

[neodyne]

To prevent history from being recorded in a file set HISTFILE to an empty value in .profile:

This works, thank you.

[avih]

@avih I expect the lseek() you're seeing is the one in libbb/lineedit.c.

I guess so, but on linux HISTFILE=/dev/null busybox sh seems to work.

Maybe lseek(fd, 0, SEEK_END) on windows shouldn't fail for NUL file?

[rmyorston]

The wrapper for lseek() was rather cautious and only accepted files on disk. Character devices seem to be OK too, so I've added support for them.

It's now possible to use HISTFILE=/dev/null or HISTFILE=nul to prevent history from being saved. At least, in prerelease binaries from PRE-5378.

[avih]

The wrapper for lseek() was rather cautious and only accepted files on disk. Character devices seem to be OK too, so I've added support for them.

Nice.

In addition, the wrapper for open(2) now converts the Unix-style mode argument to Windows-style.

It's not immediately obvious to me why this change was added or how it's related to the lseek change.

Does this solve any known problem and/or modify the behavior of some known things?

Or is that a general compatibility enhancement which might become useful in the future or fix existing but unknown issues?

[rmyorston]

The change to the mode argument isn't necessary for the problem at hand. It's just something I noticed while investigating the matter.

There are plenty of calls to open(2) with O_CREAT and a mode argument in the upstream code. I haven't audited them to look for problems but it seemed reasonable to make the change as a precaution.

[rmyorston]

Actually, the definitions of the mode bits Windows wants are compatible with the user mode bits on Unix. So the code was probably OK even without the change.

[avih]

Hmm.. I don't think all the new code is required.

According to the ms docs of _open, the pmode argument is ignored when the file exists, and when it doesn't exist it defines the permission after it's created and closed (as either read-only or read-write).

So why not simply use hardcoded pmode of _S_IREAD | _S_IWRITE regardless of mode (or just _S_IWRITE which is the same according to the docs), and remove some no-op logic?

If a value other than some combination of _S_IREAD and _S_IWRITE is specified for pmode—even if it would specify a valid pmode in another operating system—or if any value other than the allowed oflag values is specified, the function generates an assertion in Debug mode and invokes the invalid parameter handler.

This is weird. On one hand it suggest that anything else is invalid, but I doubt 0666 (the previous pmode value) is considered valid, but I also doubt it was always failing when called with 2 args (and mode defaults to 666) ?

Either way, a const value of _S_IREAD | _S_IWRITE should cover the use cases without additional logic, unless a mode (not pmode) of _O_READ | _O_CREATE create the file if it's missing, and we expect it to remain read-only after close? (because pmode would be _S_IREAD in this case with the new code).

EDIT: wait. I'm confused. Need to re-read the diff more carefully.

[avih]

OK, first, I think mode doesn't need an initialization to 0666 because it's always overwritten later as mode = va_arg(args, int).

However, I'm not sure what value it would get if it's called with only 2 args. I think it's undefined behavior?

C99 va_arg:

If there is no actual next argument, ... the behavior is undefined

I might be missing something because otherwise I don't know how it can be distinguished in C whether there are 2 or 3 args, unlike printf for instance where the number of arguments is derived from the format string.

EDIT: pmode is only expected when flags include _O_CREAT according to ms docs. That's also how musl does that: https://git.musl-libc.org/cgit/musl/tree/src/fcntl/open.c . So I think it's indeed UB when it's only called with two args. On linux:

The mode argument (windows: pmode) must be supplied if O_CREAT or O_TMPFILE is specified in flags; if it is not supplied, some arbitrary bytes from the stack will be applied as the file mode.

Next. Assuming we got past that hurdle and mode is either the 3rd argument or has a default value of 0666 (or maybe it should be changed to _S_IREAD | _S_IWRITE or 0), then with the knowledge that the bits are compatible, it should suffice to do pmode = mode & (_S_IREAD | _S_IWRITE) (or just use that expression directly).

[rmyorston]

I'd been following the same thought process and got as far as:

diff --git a/win32/mingw.c b/win32/mingw.c
index 4398f5462..0fc4d293c 100644
--- a/win32/mingw.c
+++ b/win32/mingw.c
@@ -239,9 +239,11 @@ int mingw_open (const char *filename, int oflags, ...)
        return fd;
    }

-   va_start(args, oflags);
-   mode = va_arg(args, int);
-   va_end(args);
+   if ((oflags & O_CREAT)) {
+       va_start(args, oflags);
+       mode = va_arg(args, int);
+       va_end(args);
+   }

    pmode = ((mode & S_IWUSR) ? _S_IWRITE : 0) |
                    ((mode & S_IRUSR) ? _S_IREAD : 0);

We don't need to worry about O_TMPFILE because BusyBox doesn't use it and it isn't supported on Windows.

[avih]

I'd been following the same thought process and got as far as: ...

LGTM.

What about the default mode value, possibly as 0, and simpler pmode = mode & (_S_IREAD | _S_IWRITE) ?

We don't need to worry about O_TMPFILE because BusyBox doesn't use it and it isn't supported on Windows.

It's not posix, and I don't know if identical semantics as linux, but ms docs say:

_O_CREAT | _O_TEMPORARY: Creates a file as temporary; the file is deleted when the last file descriptor is closed. The pmode argument is required when _O_CREAT is specified. To preserve legacy behavior for app-compatibility, other processes aren't prevented from deleting this file.

But on Windows it's apparently only supported with _O_CREAT anyway (I think?), so no need for special handling as far as pmode goes, except maybe adding _O_CREATE to flags if _O_TEMPORARY is set, though it's likely not worth adding unlees needed and someone reports a bug that a temp file remains someplace ;)

[rmyorston]

pmode = mode & (_S_IREAD | _S_IWRITE)

This relies on the Unix and Windows mode bits being compatible. Although this is apparent from reading the include files it isn't guaranteed by the documentation.

_O_TEMPORARY isn't in Linux, so it'll never be used in BusyBox. It's safe to ignore it.

[avih]

There might be another issue.

Either mingw_xopen is unused and should be removed (I don't see what maps to it), or it is used, but I think buggy:

xopen (which takes exactly two args) is sometimes called with O_CREAT, then calls xopen3(name, flags, 0666), but mingw_xopen takes two args and calls mingw_open with those two args, presumably even with O_CREAT, which can lead to incorrect pmode.

Regardless of the UB, presumably the possible bugs are either that the call is rejected because pmode is invalid, or the file is created as read-only instead of read-write (because in windows all files are readable, and the only imact pmode has is to decide whether it's also writable).

Regardless, the pmode docs are unclear to me:

If a value other than some combination of _S_IREAD and _S_IWRITE is specified for pmode ... or if any value other than the allowed oflag values is specified, the function ... invokes the invalid parameter handler, as described in Parameter validation. If execution is allowed to continue, the function returns -1 and sets errno to EINVAL.

Because _S_IREAD | _S_IWRITE (0x180) is different than the combined "allowed oflag values" (which is definitely more than the two set bits of 0x180, e.g. _O_RDWR is 2), so if the valus is e.g. 2, then it's not "some combination of _S_IREAD and _S_IWRITE", therefore it's invalid according to the 1st condition but it is an allowed oflag value, so it's not invalid not according to the second condition, so it's not obvious whether it's indeed invalid, and how it should be interpreted - because it only documents combinations of _S_IREAD and _S_IWRITE.

EDIT: OK, I think what it wants to say is that if pmode is not a combination of _S_IREAD and _S_IWRITE, or if oflag includes something which is not a specified oflag value, then it's invalid. I.e. the 1st condition refers to pmode, and the 2nd to oflag.

[rmyorston]

Calls to mingw_xopen() are constructed by the macro MINGW_SPECIAL(). All such calls are in relation to opening special devices (/dev/zero or /dev/urandom). These devices are assumed to exist (and even if they didn't there'd be no point in creating them). So calls to mingw_xopen() don't include O_CREAT.

the 1st condition refers to pmode, and the 2nd to oflag.

Yes, that was my understanding of the text.

[avih]

Thanks.

Is it waiting for anything else? currently master still has this UB with open with two args.

[rmyorston]

Is it waiting for anything else?

Just for your final confirmation. Now shipped: PRE-5381 or above.

[avih]

I didn't think there were open things left, and didn't realize you're waiting for me. Thanks for the update.