FREF(9) in unp_internalize()

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

FREF(9) in unp_internalize()

Martin Pieuchot
Diff below does FREF(9) earlier instead of incrementing `f_count' by hand.

The error path is also updated to call FRELE(9) accordingly.

ok?

Index: kern/uipc_usrreq.c
===================================================================
RCS file: /cvs/src/sys/kern/uipc_usrreq.c,v
retrieving revision 1.123
diff -u -p -r1.123 uipc_usrreq.c
--- kern/uipc_usrreq.c 4 Jan 2018 10:45:30 -0000 1.123
+++ kern/uipc_usrreq.c 16 Apr 2018 08:13:52 -0000
@@ -838,23 +838,27 @@ morespace:
  error = EBADF;
  goto fail;
  }
+ FREF(fp);
  if (fp->f_count == LONG_MAX-2) {
+ FRELE(fp, p);
  error = EDEADLK;
  goto fail;
  }
  error = pledge_sendfd(p, fp);
- if (error)
+ if (error) {
+ FRELE(fp, p);
  goto fail;
-    
+ }
+
  /* kqueue descriptors cannot be copied */
  if (fp->f_type == DTYPE_KQUEUE) {
+ FRELE(fp, p);
  error = EINVAL;
  goto fail;
  }
  rp->fp = fp;
  rp->flags = fdp->fd_ofileflags[fd] & UF_PLEDGED;
  rp--;
- fp->f_count++;
  if ((unp = fptounp(fp)) != NULL) {
  unp->unp_file = fp;
  unp->unp_msgcount++;
@@ -867,7 +871,7 @@ fail:
  for ( ; i > 0; i--) {
  rp++;
  fp = rp->fp;
- fp->f_count--;
+ FRELE(fp, p);
  if ((unp = fptounp(fp)) != NULL)
  unp->unp_msgcount--;
  unp_rights--;

Reply | Threaded
Open this post in threaded view
|

Re: FREF(9) in unp_internalize()

Todd C. Miller-2
On Mon, 16 Apr 2018 10:19:40 +0200, Martin Pieuchot wrote:

> Diff below does FREF(9) earlier instead of incrementing `f_count' by hand.
>
> The error path is also updated to call FRELE(9) accordingly.

Wouldn't it be less error prone to simply add:

        if (fp != NULL)
                FRELE(fp, p);

to the fail label?  If we get to fail, fp is either NULL or needs to
drop a reference.

 - todd

Reply | Threaded
Open this post in threaded view
|

Re: FREF(9) in unp_internalize()

Martin Pieuchot
On 16/04/18(Mon) 09:09, Todd C. Miller wrote:

> On Mon, 16 Apr 2018 10:19:40 +0200, Martin Pieuchot wrote:
>
> > Diff below does FREF(9) earlier instead of incrementing `f_count' by hand.
> >
> > The error path is also updated to call FRELE(9) accordingly.
>
> Wouldn't it be less error prone to simply add:
>
> if (fp != NULL)
> FRELE(fp, p);
>
> to the fail label?  If we get to fail, fp is either NULL or needs to
> drop a reference.

Sure, here's an updated diff.  It also moves the FRELE(9) in the error
loop down as suggested by visa@.

Index: kern/uipc_usrreq.c
===================================================================
RCS file: /cvs/src/sys/kern/uipc_usrreq.c,v
retrieving revision 1.123
diff -u -p -r1.123 uipc_usrreq.c
--- kern/uipc_usrreq.c 4 Jan 2018 10:45:30 -0000 1.123
+++ kern/uipc_usrreq.c 17 Apr 2018 07:48:09 -0000
@@ -838,6 +838,7 @@ morespace:
  error = EBADF;
  goto fail;
  }
+ FREF(fp);
  if (fp->f_count == LONG_MAX-2) {
  error = EDEADLK;
  goto fail;
@@ -845,7 +846,7 @@ morespace:
  error = pledge_sendfd(p, fp);
  if (error)
  goto fail;
-    
+
  /* kqueue descriptors cannot be copied */
  if (fp->f_type == DTYPE_KQUEUE) {
  error = EINVAL;
@@ -854,7 +855,6 @@ morespace:
  rp->fp = fp;
  rp->flags = fdp->fd_ofileflags[fd] & UF_PLEDGED;
  rp--;
- fp->f_count++;
  if ((unp = fptounp(fp)) != NULL) {
  unp->unp_file = fp;
  unp->unp_msgcount++;
@@ -863,13 +863,15 @@ morespace:
  }
  return (0);
 fail:
+ if (fp != NULL)
+ FRELE(fp, p);
  /* Back out what we just did. */
  for ( ; i > 0; i--) {
  rp++;
  fp = rp->fp;
- fp->f_count--;
  if ((unp = fptounp(fp)) != NULL)
  unp->unp_msgcount--;
+ FRELE(fp, p);
  unp_rights--;
  }
 

Reply | Threaded
Open this post in threaded view
|

Re: FREF(9) in unp_internalize()

Todd C. Miller-2
On Tue, 17 Apr 2018 09:50:49 +0200, Martin Pieuchot wrote:

> Sure, here's an updated diff.  It also moves the FRELE(9) in the error
> loop down as suggested by visa@.

OK millert@

 - todd