236312 - Windows death after upgrade (PendingFileRenameOperations registry corruption)

Status: RESOLVED FIXED

(SeaMonkey :: Installer, defect)

Description

[Martin Sheppard]

User-Agent:       
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6) Gecko/20040113

Mozilla install stuffs up HKLM\SYSTEM\CurrentControlSet\Control\Session 
Manager\PendingFileRenameOperations

Reproducible: Always
Steps to Reproduce:
1. Install Windows Security Patch
2. Install Mozilla


Actual Results:  
HKLM\SYSTEM\CurrentControlSet\Control\Session 
Manager\PendingFileRenameOperations was modified so that it deleted critical 
system files

Expected Results:  
No Change to HKLM\SYSTEM\CurrentControlSet\Control\Session 
Manager\PendingFileRenameOperations

I don't believe that this happened with the Mozilla 1.5 installer

Comment 1

[WD]

That reg key is used by the MoveFileEx() API call to replace or delete in-use
files.   The Windows Security Patch you installed surely uses that call.   I'm
sure the Mozilla installer also uses it to replace any in-use files.

No bug -> INVALID

Comment 2

[Martin Sheppard]

Attachment: QChain output showing contents of PendingFileRenameOperations key at various stages during install

Comment 3

[Martin Sheppard]

Ok, I wasn't specific enough in my original description. Here are the exact
details of reproducing the bug:

- Start with Windows 2000 SP3 (or SP4), bare install
- Install Mozilla 1.5 with Default Settings
- Reboot (opotional)
- Install Windows2000-KB824146-x86-ENU.exe (or any other Windows patch) without
rebooting
- Run QChain.exe (not necessary, but produces nice text file showing issue)
- Install Mozilla 1.6 with Default Settings
- Run QChain.exe (not necessary, but produces nice text file showing issue)
- Reboot and watch Windows delete important system files

The problem appears to be that when Mozilla 1.6 is installed it will first
uninstall mozilla 1.5. The Mozilla 1.5 uninstaller adds a
PendingFileRenameOperation to delete the uninstall executable. Next, the Mozilla
1.6 installer reads the PendingFileRenameOperations key, sees that it will
delete its own uninstaller after the reboot and removes the delete operation
from the key. The trouble is that if there are other operations in the key then
it mangles them for some reason with the effect that on the next reboot, system
files are deleted rather than being replaced with a newer version.

See the attachment which shows how the registry key changes through the install
process.

The best way to implement this would probably be to create a temporary file for
the new uninstaller and use MoveFileEx, so that on the next reboot the old
Mozilla 1.5 uninstaller will be delted and then the new uninstaller will be
renamed to replace it.

This is a serious bug - depending on which Windows updates you installed it can
render your computer unbootable and it has done this to a number of our computers.

Comment 4

[WD]

Ok.  That description is much better.
It appears that there is something wrong with the
ClearWinRegUninstallFileDeletion function :
http://lxr.mozilla.org/seamonkey/source/xpinstall/wizard/windows/setup/extra.c#516

The installer does use MoveFileEX() to replace in-use files, but judging by the
comments above the ClearWinRegUninstallFileDeletion() function, the installer is
manually editing the contents of the PendingFileRenameOperations key.  (And
corrupting it?)

Comment 5

[WD]

Confirming.

Easier steps to reproduce:  (Without doing windows updates and risking damage to
your system)
1. Install MoveOnBoot
http://www.webattack.com/get/moveonboot.html
2. Create two text files.  Eg. "testnew.txt" and "testold.txt"
3. Right-click one of the files and use MoveOnBoot to move one file to the
other.  (To simulate replacing an in-use file)
4. Look at the following Registry Value:
HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Control\Session
Manager\PendingFileRenameOperations
5. Run Mozilla installer for newer version than is currently installed. (eg. 1.5
-> 1.6, or 1.6 -> Nightly)
6. Look at the following Registry Value:
HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Control\Session
Manager\PendingFileRenameOperations

Expected Results:
The values should be *identical*

Actual Results:
The registry value is different after the mozilla installer processes it.  After
processing there are two problems:
1) There is an extra NULL character at the end.  (Perhaps not critical)
2) The '\' character that separates the designated source file and the
destination file is replaced with a NULL.  (essentially changing every single
"replace" operation with two "delete" operations)

Comment 6

[WD]

Attachment: Output from qchain.exe and regedit exports, compared

Comment 7

[WD]

The problem is that the PendingFileRenameOperations registry value can have
items contained in it of two types:

1) szDestFile\0\0              (for file deletes)
2) szSrcFile\0szDstFile\0\0    (for file replacement)

The ClearWinRegUninstallFileDeletion() function is treating the above items as
strings, and due to the embedded null in the second case is treating it as two
separate items when it should be one.   When it reads in and re-writes the
registry value, it is being corrupted.

Comment 8

[WD]

Attachment: Patch to enable proper parsing of PendingFileRenameOperations

This patch puts a single null between arguments of a single move command, and
double nulls between items of the PendingFileRenameOperations registry value.

Comment 9

[Martin Sheppard]

The patch looks a little complicated to me - it feels to me like there there 
must be a slightly simpler algorithm, but I'm not sure what it might be since I 
haven't looked at all the code in that function, only the patch.

A possible alternative which doesn't involve messing with the 
PendingFileRenameOperations registry key at all is the following algorithm:

1. Copy uninstaller to C:\winnt\uninstallername.exe and to c:\winnt\sometempfile
2. Execute MoveFileEX(c:\winnt\sometempfile, C:\winnt\uninstallername.exe, 
DELAY till next reboot)

On the next reboot the uninstaller will be removed and then replaced. Also, an 
uninstall will still work correctly even if executed before the reboot happens.

I don't care which implementation is used, but I thought I'd suggest it as an 
alternative.

Martin.

Comment 10

[Andrew Schultz]

Comment on attachment 143085 [details] [diff] [review]
Patch to enable proper parsing of PendingFileRenameOperations

WD: you're adding tabs in a few places.  Tabs are evil.  See 
http://www.mozilla.org/hacking/mozilla-style-guide.html#Visual

Also, you're patch has DOS linefeeds, so it won't apply (at least on Unix).  I
don't know the best way to remove them.  If you generate a cvs diff, I think
they won't be there.

Comment 11

[WD]

Attachment: Cleaned up version of patch (no ^M or tabs)

Comment 12

[Benjamin Smedberg]

I will not be able to review this for a week at least.

Comment 13

[Daniel Veditz [:dveditz]]

This probably should block 1.7

Firefox has its own copy of the installer, it'll have this bug plus any others
we've fixed since the fork (bad Ben, no biscuit).

the OS/2 installer also appears to have its own copy, but that makes quite a bit
more sense given the OS-specific calls in an install. I can't find an analog of
this section in there, but cc'ing mkaply just to make sure.

Sean, any thoughts?

Comment 14

[Mike Kaply [:mkaply]]

definitely a blocker but not an OS/2 issue - we don't have the same sort of
cleanup stuff as Windows.

Comment 15

[Bernard Alleysson]

This is a very serious bug. It can render your system unusable. 

It's easy to run into this especially when you download and install windows
updates automatically, there are times when you don't want to reboot. Personally
I've run into this and I got a blue screen (I couldn't login) because of missing
user32.dll, I had to reinstall windows !

IMHO the patch here should be reviewed ASAP and at this point no Mozilla release
should happen until this bug is fixed.

Comment 16

[timeless]

Comment on attachment 143535 [details] [diff] [review]
Cleaned up version of patch (no ^M or tabs)

>+  BOOL  bPendingFileRename = FALSE;
>+  int szInMultiStrIndex = 0;
> 
>   if(!GetWindowsDirectory(szWinDir, sizeof(szWinDir)))
>     return;
> 
>   wsprintf(szLCUninstallFilenameLongBuf, "%s\\%s", szWinDir, sgProduct.szUninstallFilename);
>   GetShortPathName(szLCUninstallFilenameLongBuf, szLCUninstallFilenameShortBuf, sizeof(szLCUninstallFilenameShortBuf));
>@@ -559,26 +561,56 @@ void ClearWinRegUninstallFileDeletion(vo
>         if(!strstr(szLCKeyBuf, szLCUninstallFilenameLongBuf) && !strstr(szLCKeyBuf, szLCUninstallFilenameShortBuf))
>         {
a local for lstrlen(szPtrIn) seems reasonable:
>           if((dwOutMultiStrLen + lstrlen(szPtrIn) + 3) <= sizeof(szOutMultiStr))
//first use
>           {
>             /* uninstaller not found, so copy the szPtrIn string to szPtrOut buffer */
>             lstrcpy(szPtrOut, szPtrIn);
>+            
>+            /* the following section is conditional because there are two possible entries in PendingFileRenameOperations 
>+             * szDstfile\0\0 is for deletions, szSrcFile\0szDstFile\0\0 is for file replacement (move)                    
>+             * since the string operations will stop at the null in the middle of the move command, it is done in 2 passes
>+             * only inserting one null between move arguments, and two nulls between the MoveFileEx operations
>+             */
>+            szInMultiStrIndex += lstrlen(szPtrIn);
//second use
these two codepaths are very similar:
>+            if ( (szInMultiStr[szInMultiStrIndex] == '\0') && (szInMultiStr[szInMultiStrIndex + 1] != '\0' ))
>+            {
>+              szInMultiStrIndex += 1;                              /* there is only 1 NULL byte between parameters in a   */
>+              dwOutMultiStrLen += lstrlen(szPtrIn) + 1;            /* move command                                        */
//third use
>+              szPtrOut          = &szPtrOut[lstrlen(szPtrIn) + 1]; /*                                                     */
//fourth use
>+              bPendingFileRename = TRUE;                           /* working with a move operation, rather than delete   */
>+            }
>+            else
>+            {
>+              szInMultiStrIndex += 2;
>+              dwOutMultiStrLen += lstrlen(szPtrIn) + 2;            /* there are actually 2 NULL bytes between the strings */
//alt third use
>+              szPtrOut          = &szPtrOut[lstrlen(szPtrIn) + 2]; /* there are actually 2 NULL bytes between the strings */
//alt fourth use
>+              bPendingFileRename = FALSE;                          /* working with a delete operation, rather than move   */
>+            }
>           }
>           else
>           {
>             bFoundUninstaller = FALSE;
>             /* not enough memory; break out of while loop. */
>             break;
>           }
>         }
>         else
>+        {
>+          szInMultiStrIndex += lstrlen(szPtrIn) + 2;             /* increment to next string in the REG_MULTI_SZ          */
>+          bPendingFileRename = FALSE;                            /* you are not in the middle of a 2-pass move command    */
>           bFoundUninstaller = TRUE;
>+        }
>+        
>+        if(bPendingFileRename)
>+        {
>+          szPtrIn = &szPtrIn[lstrlen(szPtrIn) + 1];              /* there is only 1 NULL byte between strings in a move */
//i'm not sure what use number this is, perhaps fifth
>+        }
>+        else
>+        {
>+          szPtrIn = &szPtrIn[lstrlen(szPtrIn) + 2];              /* there are actually 2 NULL bytes between the strings */
//i'm not sure what use number this is, perhaps alt fifth
>+        }
>       }while(*szPtrIn != '\0');
>     }

you /could/ do this, but i'm not sure it's actually better...
bPendingFileMove = (szInMultiStr[szInMultiStrIndex] != '\0') ||
(szInMultiStr[szInMultiStrIndex + 1] == '\0');
szInMultiStrIndex += 1 + bPendingFileMove; 
dwOutMultiStrLen += lstrlen(szPtrIn) + 1 + bPendingFileMove; 
szPtrOut = &szPtrOut[lstrlen(szPtrIn) + 1 + bPendingFileMove];

Comment 17

[David G King]

Comment on attachment 143535 [details] [diff] [review]
Cleaned up version of patch (no ^M or tabs)

Changing SR based on offlist discussion with roc. However, he will not SR until
a R has been done.

Comment 18

[Daniel Veditz [:dveditz]]

I am looking into the r=

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 143535 [details] [diff] [review]
Cleaned up version of patch (no ^M or tabs)

taking this off my superreview queue until a reviewer is found.

Comment 20

[Daniel Veditz [:dveditz]]

Attachment: Alternate fix

Comment 21

[Daniel Veditz [:dveditz]]

Comment on attachment 148094 [details] [diff] [review]
Alternate fix

r=ssu via mail

Comment 22

[Doug Turner (:dougt)]

Comment on attachment 148094 [details] [diff] [review]
Alternate fix

Why do you have to allocate here:

pathToMatch = strdup(aPathToMatch);  

More comments in the WHILE loop would be nice.

Why do you need to flush (RegFlushKey(hkResult);) at this point?  Can't you
wait until windows is ready to do that?

You can reuse oldMaxValueLen and get rid of newMaxValueLen.

I haven't convinced myself that your memmove is correct.  It would be nice to
know what data was stored in the regkey.  I guess I am lost on the variable
names: np, op.	I thought that op is short for operation.  If that is the case,
I think this might be wrong:

if (StrStrI(op, pathToMatch))  Isn't the path in np?  Oh well, just need to be
clear on what you are parsing.

Comment 23

[Sean Su]

> Why do you have to allocate here:
> pathToMatch = strdup(aPathToMatch);  

You are correct, it is not needed anymore.  I believe that it was left over from
when it did a string conversion to lowercase for parsing reasons, where we
didn't want to alter the original string.  StrStrI() solves the problem.

Comment 24

[Daniel Veditz [:dveditz]]

Attachment: address review comments

Got rid of leftover pathToMatch strdup, better variable names, only write back
to the registry if we've actually changed the string.

Comment 25

[Daniel Veditz [:dveditz]]

Comment on attachment 148178 [details] [diff] [review]
address review comments

Carrying over r=ssu

Comment 26

[Daniel Veditz [:dveditz]]

Attachment: same, with added comments per sr feedback

Comment 27

[chris hofmann]

Comment on attachment 148180 [details] [diff] [review]
same, with added comments per sr feedback

a=chofmann for 1.7

Comment 28

[Daniel Veditz [:dveditz]]

Fix checked in to trunk and 1.7 branch