Closed Bug 145054 Opened 22 years ago Closed 22 years ago

Plugins may starve when cached data they read from is removed too soon

Categories

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

Product:
Core Graveyard
Component:
Plug-ins
Platform:
x86
All
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla1.2alpha

People

(Reporter: billyboy, Assigned: srgchrpv)

References

(
URL
)

Details

(Whiteboard: [PL2:NA][plugger])

Attachments

(2 files, 3 obsolete files)

patch v4
18.73 KB, patch
Details | Diff | Splinter Review
patch v5
28.23 KB, patch
serhunt
: review+
darin.moz
: superreview+
Details | Diff | Splinter Review 
I'm viewing a page which has this simple html code: <embed src="home.mid"
width=200 height=55 autostart=true>.  Plugger calls timidity with $file ==
"/tmp/plugtmp/home.mid" properly but plugger hangs.  Strace'ing mozilla reveals
that it:

1- mkdir("/tmp/plugtmp");
2- Writes the content of the midi file into /tmp/plugtmp/home.mid
3- forks the plugger
4- stat, lstat and unlink /tmp/plugtmp/home.mid immediately.

That leaves timidity in a funk.  See URL for exmaple.

This is with Mozilla 1.0 RC2
Which version of plugger are you testing with?
Related: bug 74080
I'm using plugger-4.0, here's my .mid section of /etc/plugger-4.0:

audio/mid: midi,mid: MIDI audio file 
audio/x-mid: midi,mid: MIDI audio file
audio/midi: midi,mid: MIDI audio file
audio/x-midi: midi,mid: MIDI audio file
  : timidity -s 48000 -a -idqqqqqqq "$file" >/dev/null 2>/dev/null
  : playmidi $file

Here's the section of the trace where you can see writes to FDs 38
(/tmp/plugtmp/home/.mid, 39 (cache), the plugger fork and the immediate unlink:

6621  gettimeofday({1021563233, 665779}, NULL) = 0
6621  write(38, "NW\201p\230QW\201p\230VW\201p\230UW\201p\210U@\0\210V@"..., 131
6621  write(39, "MThd\0\0\0\6\0\1\0\17\1\340MTrk\0\0\1{\0\377\1\24\221\305"..., 
6621  close(38)                         = 0
6621  gettimeofday({1021563233, 669310}, NULL) = 0
6621  socketpair(PF_UNIX, SOCK_STREAM, 0, [38, 40]) = 0
6621  rt_sigprocmask(SIG_SETMASK, ~[], [RT_0], 8) = 0
6621  fork()                            = 6634
6621  kill(6627, SIGRT_0)               = 0
6621  rt_sigprocmask(SIG_SETMASK, [RT_0], ~[KILL STOP], 8) = 0
6621  close(40)                         = 0
6621  stat("/tmp/plugtmp/home.mid", {st_mode=S_IFREG|0755, st_size=29550, ...})
6621  lstat("/tmp/plugtmp/home.mid", {st_mode=S_IFREG|0755, st_size=29550, ...})
6621  unlink("/tmp/plugtmp/home.mid")   = 0
6621  gettimeofday({1021563233, 675254}, NULL) = 0

A bit later, plugger is called thusly:

6634  execve("/usr/bin/plugger-4.0", ["plugger-4.0",
"0,2147483647,40,14680065,8,8,200"..., "/tmp/plugtmp/home.mid", "", ":0.0",
"timidity -s 48000 -a -idqqqqqqq "..., "audio/midi"], [/* 55 vars */]) = 0

And then timidity tries to:

6636  open("/tmp/plugtmp/home.mid", O_RDONLY) = -1 ENOENT (No such file or
directory)
Whiteboard: [plugger]
assigning to serge
Assignee: beppe → serge
Priority: -- → P3
Target Milestone: --- → mozilla1.2beta
yes, it's true only for embed tag, and this is not linux only,
changing OS, taking this bug.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Linux → All
Whiteboard: [plugger] → [PL2:NA][plugger]
Target Milestone: mozilla1.2beta → mozilla1.0.2
Attached patch patch v1 (obsolete) — — Splinter Review 
this patch also implemented simple logic to reused files already copied in
local plugin cache. Please review.
The patch seems to solve this problem, which was also affecting embedded audio
files.
Comment on attachment 93770 [details] [diff] [review]
patch v1

Couple of questions. 
- is my understanding correct that we are not going to delete this cache file
at all under certain circumstances?
- can we not use globals? How difficult is to add something like
|nsPIPluginHost::GetActivePlugins|?

Other than that r=av.
Attachment #93770 - Flags: review+
>we are not going to delete this cache file at all under certain circumstances
could you be more specific, please?
all files should be deleted when last instance of plugin which uses those files
are gone.

>can we not use globals?
why not?

>How difficult is to add something like |nsPIPluginHost::GetActivePlugins|?
difficulties are the same as for any new method to the interface.
but my question is what benefits it will give us?
I doubt we are going to use this method outside gkplugin.dll,.
so, to me we'll gain nothing but extra code and additional QI call.
Target Milestone: mozilla1.0.2 → mozilla1.2alpha
>I doubt we are going to use this method outside gkplugin.dll,.
>so, to me we'll gain nothing but extra code and additional QI call.

Then it does not make sense to have it a member of |nsPluginHostImpl|. The code
will be even simpler. Should we consider this option?
>Should we consider this option?
as clean up effort - yes, but not for this bug i guess.
*** Bug 161224 has been marked as a duplicate of this bug. ***
adding nsbeta1+
Keywords: nsbeta1+
The bug seems to be important. Please consider my r=av valid and seek for sr.
Maybe this bug should get higher promotion.
Summary: plugger cannot read file because browser removes it too soon. → Plugins may starve when cached data they read from is removed too soon
Comment on attachment 93770 [details] [diff] [review]
patch v1

>Index: nsPluginHostImpl.cpp

how about adding a description of what this bit of code is trying to do?

>+    nsActivePlugin *pActivePlugins = gActivePluginList->mFirst;
>+    do {
>+      // most recent streams are at the end of list 
>+      int cnt = pActivePlugins->mStreams->Count();
>+      for (int i = cnt; i; i-- ) {
>+        nsPluginStreamListenerPeer * listenerPeer = 
>+          NS_REINTERPRET_CAST(nsPluginStreamListenerPeer *, pActivePlugins->mStreams->ElementAt(i-1));
>+        if (listenerPeer &&
>+          listenerPeer->mPluginStreamInfo->GetExistingPluginCacheFile(mPluginStreamInfo, getter_AddRefs(pluginTmp))) {
>+          pActivePlugins = gActivePluginList->mLast; // break do{}while() loop
>+          break;
>+        }
>+      }
>+    } while (pActivePlugins = pActivePlugins->mNext);
>+
>+    if (pluginTmp) {
>+      mPluginStreamInfo->SetLocalCachedFile(pluginTmp);
>+    } else {

hmm... where did these URL strings come from?  how do you know that
case sensitive compare is ok?  seems like you really should be using
nsIURI::equals() to test this.

>+      !PL_strcmp(psi->mURL, mURL))
thanks darin.
>how about adding a description of what this bit of code is trying to do?
will do.

>hmm... where did these URL strings come from?
it's declare as char* in class nsPluginStreamInfo,
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/modules/plugin/base/src/nsPluginHostImpl.cpp&rev=1.408&root=/cvsroot#1275
why so, it's a different story.
cannot do nsIURI::equals() on C string.


keep in mind that if you're comparing two URLs and not using nsIURI::equals,
then you have a bug!  probably fixing this bug requires significantly reworking
the plugins to store nsIURI instead of |char*| for URLs.
yeap, there is a plan to rewrite plugins code, and we'll change char* url to
nsIURI  than, but for now it's char*.
Comment on attachment 93770 [details] [diff] [review]
patch v1

sr=darin
Attachment #93770 - Flags: superreview+
Attached patch patch v2 (obsolete) — — Splinter Review 
I apologize, the previous patch has wrong logic for removing locally cached
file:(
so, here's the new one.
I've added new class |nsPluginLocalCachedFile : public nsISupports|
which is responsible for removing the file in dtor.
Please review.
Attachment #93770 - Attachment is obsolete: true
Comment on attachment 95202 [details] [diff] [review]
patch v2

You operate comptrs, why in this new nsPluginLocalCachedFile class we still
pass raw pointers? I think something like this would be appropriate:

+  nsresult
+  SetFile(nsCOMPtr<nsIFile> aFile);
+
+  nsresult
+  GetFile(nsCOMPtr<nsIFile> &aFile);

|nsPluginStreamInfo::SetLocalCacheFile| can be adjusted too.
hmm, have you read?
http://mozilla.org/projects/xpcom/nsCOMPtr.html#guide_nsCOMPtr_in_APIs
e.g.
void f( nsCOMPtr<T> ) don't pass an nsCOMPtr by value
Yep. & should be there obviously.
Comment on attachment 95202 [details] [diff] [review]
patch v2

r=av
Attachment #95202 - Flags: review+
Darin, would you please sr= on attachment 95202 [details] [diff] [review]? thanks. 
*** Bug 163590 has been marked as a duplicate of this bug. ***
Just came to my mind. Not that I don't like the patch, but maybe this will make
it simpler and smaller on footprint.

As far as I understand the only reason to have |nsPluginLocalCachedFile| wrapper
around |nsIFile| is that we want to delete the actual file when it is not longer
needed, i.e. when the corresponding object's refcount goes to zero. Looks like
we have a special macro NS_RELEASE2 which can be used to detect this situation.
Do you think it would make sense to make use of it?
well, good point, let me see...
probably using one extra refcnt for |mCachedFile| and do something like this in
nsPluginStreamInfo::~nsPluginStreamInfo()
{
...
  NS_RELEASE2(mCachedFile, refcnt);
  if (refcnt == 1) {
    mCachedFile->Remove(PR_FALSE);
    NS_RELEASE(mCachedFile);
  }
we can do the job.
Attached patch patch v3 (obsolete) — — Splinter Review 
with NS_RELEASE2(mLocalCachedFile,rc)
and nsActivePlugin.mStreams converted to nsISupportsArray instead of
nsVoidArray
Attachment #95202 - Attachment is obsolete: true
Comment on attachment 96129 [details] [diff] [review]
patch v3

r=av, maybe to add some more explanation to a comment why we do this extra
addref.
Attachment #96129 - Flags: review+
#define NS_RELEASE2(_ptr,_rv)                                                 \
  PR_BEGIN_MACRO                                                              \
    _rv = NS_LOG_RELEASE_CALL((_ptr), (_ptr)->Release(),__FILE__,__LINE__);   \
    if (0 == (_rv)) (_ptr) = 0;                                               \
  PR_END_MACRO

if _ptr == 0, I cannot do _ptr->Foo()
I am just suggesting to add more comments on this in the patch.
+  // add one extra refcnt, we can do NS_RELEASE2(mLocalCachedFile...) in dtor
is this not enough?
do I have to explain that _ptr->Foo() is bad if _ptr==0?
How about something like this:

// we need this extra refcount in order to be able to remove the file physically
in // the destructor, when it is released by everybody else 
av: I'll add come comments, thanks.
darin: would you please sr= on patch v3 (attachment 96129 [details] [diff] [review])? thanks.
Comment on attachment 96129 [details] [diff] [review]
patch v3

>Index: nsPluginHostImpl.h

>   PRBool                 mStopped;
>   PRTime                 mllStopTime;
>   PRBool                 mDefaultPlugin;
>+  PRBool                 mXPConnected;

it might be worthwhile to convert these PRBool variables
over to PRPackedBool, and list them adjacent to one another.
that might reduce the size of the data structure a bit, fwiw.


>Index: nsPluginHostImpl.cpp

>+  nsresult rv = file->Clone(&mLocalCachedFile);
>+  // add one extra refcnt, we can do NS_RELEASE2(mLocalCachedFile...) in dtor
>+  NS_IF_ADDREF(mLocalCachedFile);

so, just to make sure i understand what's going on here... setting
mLocalCachedFile results in 2 references.  the destructor will call
release twice if there are only 2 references to the file, else it
will only call release once.  mLocalCachedFile could gain an additional
reference if UseExistingPluginCacheFile is called, but that's ok since
now you'd have two nsPluginStreamInfo objects owning 3 references to
mLocalCachedFile.  one of the psi's would end up calling release twice.
is that correct?

so, what happens if someone calls GetLocalCachedFile?  aren't we in
trouble if they don't do the same NS_RELEASE2 fu?  can we possibly
eliminate that method?	does anyone call it?
>it might be worthwhile to convert these PRBool variables over to PRPackedBool
I agree, but I did not introduce that var in this patch,
and we'll change it in revise plugins code.

>one of the psi's would end up calling release twice. is that correct?
yes, this is correct,
the final release of |mLocalCachedFile| will be call in
|nsPluginStreamInfo::~nsPluginStreamInfo|
...
+  if (mLocalCachedFile) {
+    nsrefcnt refcnt;
+    NS_RELEASE2(mLocalCachedFile, refcnt);
+    if (refcnt == 1) {
+      mLocalCachedFile->Remove(PR_FALSE);
+      NS_RELEASE(mLocalCachedFile);
+    }

only if (refcnt == 1), that is why I've added one extra refcnt 
in |nsPluginStreamInfo::SetLocalCachedFile|

>so, what happens if someone calls GetLocalCachedFile?
|GetLocalCachedFile| is *non* interface simple getter method
 void
 nsPluginStreamInfo::GetLocalCachedFile(nsIFile** file)
 { 
-  *file = mCachedFile;
+  *file = mLocalCachedFile;
   NS_IF_ADDREF(*file);
 }
>aren't we in trouble if they don't do the same NS_RELEASE2 fu?
the caller has to call NS_RELEASE or NS_RELEASE2 w/o any problem.

>can we possibly eliminate that method?
why? what's risk to have this method in.

>does anyone call it?
we do call in once from |nsPluginStreamListenerPeer::OnStopRequest|
to figure out which file to use in |OnFileAvailable|
so long as the reference, added when GetLocalCacheFile is called, is released
before the destructor of any psi is invoked, you are ok.  but if someone calls
GetLocalCacheFile and holds onto that reference for any "long" period of time,
you could end up leaking the mLocalCacheFile.  that's my fear anyways.  of
course, the code as is doesn't leak, but someone down the road might call
GetLocalCacheFile not knowing about this funky reference counting arrangement. 
can you do anything to prevent that kind of future problem?
Attached patch patch v4 — — Splinter Review 
changed method signature
from
void nsPluginStreamInfo::GetLocalCachedFile(nsIFile** file);
to
nsresult nsPluginStreamInfo::GetLocalCachedFile(nsCOMPtr<nsIFile>& file);
of course it does not 100% bulletproof,  but if someone  will hold onto any 
reference more that it's needed we can leak any objects:(
Attachment #96129 - Attachment is obsolete: true
not to be a pest, but i don't see how that change really helps... it seems like
a large comment in the class definition would be sufficient :-/
Darin, I think to leak |mLocalCachedFile| using
|nsPluginStreamInfo::GetLocalCachedFile(nsCOMPtr<nsIFile>& file)|
is not a trivial task,  |nsCOMPtr<nsIFile> file|  will be released automatically
when it goes out of scope.
I cannot assume the scenario when it'll be possible to leak that.
{
  nsCOMPtr<nsIFile> file;
  {
    nsCOMPtr<nsIPluginStreamInfo> psi = // create psi
    psi->GetLocalCachedFile(file);
  }
}

this admittedly simplified code leaks |file|.  my point is that the leak can
still happen easily.  IMO, the v3 patch was fine... it just needed a comment
explaining that callers should keep in mind the fact that should not hold a
reference to the returned file for very long.
{
  nsCOMPtr<nsIFile> file;
  {
    nsCOMPtr<nsIPluginStreamInfo> psi = // create psi
    psi->GetLocalCachedFile(file);
  }
} <== |file| will be release out of this scope, how can we leak it?

well, I can try to move |mLocalCachedFile| from |nsPluginStreamInfo| into
|nsPluginStreamListenerPeer| and eliminate those
|nsPluginStreamInfo::S/GetLocalCachedFile| methods, if you want me to do so.
like i said, i think it would be enough to add a BIG comment above the
declaration of GetLocalCachedFile.  that should be plenty to help out folks down
the road, don't you think?
I agree, but now I'm really thinking to move |mLocalCachedFile| from
|nsPluginStreamInfo| into |nsPluginStreamListenerPeer|.
I see no reason to keep that member belong |nsPluginStreamInfo| if we access it
only from |nsPluginStreamListenerPeer|.
av, peterl what do you think?
Makes sense. What was the reason to have it in |nsPluginStreamInfo| in the first
place?
that was introduced by fix for bug 87397
Attached patch patch v5 — — Splinter Review 
with |mLocalCachedFile| moved from |nsPluginStreamInfo| into
|nsPluginStreamListenerPeer|
i think it's more clear code.
Comment on attachment 96929 [details] [diff] [review]
patch v5

This is nice clean up. Maybe it would make sense and use this opportunity to
get rid |nsPluginHostImpl::mActivePlugins| list? We can just make it global
rather than create a global reference to it.
of course we can, but I would not do that in this patch, the patch is big enough
already, and changing |nsPluginHostImpl::mActivePluginList| could bring the risk
which is not related to this bug at all.
Comment on attachment 96929 [details] [diff] [review]
patch v5

r=av
Attachment #96929 - Flags: review+
Comment on attachment 96929 [details] [diff] [review]
patch v5

sr=darin
Attachment #96929 - Flags: superreview+
on the trunk
nsPluginHostImpl.cpp;
new revision: 1.421; previous revision: 1.420
nsPluginHostImpl.h;
new revision: 1.86; previous revision: 1.85
...
Thanks all
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: