57210 - If you have npjava32.dll in the Netscape 4.7x plugin dir, Mozilla crashes on startup

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P1)

Description

[Ken Yee]

As above.  The workaround is to rename npjava32.dll to something else before
starting Mozilla.  I can send my copy of npjava32.dll or my entire plugin
directory from Netscape if needed to reproduce this bug.
Netscape seems to be able to deal with renaming npjava32.dll so this is a
reasonable workaround.

Comment 1

[av (gone)]

This is regression introduced by fixing another bug (Mac "plugins"/"plug-ins" 
thing.

Comment 2

[av (gone)]

The fix is easy and perfectly safe. Just adding a check for npjava* filename.

Comment 3

[av (gone)]

Attachment: Patch fixing the problem

Comment 4

[av (gone)]

Adding ekrock.

Comment 5

[av (gone)]

Correction. Regression was introduced when we decided not to load anything from 
the old installation except 'big three' and Java plugin. Check for npjava*.* was 
accidentally dropped in version 1.121 of nsPluginHostImpl.cpp.

Comment 6

[edburns]

Looks good.  r=edburns.

Comment 7

[Phil Peterson]

Marking [rtm need info] since I don't see a super review in here. Please mark
[rtm+] after getting one.

Comment 8

[av (gone)]

Hm... In fact, even without this patch no java dll should be picked from 4.x 
plugins.

Shrirang, can you reproduce the problem? I seem to loose all my old java 
plugins. Could be just invalid with current version of Mozilla.

Comment 9

[shrirang khanzode]

Andrei, as you said, I lost my old java plugin and see some new files 
there(npjava130_01c.dll...). there is no npjava32.dll at all and this problem 
does not seem to happen. If I am not wrong, we stopped sniffing for java plugins 
at all on windows after it started crashing in beta1....

Comment 10

[av (gone)]

Reporter: if you do use one of the recent Mozilla spins, would you please send 
the dll in question to me, Shrirang or just attach it to the bug report?
Thanks.

Comment 11

[shrirang khanzode]

Attachment: dll

Comment 12

[av (gone)]

Oh, this is IBM made plugin. I see the crash in PLC4.dll. Details to follow.

Comment 13

[av (gone)]

Please disregard all my previous comments. The problem is not with the java 
plugin itself. Any plugin can display this, and I beleive we saw this before. 
When we look through 4.x Plugins dir on start up we read all the plugins info in 
order to decide whether to ignore a specific plugin or to use it. It crashes on 
this stage, and it happens when the number of mime types listed in the plugin 
version stamp does not correspont to the number of file extensions. As I said we 
saw this with other plugins and our code is still not safe enough to deal with 
this.

I'll try to come up with the patch soon.

Comment 14

[av (gone)]

OK, here is one-liner which fixes the crash. So, now when the data contains less 
segments than variants it will just break off the loop.

static char** MakeStringArray(PRUint32 variants, char* data)
{
  if((variants <= 0) || (data == NULL))
    return NULL;

  char ** array = (char **)PR_Calloc(variants, sizeof(char *));
  if(array == NULL)
    return NULL;

  char * start = data;

  for(PRUint32 i = 0; variants; i++)
  {
    char * p = PL_strchr(start, '|');
    if(p != NULL)
      *p = 0;

    array[i] = PL_strdup(start);

+    if(p == NULL) // nothing more to look for
+      break;

    start = ++p;
  }
  return array;
}

Comment 15

[av (gone)]

This version could be safer, since there is no garantee that somewhere in the 
code it will not try to access array elemets without checking for null.

static char** MakeStringArray(PRUint32 variants, char* data)
{
  if((variants <= 0) || (data == NULL))
    return NULL;

  char ** array = (char **)PR_Calloc(variants, sizeof(char *));
  if(array == NULL)
    return NULL;

  char * start = data;

  for(PRUint32 i = 0; variants; i++)
  {
    char * p = PL_strchr(start, '|');
    if(p != NULL)
      *p = 0;

    array[i] = PL_strdup(start);

    if(p == NULL) // nothing more to look for
    {             // fill everything else with empty strings (safer?)
      for(; variants; i++)
        array[i] = PL_strdup("");
      break;
    }

    start = ++p;
  }
  return array;
}

Comment 16

[av (gone)]

Attachment: 'Officially' looking patch, no typos here

Comment 17

[edburns]

As long as the memory gets freed, ok with me.  r=edburns.

Comment 18

[Chris Waterson]

This looks good. With respect to freeing the memory, I believe that this'll
happen in nsPluginFile::FreePluginInfo(). I only have two nits to pick:

1. Put a comment in that says something to the effect of, "although we expect
the MIME type and extension arrays to be of the same length, some plugins don't
adhere to this, so we need to be paranoid."

2. Instead of writing

   ++i;
   for (; i < variants; ++i)
     /* blah */

why not write

   while (++i < variants)
     /* blah */

(In fact, you may even want to work up a cleaner version of the patch for the
trunk that loops conditioned on the results of strchr, and then a second loop
that pads out if you've got records left over.)

Anyway, make the two changes above, and sr=waterson

Comment 19

[av (gone)]

Attachment: Patch corrected for waterson's suggestions

Comment 20

[av (gone)]

Now, as I have sr=waterson, r=edburns, r=serge, I assume I can nominate it for 
rtm+. Clearing regression keyword.

Comment 21

[Phil Peterson]

rtm++

Comment 22

[av (gone)]

Checked in to the branch.

Comment 23

[av (gone)]

Trunk got it too.

Comment 24

[av (gone)]

Matking fixed.

Comment 25

[shrirang khanzode]

Verified that this is fixed on today's windows branch build. Adding keyword: 
vtrunk for trunk verification. Mozilla QA, to verify this bug, rename the 
attachment in this bug as "npjava32.dll' and save it in the plugins folder under 
4.x installation and launch seamonkey. There should be no crash and the browser 
should launch.

Comment 26

[Asa Dotzler [:asa]]

[e-mail], thanks for the clear steps to verify.  I saved and renamed
the attachment to my 4.7 plugins folder and then launched Mozilla Seamonkey
without a crash.
Verified Fixed on Mozilla trunk
win32 installer build 102604 NT 4
Setting bug to Verified and removing vtrunk keyword