Hello,
Mikhail Karpov, le lun. 26 oct. 2026 00:27:41 +0700, a ecrit:
> diff --git a/console/console.c b/console/console.c
> index efc83da..7157588 100644
> --- a/console/console.c
> +++ b/console/console.c
> @@ -1234,6 +1234,7 @@ netfs_S_io_map (struct protid *cred,
>
> flags = cred->po->openstat & (O_READ | O_WRITE);
>
> + error_t err;
> pthread_mutex_lock (&np->lock);
> switch (flags)
> {
> @@ -1242,7 +1243,13 @@ netfs_S_io_map (struct protid *cred,
> VM_PROT_READ | VM_PROT_WRITE);
> if (*wrobj == MACH_PORT_NULL)
> goto error;
> - mach_port_mod_refs (mach_task_self (), *rdobj, MACH_PORT_RIGHT_SEND,
> 1);
> + err = mach_port_mod_refs (mach_task_self (), *rdobj,
> + MACH_PORT_RIGHT_SEND, 1);
> + if (err)
> + {
Before returning, *rdobj needs to be deallocate to drop the send right
taken by display_get_filemap, which is otherwise leaked.
> + pthread_mutex_unlock (&np->lock);
> + return err;
> + }
> break;
> case O_READ:
> *rdobj = display_get_filemap (vcons->display, VM_PROT_READ);
> diff --git a/libdiskfs/peropen-make.c b/libdiskfs/peropen-make.c
> index b4564a9..5b0f316 100644
> --- a/libdiskfs/peropen-make.c
> +++ b/libdiskfs/peropen-make.c
> po->shadow_root = context->shadow_root;
> if (po->shadow_root)
> @@ -69,8 +72,12 @@ diskfs_make_peropen (struct node *np, int flags, struct
> peropen *context,
>
> po->shadow_root_parent = context->shadow_root_parent;
> if (po->shadow_root_parent != MACH_PORT_NULL)
> - mach_port_mod_refs (mach_task_self (), po->shadow_root_parent,
> - MACH_PORT_RIGHT_SEND, 1);
> + {
> + err = mach_port_mod_refs (mach_task_self (), po->shadow_root_parent,
> + MACH_PORT_RIGHT_SEND, 1);
> + if (err)
> + goto free_path;
There is also the nref on po->shadow_root that needs to be released.
> diff --git a/libfshelp/translator-list.c b/libfshelp/translator-list.c
> index 6d4f941..6a42549 100644
> --- a/libfshelp/translator-list.c
> +++ b/libfshelp/translator-list.c
> @@ -147,8 +147,12 @@ fshelp_set_active_translator (struct port_info *pi,
>
> if (MACH_PORT_VALID (t->active))
> mach_port_deallocate (mach_task_self (), t->active);
> - mach_port_mod_refs (mach_task_self (), transbox->active,
> - MACH_PORT_RIGHT_SEND, +1);
> +
> + err = mach_port_mod_refs (mach_task_self (), transbox->active,
> + MACH_PORT_RIGHT_SEND, +1);
> + if (err)
> + goto out;
kMmm, we will have already modified stuff above. Better move
the mod_refs call above to avoid doing anything if the mod_refs
fails. Also, on mod_refs failure, we need to revert the
mach_port_request_notification() action.
> diff --git a/libnetfs/make-peropen.c b/libnetfs/make-peropen.c
> index 9bb4892..1ebe5ee 100644
> --- a/libnetfs/make-peropen.c
> +++ b/libnetfs/make-peropen.c
> @@ -75,11 +72,22 @@ netfs_make_peropen (struct node *np, int flags, struct
> peropen *context)
>
> po->shadow_root_parent = context->shadow_root_parent;
> if (po->shadow_root_parent != MACH_PORT_NULL)
> - mach_port_mod_refs (mach_task_self (), po->shadow_root_parent,
> - MACH_PORT_RIGHT_SEND, 1);
> + {
> + err = mach_port_mod_refs (mach_task_self (), po->shadow_root_parent,
> + MACH_PORT_RIGHT_SEND, 1);
> + if (err)
> + goto free_path;
Same here.
> diff --git a/libports/complete-deallocate.c b/libports/complete-deallocate.c
> index b7d5f76..a6fe0ee 100644
> --- a/libports/complete-deallocate.c
> +++ b/libports/complete-deallocate.c
> @@ -47,8 +47,9 @@ _ports_complete_deallocate (struct port_info *pi)
> refcounts_deref_weak (&pi->refcounts, &result);
> pthread_rwlock_unlock (&_ports_htable_lock);
>
> - mach_port_mod_refs (mach_task_self (), pi->port_right,
> - MACH_PORT_RIGHT_RECEIVE, -1);
> + error_t err = mach_port_mod_refs (mach_task_self (), pi->port_right,
> + MACH_PORT_RIGHT_RECEIVE, -1);
> + assert_perror_backtrace (err);
Yes, that's not supposed to fail.
> diff --git a/libshouldbeinlibc/exec-reauth.c b/libshouldbeinlibc/exec-reauth.c
> index b830f22..c04fe05 100644
> --- a/libshouldbeinlibc/exec-reauth.c
> +++ b/libshouldbeinlibc/exec-reauth.c
> @@ -59,25 +59,29 @@ exec_reauth (auth_t auth, int secure, int must_reauth,
> if (!err)
> err = auth_user_authenticate (auth, ref, MACH_MSG_TYPE_MAKE_SEND,
> &newport);
> - mach_port_mod_refs (mach_task_self (), ref, MACH_PORT_RIGHT_RECEIVE,
> -1);
> - /* If they gave us nothing, make sure not to proceed. */
> - if (!err && newport == MACH_PORT_NULL)
> - err = KERN_INVALID_ARGUMENT;
> + error_t refs_err = mach_port_mod_refs (mach_task_self (), ref,
> + MACH_PORT_RIGHT_RECEIVE, -1);
Here as well, that's not supposed to fail, better abort than trying to
fix up things, we're very screwed anyway if things went that badly that
we don't even have a receive right to drop.
> diff --git a/libshouldbeinlibc/portxlate.c b/libshouldbeinlibc/portxlate.c
> index eb6abfb..2ff8b7a 100644
> --- a/libshouldbeinlibc/portxlate.c
> +++ b/libshouldbeinlibc/portxlate.c
> @@ -30,55 +30,53 @@ port_name_xlator_create (mach_port_t from_task,
> mach_port_t to_task,
> struct port_name_xlator **xlator)
> {
> error_t err;
> - struct port_name_xlator *x = malloc (sizeof (struct port_name_xlator));
> + struct port_name_xlator *x = calloc (1, sizeof (struct port_name_xlator));
>
> if (! x)
> - return ENOMEM;
> + return errno;
> +
> + err = mach_port_mod_refs (mach_task_self (), from_task,
> + MACH_PORT_RIGHT_SEND, +1);
> + if (err)
> + goto error;
>
> - mach_port_mod_refs (mach_task_self (), from_task, MACH_PORT_RIGHT_SEND,
> +1);
> x->from_task = from_task;
> - mach_port_mod_refs (mach_task_self (), to_task, MACH_PORT_RIGHT_SEND, +1);
> + err = mach_port_mod_refs (mach_task_self (), to_task,
> + MACH_PORT_RIGHT_SEND, +1);
> + if (err)
Here you need to deallocate from_task.
> diff --git a/libstore/unknown.c b/libstore/unknown.c
> index 624c184..058285c 100644
> --- a/libstore/unknown.c
> +++ b/libstore/unknown.c
> @@ -60,10 +60,7 @@ noflags (struct store *store, int flags)
> static struct store_enc *
> duplicate_encoding (struct store_enc *enc)
> {
> - struct store_enc *us;
> - size_t i;
> -
> - us = calloc (1, sizeof *us);
> + struct store_enc *us = calloc (1, sizeof *us);
> if (us == NULL)
> return NULL;
>
> @@ -98,9 +95,14 @@ duplicate_encoding (struct store_enc *enc)
> us->num_offsets = enc->num_offsets;
> us->data_len = enc->data_len;
>
> - for (i = 0; i < us->num_ports; ++i)
> - mach_port_mod_refs (mach_task_self (), us->ports[i],
> - MACH_PORT_RIGHT_SEND, +1);
> + error_t err;
> + for (size_t i = 0; i < us->num_ports; ++i)
> + {
> + err = mach_port_mod_refs (mach_task_self (), us->ports[i],
> + MACH_PORT_RIGHT_SEND, +1);
> + if (err)
> + goto no_memory;
store_enc_dealloc needs to properly deallocate only the first part of
the ports of the array. You can memset to NULL the rest of the array for
instance.
> diff --git a/proc/host.c b/proc/host.c
> index adda74d..9d3f204 100644
> --- a/proc/host.c
> +++ b/proc/host.c
> @@ -139,7 +139,6 @@ S_proc_getexecdata (struct proc *p,
> int **ints,
> mach_msg_type_number_t *nints)
> {
> - int i;
> int ports_allocated = 0;
> /* No need to check P here; we don't use it. */
>
> @@ -151,28 +150,45 @@ S_proc_getexecdata (struct proc *p,
> *ports = mmap (0, round_page (n_std_ports * sizeof (mach_port_t)),
> PROT_READ|PROT_WRITE, MAP_ANON, 0, 0);
> if (*ports == MAP_FAILED)
> - return ENOMEM;
> + return errno;
> ports_allocated = 1;
> }
> memcpy (*ports, std_port_array, n_std_ports * sizeof (mach_port_t));
> *nports = n_std_ports;
>
> + error_t err;
> + int ints_allocated = 0;
> if (*nints < n_std_ints)
> {
> *ints = mmap (0, round_page (n_std_ints * sizeof (int)),
> PROT_READ|PROT_WRITE, MAP_ANON, 0, 0);
> if (*ints == MAP_FAILED)
> {
> + err = errno;
> if (ports_allocated)
> munmap (*ports, round_page (n_std_ports * sizeof (mach_port_t)));
> - return ENOMEM;
> + return err;
> }
> +
> + ints_allocated = 1;
> }
> memcpy (*ints, std_int_array, n_std_ints * sizeof (int));
> *nints = n_std_ints;
>
> - for (i = 0; i < n_std_ports; i++)
> - mach_port_mod_refs (mach_task_self (), std_port_array[i],
> MACH_PORT_RIGHT_SEND, 1);
> + for (int i = 0; i < n_std_ports; i++)
> + {
> + err = mach_port_mod_refs (mach_task_self (), std_port_array[i],
> + MACH_PORT_RIGHT_SEND, 1);
> + if (err)
> + {
You also need to deallocate the first part of the ports of the array.
> + if (ports_allocated)
> + munmap (*ports, round_page (n_std_ports * sizeof (mach_port_t)));
> + if (ints_allocated)
> + munmap (*ints, round_page (n_std_ints * sizeof (int)));
> + return err;
It's worth factorizing this with error labels.
> diff --git a/proc/mgt.c b/proc/mgt.c
> index 8223bf5..18ee1e5 100644
> --- a/proc/mgt.c
> +++ b/proc/mgt.c
> @@ -283,8 +283,12 @@ S_proc_child (struct proc *parentp,
>
> if (MACH_PORT_VALID (parentp->p_task_namespace))
> {
> - mach_port_mod_refs (mach_task_self (), parentp->p_task_namespace,
> - MACH_PORT_RIGHT_SEND, +1);
> + error_t err = mach_port_mod_refs (mach_task_self (),
> + parentp->p_task_namespace,
> + MACH_PORT_RIGHT_SEND, +1);
> + if (err)
> + return err;
That's very late for returning an error... We'd want to move that up to
avoid doing anything on error.
> @@ -1386,19 +1390,25 @@ S_mach_notify_new_task (struct port_info *notify,
> if (! parentp)
> return ESRCH;
>
> + error_t err;
> childp = task_find_nocreate (task);
> if (! childp)
> {
> - mach_port_mod_refs (mach_task_self (), task, MACH_PORT_RIGHT_SEND, +1);
> + err = mach_port_mod_refs (mach_task_self (), task,
> + MACH_PORT_RIGHT_SEND, +1);
> + if (err)
> + return err;
> +
> childp = new_proc (task);
> }
>
> if (MACH_PORT_VALID (parentp->p_task_namespace))
> {
> - error_t err;
> /* Tasks in a task namespace are not expected to call
> proc_child, so we do it on their behalf. */
> - mach_port_mod_refs (mach_task_self (), task, MACH_PORT_RIGHT_SEND, +1);
> + err = mach_port_mod_refs (mach_task_self (), task,
> + MACH_PORT_RIGHT_SEND, +1);
> + assert_perror_backtrace (err);
No, it's fine to return an error, we can still do it at this point (just
need to deallocate on task if the mod_refs was done above).
> err = S_proc_child (parentp, task);
> assert_perror_backtrace (err);
That however is really not supposed to happen, thus the assertion.
> diff --git a/proc/pgrp.c b/proc/pgrp.c
> index e2b497e..81129c2 100644
> --- a/proc/pgrp.c
> +++ b/proc/pgrp.c
> @@ -116,7 +120,17 @@ S_proc_setsid (struct proc *p)
> leave_pgrp (p);
>
> sess = new_session (p);
> - p->p_pgrp= new_pgrp (p->p_pid, sess);
> + if (!sess)
> + return errno;
> +
> + p->p_pgrp = new_pgrp (p->p_pid, sess);
> + if (!p->p_pgrp)
Those are again too late, move them up so we don't leave_pgrp if we
error out actually.
I have applied the rest, thanks!
Samuel
