Hello,
There are several places where the return values of the functions
netfs_make_peropen and netfs_make_protid are not checked. I also examined
the code in corresponding files within libdiskfs and found minor differences
in the implementation of certain functions. It might be worth investigating
these areas for potential errors.
In libnetfs/dir-lookup.c, iohelp_free_iouser (user) is called only in the
event of an error
err = iohelp_create_empty_iouser (&user);
if (! err)
{
newpo = netfs_make_peropen (dnp, 0, dircred->po);
if (! newpo)
{
err = errno;
iohelp_free_iouser (user);
}
else
{
newpi = netfs_make_protid (newpo, user);
if (! newpi)
{
err = errno;
iohelp_free_iouser (user);
}
else
newpo = NULL;
}
}
while in libdiskfs/dir-lookup.c, iohelp_free_iouser (user) is called
regardless of the outcome of diskfs_make_peropen and diskfs_create_protid.
err = iohelp_create_empty_iouser (&user);
if (! err)
{
err = diskfs_make_peropen (dnp, 0, dircred->po, &newpo);
if (! err)
{
err = diskfs_create_protid (newpo, user, &newpi);
if (! err)
newpo = 0;
}
iohelp_free_iouser (user);
}
In libnetfs/io-restrict-auth.c, iohelp_free_iouser (new_user) is called only
if an error occurs
refcount_ref (&user->po->refcnt);
newpi = netfs_make_protid (user->po, new_user);
if (newpi)
{
*newport = ports_get_right (newpi);
*newporttype = MACH_MSG_TYPE_MAKE_SEND;
ports_port_deref (newpi);
}
else
{
refcount_deref (&user->po->refcnt);
iohelp_free_iouser (new_user);
err = ENOMEM;
}
return err;
}
while in libdiskfs/io-restrict-auth.c, iohelp_free_iouser (user) is called
regardless of the result of diskfs_create_protid.
refcount_ref (&cred->po->refcnt);
err = diskfs_create_protid (cred->po, user, &newpi);
if (! err)
{
*newport = ports_get_right (newpi);
*newportpoly = MACH_MSG_TYPE_MAKE_SEND;
ports_port_deref (newpi);
}
else
refcount_deref (&cred->po->refcnt);
iohelp_free_iouser (user);
return err;
}
Also, I have removed a redundant error check from both
libdiskfs/dir-lookup.c
and libnetfs/dir-lookup.c, as the code would proceed to goto out upon
exiting
the block in any case.
Thanks,
--
Mikhail Karpov
From 7d65aec5415a4a0f04532c032939d5421f06844b Mon Sep 17 00:00:00 2001
From: Mikhail Karpov <[email protected]>
Date: Sat, 7 Nov 2026 15:34:44 +0700
Subject: [PATCH 3/4] Added checks for netfs_make_peropen and netfs_make_protid
return value in several place
---
libnetfs/dir-lookup.c | 43 +++++++++++----
libnetfs/dir-mkfile.c | 55 +++++++++++--------
libnetfs/file-exec.c | 113 +++++++++++++++++++++------------------
libnetfs/file-reparent.c | 62 +++++++++++++--------
libnetfs/fsys-getroot.c | 22 ++++++--
libnetfs/io-duplicate.c | 22 ++++++--
trans/fakeroot.c | 40 +++++++++-----
7 files changed, 228 insertions(+), 129 deletions(-)
diff --git a/libnetfs/dir-lookup.c b/libnetfs/dir-lookup.c
index 9a17ddb..ef8d780 100644
--- a/libnetfs/dir-lookup.c
+++ b/libnetfs/dir-lookup.c
@@ -47,6 +47,7 @@ netfs_S_dir_lookup (struct protid *dircred,
char *nextname;
char *relpath;
error_t err;
+ struct peropen *newpo = NULL;
struct protid *newpi = NULL;
struct iouser *user;
@@ -224,14 +225,23 @@ netfs_S_dir_lookup (struct protid *dircred,
err = iohelp_create_empty_iouser (&user);
if (! err)
{
- newpi = netfs_make_protid (netfs_make_peropen (dnp, 0,
- dircred->po),
- user);
- if (! newpi)
- {
+ newpo = netfs_make_peropen (dnp, 0, dircred->po);
+ if (! newpo)
+ {
err = errno;
iohelp_free_iouser (user);
}
+ else
+ {
+ newpi = netfs_make_protid (newpo, user);
+ if (! newpi)
+ {
+ err = errno;
+ iohelp_free_iouser (user);
+ }
+ else
+ newpo = NULL;
+ }
}
if (! err)
@@ -315,8 +325,6 @@ netfs_S_dir_lookup (struct protid *dircred,
&np->transbox);
if (complete_path != translator_path)
free(complete_path);
- if (err)
- goto out;
}
goto out;
@@ -434,14 +442,25 @@ netfs_S_dir_lookup (struct protid *dircred,
if (err)
goto out;
- newpi = netfs_make_protid (netfs_make_peropen (np, flags, dircred->po),
- user);
- if (! newpi)
+ newpo = netfs_make_peropen (np, flags, dircred->po);
+ if (! newpo)
{
- iohelp_free_iouser (user);
err = errno;
+ iohelp_free_iouser (user);
goto out;
}
+ else
+ {
+ newpi = netfs_make_protid (newpo, user);
+ if (! newpi)
+ {
+ err = errno;
+ iohelp_free_iouser (user);
+ goto out;
+ }
+ else
+ newpo = NULL;
+ }
mach_port_t rendezvous = MACH_PORT_NULL;
struct flock64 lock =
@@ -497,6 +516,8 @@ netfs_S_dir_lookup (struct protid *dircred,
netfs_nrele (dnp);
if (newpi)
ports_port_deref (newpi);
+ if (newpo)
+ netfs_release_peropen (newpo);
free (relpath);
return err;
diff --git a/libnetfs/dir-mkfile.c b/libnetfs/dir-mkfile.c
index 9036101..90f5b69 100644
--- a/libnetfs/dir-mkfile.c
+++ b/libnetfs/dir-mkfile.c
@@ -30,34 +30,45 @@ netfs_S_dir_mkfile (struct protid *diruser, int flags, mode_t mode,
struct node *np;
struct iouser *user;
struct protid *newpi;
+ struct peropen *newpo;
pthread_mutex_lock (&diruser->po->np->lock);
err = netfs_attempt_mkfile (diruser->user, diruser->po->np, mode, &np);
+ if (err)
+ return err;
- if (!err)
+ /* the dir is now unlocked and NP is locked */
+ flags &= ~OPENONLY_STATE_MODES;
+ err = iohelp_dup_iouser (&user, diruser->user);
+ if (err)
+ goto out;
+
+ newpo = netfs_make_peropen (np, flags, diruser->po);
+ if (!newpo)
+ {
+ err = errno;
+ goto free_user;
+ }
+
+ newpi = netfs_make_protid (newpo, user);
+ if (!newpi)
{
- /* the dir is now unlocked and NP is locked */
- flags &= ~OPENONLY_STATE_MODES;
- err = iohelp_dup_iouser (&user, diruser->user);
- if (! err)
- {
- newpi = netfs_make_protid (netfs_make_peropen (np, flags,
- diruser->po),
- user);
- if (newpi)
- {
- *newfile = ports_get_right (newpi);
- *newfiletype = MACH_MSG_TYPE_MAKE_SEND;
- ports_port_deref (newpi);
- }
- else
- {
- err = errno;
- iohelp_free_iouser (user);
- }
- }
- netfs_nput (np);
+ err = errno;
+ goto release_newpo;
}
+ *newfile = ports_get_right (newpi);
+ *newfiletype = MACH_MSG_TYPE_MAKE_SEND;
+ ports_port_deref (newpi);
+
+ out:
+ netfs_nput (np);
return err;
+
+ release_newpo:
+ netfs_release_peropen (newpo);
+
+ free_user:
+ iohelp_free_iouser (user);
+ goto out;
}
diff --git a/libnetfs/file-exec.c b/libnetfs/file-exec.c
index b5293af..ad5d1ee 100644
--- a/libnetfs/file-exec.c
+++ b/libnetfs/file-exec.c
@@ -157,60 +157,64 @@ netfs_S_file_exec_paths (struct protid *cred,
flags |= EXEC_NEWTASK;
#endif
- if (! err)
+ if (err)
+ return err;
+
+ struct iouser *user;
+ struct protid *newpi;
+ struct peropen *newpo;
+
+ err = iohelp_dup_iouser (&user, cred->user);
+ if (err)
+ return err;
+
+ newpo = netfs_make_peropen (np, O_READ, cred->po);
+ if (!newpo)
{
- struct iouser *user;
- struct protid *newpi;
-
- err = iohelp_dup_iouser (&user, cred->user);
- if (! err)
- {
- newpi = netfs_make_protid (netfs_make_peropen (np, O_READ, cred->po),
- user);
- if (newpi)
- {
- right = ports_get_send_right (newpi);
-
- /* Add a gratuitous send right on the protid to avoid a no-sender, and thus
- interrupt the exec, just because we are precisely replacing the calling
- process! */
- cred_right = ports_get_send_right (cred);
+ err = errno;
+ goto free_user;
+ }
+
+ newpi = netfs_make_protid (newpo, user);
+ if (!newpi)
+ {
+ err = errno;
+ goto release_newpo;
+ }
+
+ right = ports_get_send_right (newpi);
+
+ /* Add a gratuitous send right on the protid to avoid a no-sender, and thus
+ interrupt the exec, just because we are precisely replacing the calling
+ process! */
+ cred_right = ports_get_send_right (cred);
#ifdef HAVE_EXEC_EXEC_PATHS
- err = exec_exec_paths (_netfs_exec,
- right, MACH_MSG_TYPE_COPY_SEND,
- task, flags, path, abspath,
- argv, argvlen, envp, envplen,
- fds, MACH_MSG_TYPE_COPY_SEND, fdslen,
- portarray, MACH_MSG_TYPE_COPY_SEND,
- portarraylen,
- intarray, intarraylen,
- deallocnames, deallocnameslen,
- destroynames, destroynameslen);
- /* For backwards compatibility. Just drop it when we kill
- exec_exec. */
- if (err == MIG_BAD_ID)
+ err = exec_exec_paths (_netfs_exec,
+ right, MACH_MSG_TYPE_COPY_SEND,
+ task, flags, path, abspath,
+ argv, argvlen, envp, envplen,
+ fds, MACH_MSG_TYPE_COPY_SEND, fdslen,
+ portarray, MACH_MSG_TYPE_COPY_SEND,
+ portarraylen,
+ intarray, intarraylen,
+ deallocnames, deallocnameslen,
+ destroynames, destroynameslen);
+ /* For backwards compatibility. Just drop it when we kill exec_exec. */
+ if (err == MIG_BAD_ID)
#endif
- err = exec_exec (_netfs_exec,
- right, MACH_MSG_TYPE_COPY_SEND,
- task, flags, argv, argvlen, envp, envplen,
- fds, MACH_MSG_TYPE_COPY_SEND, fdslen,
- portarray, MACH_MSG_TYPE_COPY_SEND,
- portarraylen,
- intarray, intarraylen,
- deallocnames, deallocnameslen,
- destroynames, destroynameslen);
-
- mach_port_deallocate (mach_task_self (), right);
- mach_port_deallocate (mach_task_self (), cred_right);
- ports_port_deref (newpi);
- }
- else
- {
- err = errno;
- iohelp_free_iouser (user);
- }
- }
- }
+ err = exec_exec (_netfs_exec,
+ right, MACH_MSG_TYPE_COPY_SEND,
+ task, flags, argv, argvlen, envp, envplen,
+ fds, MACH_MSG_TYPE_COPY_SEND, fdslen,
+ portarray, MACH_MSG_TYPE_COPY_SEND,
+ portarraylen,
+ intarray, intarraylen,
+ deallocnames, deallocnameslen,
+ destroynames, destroynameslen);
+
+ mach_port_deallocate (mach_task_self (), right);
+ mach_port_deallocate (mach_task_self (), cred_right);
+ ports_port_deref (newpi);
if (! err)
{
@@ -224,4 +228,11 @@ netfs_S_file_exec_paths (struct protid *cred,
}
return err;
+
+ release_newpo:
+ netfs_release_peropen (newpo);
+
+ free_user:
+ iohelp_free_iouser (user);
+ return err;
}
diff --git a/libnetfs/file-reparent.c b/libnetfs/file-reparent.c
index b04bab3..3205db3 100644
--- a/libnetfs/file-reparent.c
+++ b/libnetfs/file-reparent.c
@@ -30,6 +30,7 @@ netfs_S_file_reparent (struct protid *cred, mach_port_t parent,
struct node *node;
struct protid *new_cred;
struct iouser *user;
+ struct peropen *newpo;
if (! cred)
return EOPNOTSUPP;
@@ -41,34 +42,49 @@ netfs_S_file_reparent (struct protid *cred, mach_port_t parent,
node = cred->po->np;
pthread_mutex_lock (&node->lock);
-
- new_cred =
- netfs_make_protid (netfs_make_peropen (node, cred->po->openstat, cred->po),
- user);
+ newpo = netfs_make_peropen (node, cred->po->openstat, cred->po);
+ if (!newpo)
+ {
+ err = errno;
+ pthread_mutex_unlock (&node->lock);
+ goto free_user;
+ }
+
+ new_cred = netfs_make_protid (newpo, user);
+ if (!new_cred)
+ {
+ err = errno;
+ pthread_mutex_unlock (&node->lock);
+ goto release_newpo;
+ }
+
pthread_mutex_unlock (&node->lock);
- if (new_cred)
+ /* Remove old shadow root state. */
+ if (new_cred->po->shadow_root && new_cred->po->shadow_root != node)
{
- /* Remove old shadow root state. */
- if (new_cred->po->shadow_root && new_cred->po->shadow_root != node)
- {
- pthread_mutex_lock (&new_cred->po->shadow_root->lock);
- netfs_nput (new_cred->po->shadow_root);
- }
- if (new_cred->po->shadow_root_parent)
- mach_port_deallocate (mach_task_self (), new_cred->po->shadow_root_parent);
+ pthread_mutex_lock (&new_cred->po->shadow_root->lock);
+ netfs_nput (new_cred->po->shadow_root);
+ }
- /* And install PARENT instead. */
- new_cred->po->shadow_root = node;
- new_cred->po->shadow_root_parent = parent;
+ if (new_cred->po->shadow_root_parent)
+ mach_port_deallocate (mach_task_self (),
+ new_cred->po->shadow_root_parent);
- *new_file = ports_get_right (new_cred);
- *new_file_type = MACH_MSG_TYPE_MAKE_SEND;
+ /* And install PARENT instead. */
+ new_cred->po->shadow_root = node;
+ new_cred->po->shadow_root_parent = parent;
- ports_port_deref (new_cred);
+ *new_file = ports_get_right (new_cred);
+ *new_file_type = MACH_MSG_TYPE_MAKE_SEND;
- return 0;
- }
- else
- return errno;
+ ports_port_deref (new_cred);
+ return 0;
+
+ release_newpo:
+ netfs_release_peropen (newpo);
+
+ free_user:
+ iohelp_free_iouser (user);
+ return err;
}
diff --git a/libnetfs/fsys-getroot.c b/libnetfs/fsys-getroot.c
index fb9fef1..c73ad3b 100644
--- a/libnetfs/fsys-getroot.c
+++ b/libnetfs/fsys-getroot.c
@@ -41,6 +41,7 @@ netfs_S_fsys_getroot (struct netfs_control *pt,
struct iouser *cred;
error_t err;
struct protid *newpi;
+ struct peropen *newpo;
mode_t type;
struct peropen peropen_context =
{
@@ -133,9 +134,20 @@ netfs_S_fsys_getroot (struct netfs_control *pt,
flags &= ~OPENONLY_STATE_MODES;
- newpi = netfs_make_protid (netfs_make_peropen (netfs_root_node, flags,
- &peropen_context),
- cred);
+ newpo = netfs_make_peropen (netfs_root_node, flags, &peropen_context);
+ if (!newpo)
+ {
+ err = errno;
+ goto out;
+ }
+
+ newpi = netfs_make_protid (newpo, cred);
+ if (!newpi)
+ {
+ err = errno;
+ goto release_newpo;
+ }
+
mach_port_deallocate (mach_task_self (), dotdot);
*do_retry = FS_RETRY_NORMAL;
@@ -149,4 +161,8 @@ netfs_S_fsys_getroot (struct netfs_control *pt,
iohelp_free_iouser (cred);
pthread_mutex_unlock (&netfs_root_node->lock);
return err;
+
+ release_newpo:
+ netfs_release_peropen (newpo);
+ goto out;
}
diff --git a/libnetfs/io-duplicate.c b/libnetfs/io-duplicate.c
index e6ac7a1..bca0949 100644
--- a/libnetfs/io-duplicate.c
+++ b/libnetfs/io-duplicate.c
@@ -34,12 +34,24 @@ netfs_S_io_duplicate (struct protid *user,
if (err)
return err;
- refcount_ref (&user->po->refcnt);
pthread_mutex_lock (&user->po->np->lock);
+
+ refcount_ref (&user->po->refcnt);
newpi = netfs_make_protid (user->po, clone);
- *newport = ports_get_right (newpi);
+ if (newpi)
+ {
+ *newport = ports_get_right (newpi);
+ *newporttp = MACH_MSG_TYPE_MAKE_SEND;
+ ports_port_deref (newpi);
+ }
+ else
+ {
+ err = errno;
+ iohelp_free_iouser (clone);
+ refcount_deref (&user->po->refcnt);
+ }
+
pthread_mutex_unlock (&user->po->np->lock);
- *newporttp = MACH_MSG_TYPE_MAKE_SEND;
- ports_port_deref (newpi);
- return 0;
+
+ return err;
}
diff --git a/trans/fakeroot.c b/trans/fakeroot.c
index 764d6fd..fd11f1d 100644
--- a/trans/fakeroot.c
+++ b/trans/fakeroot.c
@@ -292,6 +292,7 @@ netfs_S_dir_lookup (struct protid *diruser,
{
struct node *dnp, *np;
error_t err;
+ struct peropen *newpo;
struct protid *newpi;
struct iouser *user;
mach_port_t file;
@@ -489,27 +490,38 @@ netfs_S_dir_lookup (struct protid *diruser,
flags &= ~(O_CREAT|O_EXCL|O_NOLINK|O_NOTRANS|O_NONBLOCK);
err = iohelp_dup_iouser (&user, diruser->user);
- if (!err)
+ if (err)
+ goto lose;
+
+ newpo = netfs_make_peropen (np, flags, diruser->po);
+ if (!newpo)
{
- newpi = netfs_make_protid (netfs_make_peropen (np, flags, diruser->po),
- user);
- if (! newpi)
- {
- iohelp_free_iouser (user);
- err = errno;
- }
- else
- {
- *retry_port = ports_get_right (newpi);
- *retry_port_type = MACH_MSG_TYPE_MAKE_SEND;
- ports_port_deref (newpi);
- }
+ err = errno;
+ goto free_user;
+ }
+
+ newpi = netfs_make_protid (newpo, user);
+ if (!newpi)
+ {
+ err = errno;
+ goto release_newpo;
}
+ *retry_port = ports_get_right (newpi);
+ *retry_port_type = MACH_MSG_TYPE_MAKE_SEND;
+ ports_port_deref (newpi);
+
lose:
if (np != NULL)
netfs_nput (np);
return err;
+
+ release_newpo:
+ netfs_release_peropen (newpo);
+
+ free_user:
+ iohelp_free_iouser (user);
+ goto lose;
}
/* The user may define this function. Attempt to set the passive
--
2.43.0
From 0c8162a50a2d28c9e650336a29174730690ac8e8 Mon Sep 17 00:00:00 2001
From: Mikhail Karpov <[email protected]>
Date: Sat, 7 Nov 2026 15:32:03 +0700
Subject: [PATCH 2/4] Remove an unnecessary error check in
libdiskfs/dir-lookup.c
---
libdiskfs/dir-lookup.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/libdiskfs/dir-lookup.c b/libdiskfs/dir-lookup.c
index 7c41683..538baa5 100644
--- a/libdiskfs/dir-lookup.c
+++ b/libdiskfs/dir-lookup.c
@@ -332,8 +332,6 @@ try_lookup (struct protid *dircred,
&np->transbox);
if (complete_path != translator_path)
free(complete_path);
- if (err)
- goto out;
}
goto out;
--
2.43.0
