Re: FREF(9) in unp_internalize()

[Todd C. Miller]

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

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 -   1.123
+++ kern/uipc_usrreq.c  17 Apr 2018 07:48:09 -
@@ -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--;
}
 

Re: FREF(9) in unp_internalize()

[Todd C. Miller]

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

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 -   1.123
+++ kern/uipc_usrreq.c  16 Apr 2018 08:13:52 -
@@ -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--;