NSIS in the news...

Read this on /.

Shortly after releasing EVE Online: Trinity at 22:04 GMT on Wednesday, 5 December, we started receiving reports that the Classic to Premium graphics content upgrade was causing problems to players by deleting the file C:\boot.ini, which is a Windows system startup file.[...]

It might appear from the installer log that we made the mistake of explicitly deleting \boot.ini instead of just boot.ini. The former would delete the file from the root of the current drive whereas the latter would only delete from the current working directory. It is not quite so; the mistake was to assume that the file would be deleted from the current working directory without giving the full path. In fact, this is the code in the installer script that caused the problems:

Section "EVE-ONLINE"
SetOutPath "$INSTDIR"

Delete "boot.ini"
Delete "manifest.dat"

File "boot.ini"
File "manifest.dat"
File "resDX9*.stuff"
SectionEnd

The assumption was that by setting SetOutPath "$INSTDIR" then all commands that followed would use that working directory.

The File commands use the output path that has been set. The documentation for the Delete function says the file _should_ be specified with a full path but in fact it _must_ be specified with a full path, like so:

Delete "$INSTDIR\boot.ini"
Delete "$INSTDIR\manifest.dat"

Otherwise, it is assumed that the file should be deleted from the root.

Oops 🙂

That code of theirs was actually supposed to work. I've fixed the bug behind this that caused it to delete boot.ini from the root instead of the working directory.

And for the sake of a complete discussion, CCP's blog post:



EVE user comments:



Slashdot article:



I really liked the last comment on the article:

Having mixed, inconsistent addressing modes is a major design screw-up. And having a default of the "delete" command to delete somewere else than is intuitive, is completely unacceptable and the people responsible for it should be fired and striped of their qualification. These people are not engineers, but incompetent hacks.

The right way to do it is that you can have inconsitencies, if a) there is a very good reason for it (here, there was none) and b) you make sure people notice. This can be done by making the command look different, e.g. "DeleteWithAbsolutePathOrRoot" or doing additional checking, like the delete command refusing to work if it is not given an absolute path. Of course documentation must also be correct, but the real screw-up is the command that looks just like the "file" command but works differently in an obviously potentially destructive fashion.

So I guess there'd be no paycheck next month...

That code of theirs was actually supposed to work.

So until today's fix, unless given an absolute path, Delete() will silently delete files from the root directory? I guess the documentation should have stressed this.

But then, they should have noticed this during testing.

You're fired 😉

Nope, that's not what happened. If Delete was given a path relative to the working directory ("blah.txt" but not "..\blah.txt" or ".\blah.txt" or "$INSTDIR\blah.txt"), it'd look for files in the working directory, but try deleting them from the root directory. This happened because filename was erroneously prefixed with a backslash. More accurately, the directory from which the files were deleted was suffixed with a backslash even though it was an empty string. The log was updated accordingly, saying it deleted "\blah.txt".