seL4 / seL4_libs

No-assurance libraries for rapid-prototyping of seL4 apps.
https://docs.sel4.systems
Other
52 stars 62 forks source link

Remove assert in libsel4muslcsys/src/sys_io.c::sys_close() #49

Closed bigcardo closed 2 years ago

bigcardo commented 2 years ago

There we assert that the only file type admitted is FILE_TYPE_CPIO, however libsel4camkes implements support to FILE_TYPE_SOCKET.

Signed-off-by: Carmelo Pintaudi carmelo.pintaudi@hensoldt-cyber.de

axel-h commented 2 years ago

Is the call coming from camkes_sys_close() when we arrive here? I'm not sure how to handle this properly for unknown types, maybe sys_close() should not be called at all then?

bigcardo commented 2 years ago

Yes the call is coming from camkes_sys_close(). If you do not call sys_close() then you have to provide a close() function that takes the freeing actions like add_free_fd(fd). This would be quite odd and resulting in a mixed paradigm: open() goes thru the libc while close() not.

axel-h commented 2 years ago

@kent-mcleod Why is you opinion on this, how it this supposed to be used with custom types?

kent-mcleod commented 2 years ago

I expanded more here (https://github.com/seL4/seL4_libs/pull/49#discussion_r771038071). But essentially, adding a call to the camkes component: camkes_install_io_syscalls() will install the camkes_sys_close syscall which handles sockets.

bigcardo commented 2 years ago

As far as I understand, and from my experience, camkes_sys_close() is called and it does handle sockets but..it does call original_sys_close()

static long camkes_sys_close(va_list ap)
{
    va_list copy;
    va_copy(copy, ap);
    int fd = va_arg(ap, int);
    if (sock_close && valid_fd(fd)) {
        muslcsys_fd_t *fds =  get_fd_struct(fd);
        if (fds->filetype == FILE_TYPE_SOCKET) {
            sock_close(*(int *)fds->data);
        }
    }
    long ret;
    if (original_sys_close) {
        ret = original_sys_close(copy);
    } else {
        ret = -ENOSYS;
    }
    va_end(copy);
    return ret;
}

To me this seems reasonable because original_sys_close() (that would be libsel4muslcsys/src/sys_io.c::sys_close()) will take care of performing add_free_fd()

long sys_close(va_list ap)
{
    int fd = va_arg(ap, int);
    if (fd < FIRST_USER_FD) {
        assert(!"not implemented");
        return -EBADF;
    }

    if (!valid_fd(fd)) {
        return -EBADF;
    }

    muslcsys_fd_t *fds = get_fd_struct(fd);

    if (fds->filetype == FILE_TYPE_CPIO) {
        free(fds->data);
    } else {
        assert(!"not implemented");
    }
    add_free_fd(fd);
    return 0;
}

The sockclose() function can't do that because is working on (int )fds->data_, not on fd. Overriding the sys_close() to me does not seem to be a reasonable option because this would lead, for consistency, to override the whole layer included the sys_open_impl() that would take care of performing the dual function of add_free_fd(): allocate_fd()

Is overriding sys_close() the only possible way here?

kent-mcleod commented 2 years ago

You're right that my suggestion won't work without removing the assert.