Re: [Minios-devel] [UNIKRAFT PATCH] lib/{ramfs, vfscore}: fix invalid error codes

[Sharan Santhanam <sharan.santhanam@xxxxxxxxx>]

Hello Hugo,

Please find the comment inline:

On 3/1/20 10:19 AM, Hugo Lefeuvre wrote:
> + ramfs_truncate and ramfs_write currently return EIO upon failing to
>    allocate memory for ramfs_node file data buffers. In the majority of
>    cases this is caused by the allocator running out-of-memory, so
>    returning ENOMEM seems more appropriate. This avoids OOM issues hiding
>    behind obscure I/O error messages.
>
> + vfscore_vget returns 1 if the vnode was found in cache, 0 otherwise.
>
>    If we fall back to allocating a new vnode (vn_lookup returned NULL)
>    then the vnode was not found in cache and it does not make sense to
>    return anything else than 0.
>
>    In particular, this line is reached if VFS_VGET fails, meaning that
>    error will systematically be > 0. Since most calls to vfscore_vget
>    check for > 0 instead of == 1, they will assume that the vnode was
>    found in cache and dereference vpp without previously checking it,
>    causing a NULL pointer dereference.
>
>    This is not an issue for the moment since all vfs_vget implementations
>    are linked to vfscore_nullop, but might become an issue in the future
>    when those will be implemented.
>
> Signed-off-by: Hugo Lefeuvre <hugo.lefeuvre@xxxxxxxxx>
> ---
>   lib/ramfs/ramfs_vnops.c | 4 ++--
>   lib/vfscore/vnode.c     | 2 +-
>   2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/lib/ramfs/ramfs_vnops.c b/lib/ramfs/ramfs_vnops.c
> index 6eca9b2..2ea2626 100644
> --- a/lib/ramfs/ramfs_vnops.c
> +++ b/lib/ramfs/ramfs_vnops.c
> @@ -357,7 +357,7 @@ ramfs_truncate(struct vnode *vp, off_t length)
>                 new_size = round_pgup(length);
>                 new_buf = malloc(new_size);
>                 if (!new_buf)
> -                       return EIO;
> +                       return ENOMEM;
truncate and ftruncate libc function which uses this function does not 
return ENOMEM. I presume the choice to EIO was based on this choice. So 
I would not change it.
>                 if (np->rn_size != 0) {
>                         memcpy(new_buf, np->rn_buf, vp->v_size);
>                         if (np->rn_owns_buf)
> @@ -473,7 +473,7 @@ ramfs_write(struct vnode *vp, struct uio *uio, int ioflag)
>                         void *new_buf = calloc(1, new_size);
>   
>   			if (!new_buf)
> -                               return EIO;
> +                               return ENOMEM;
Same as above, the write function does not return ENOMEM.
>                         if (np->rn_size != 0) {
>                                 memcpy(new_buf, np->rn_buf, vp->v_size);
>                                 if (np->rn_owns_buf)
> diff --git a/lib/vfscore/vnode.c b/lib/vfscore/vnode.c
> index 6b5ea12..f3f1644 100644
> --- a/lib/vfscore/vnode.c
> +++ b/lib/vfscore/vnode.c
> @@ -209,7 +209,7 @@ vfscore_vget(struct mount *mp, uint64_t ino, struct vnode 
> **vpp)
>         if ((error = VFS_VGET(mp, vp)) != 0) {
>                 VNODE_UNLOCK();
>                 free(vp);
> -               return error;
> +               return 0;
>         }
>         vfs_busy(vp->v_mount);
>         uk_mutex_lock(&vp->v_lock);
>
> _______________________________________________
> Minios-devel mailing list
> Minios-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/minios-devel
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel