diff mbox

[FFmpeg-devel,v1,2/2] libavformat/file: initilize the fd to -1 instead of 0(valid fd) in case unexpected file close

Message ID 20190618104513.19549-2-lance.lmwang@gmail.com
State Superseded
Headers show

Commit Message

Lance Wang June 18, 2019, 10:45 a.m. UTC 
From: Limin Wang <lance.lmwang@gmail.com>

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavformat/file.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Lance Wang July 10, 2019, 2:48 p.m. UTC | #1 
ping?  I have developed code to use avio_open_dir function, after using avio_close_dir
to release the resource, my ffmepg command will lost interact for the fd
0 is closed by avio_close_dir.
 

On Tue, Jun 18, 2019 at 06:45:13PM +0800, lance.lmwang@gmail.com wrote:
> From: Limin Wang <lance.lmwang@gmail.com>
> 
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>  libavformat/file.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/libavformat/file.c b/libavformat/file.c
> index 08c7f8e6dd..40ae9ad2a8 100644
> --- a/libavformat/file.c
> +++ b/libavformat/file.c
> @@ -274,6 +274,7 @@ static int file_open_dir(URLContext *h)
>  #if HAVE_LSTAT
>      FileContext *c = h->priv_data;
>  
> +    c->fd = -1;
>      c->dir = opendir(h->filename);
>      if (!c->dir)
>          return AVERROR(errno);
> -- 
> 2.21.0
>
Jun Zhao July 11, 2019, 3:16 a.m. UTC | #2 
On Wed, Jul 10, 2019 at 11:11 PM Limin Wang <lance.lmwang@gmail.com> wrote:
>
>
> ping?  I have developed code to use avio_open_dir function, after using avio_close_dir
> to release the resource, my ffmepg command will lost interact for the fd
> 0 is closed by avio_close_dir.
>
>
> On Tue, Jun 18, 2019 at 06:45:13PM +0800, lance.lmwang@gmail.com wrote:
> > From: Limin Wang <lance.lmwang@gmail.com>
> >
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> >  libavformat/file.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/libavformat/file.c b/libavformat/file.c
> > index 08c7f8e6dd.. 40ae9ad2a8 100644
> > --- a/libavformat/file.c
> > +++ b/libavformat/file.c
> > @@ -274,6 +274,7 @@ static int file_open_dir(URLContext *h)
> >  #if HAVE_LSTAT
> >      FileContext *c = h->priv_data;
> >
> > +    c->fd = -1;
I don't konw why need to change the fd , suppose we have a calling
sequence like file_open ahead of file_open_dir, can we leak a fd
resource ?
> >      c->dir = opendir(h->filename);
> >      if (!c->dir)
> >          return AVERROR(errno);
> > --
> > 2.21.0
Michael Niedermayer July 12, 2019, 8:44 p.m. UTC | #3 
On Tue, Jun 18, 2019 at 06:45:13PM +0800, lance.lmwang@gmail.com wrote:
> From: Limin Wang <lance.lmwang@gmail.com>
> 
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>  libavformat/file.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/libavformat/file.c b/libavformat/file.c
> index 08c7f8e6dd..40ae9ad2a8 100644
> --- a/libavformat/file.c
> +++ b/libavformat/file.c
> @@ -274,6 +274,7 @@ static int file_open_dir(URLContext *h)
>  #if HAVE_LSTAT
>      FileContext *c = h->priv_data;
>  
> +    c->fd = -1;
>      c->dir = opendir(h->filename);
>      if (!c->dir)
>          return AVERROR(errno);

Is it more robust to set fd=-1 after successfully opening the directory ?

considering that this could be already opened as a file ...
(such combinations seem to be what the patch is about IIUC)
and that on failure we could prefer to leave the state close to the
original before the call

thanks

[...[
Lance Wang July 12, 2019, 10:36 p.m. UTC | #4 
On Fri, Jul 12, 2019 at 10:44:48PM +0200, Michael Niedermayer wrote:
> On Tue, Jun 18, 2019 at 06:45:13PM +0800, lance.lmwang@gmail.com wrote:
> > From: Limin Wang <lance.lmwang@gmail.com>
> > 
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> >  libavformat/file.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/libavformat/file.c b/libavformat/file.c
> > index 08c7f8e6dd..40ae9ad2a8 100644
> > --- a/libavformat/file.c
> > +++ b/libavformat/file.c
> > @@ -274,6 +274,7 @@ static int file_open_dir(URLContext *h)
> >  #if HAVE_LSTAT
> >      FileContext *c = h->priv_data;
> >  
> > +    c->fd = -1;
> >      c->dir = opendir(h->filename);
> >      if (!c->dir)
> >          return AVERROR(errno);
> 
> Is it more robust to set fd=-1 after successfully opening the directory ?
> 
> considering that this could be already opened as a file ...
> (such combinations seem to be what the patch is about IIUC)
> and that on failure we could prefer to leave the state close to the
> original before the call
I'm using open the directory operation without any file combiations, if
the context need to support both dir and file combined, then I need do more
testing for the condition.

> 
> thanks
> 
> [...[
> -- 
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
> I have never wished to cater to the crowd; for what I know they do not
> approve, and what they approve I do not know. -- Epicurus



> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Michael Niedermayer July 14, 2019, 12:05 p.m. UTC | #5 
On Sat, Jul 13, 2019 at 06:36:28AM +0800, Limin Wang wrote:
> On Fri, Jul 12, 2019 at 10:44:48PM +0200, Michael Niedermayer wrote:
> > On Tue, Jun 18, 2019 at 06:45:13PM +0800, lance.lmwang@gmail.com wrote:
> > > From: Limin Wang <lance.lmwang@gmail.com>
> > > 
> > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > > ---
> > >  libavformat/file.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/libavformat/file.c b/libavformat/file.c
> > > index 08c7f8e6dd..40ae9ad2a8 100644
> > > --- a/libavformat/file.c
> > > +++ b/libavformat/file.c
> > > @@ -274,6 +274,7 @@ static int file_open_dir(URLContext *h)
> > >  #if HAVE_LSTAT
> > >      FileContext *c = h->priv_data;
> > >  
> > > +    c->fd = -1;
> > >      c->dir = opendir(h->filename);
> > >      if (!c->dir)
> > >          return AVERROR(errno);
> > 
> > Is it more robust to set fd=-1 after successfully opening the directory ?
> > 
> > considering that this could be already opened as a file ...
> > (such combinations seem to be what the patch is about IIUC)
> > and that on failure we could prefer to leave the state close to the
> > original before the call
> I'm using open the directory operation without any file combiations, if
> the context need to support both dir and file combined, then I need do more
> testing for the condition.

It doesnt need no, but people do mix it as it seems. So the behvior
in that case should be "usefull" like erroring our or failing an assert
while in this case i think it would just leak the open file and continue
that may be harder to debug for the user

thx
[...]
diff mbox

Patch

diff --git a/libavformat/file.c b/libavformat/file.c
index 08c7f8e6dd..40ae9ad2a8 100644
--- a/libavformat/file.c
+++ b/libavformat/file.c
@@ -274,6 +274,7 @@  static int file_open_dir(URLContext *h)
 #if HAVE_LSTAT
     FileContext *c = h->priv_data;
 
+    c->fd = -1;
     c->dir = opendir(h->filename);
     if (!c->dir)
         return AVERROR(errno);