Regression on POSIX ql_syscall_open

[iMoD1998]

Describe the bug
In version 1.4.6, ql_syscall_open is hard coded to always return EPERM (-1) for evey failed open attempt, preventing some libc implementations from traversing the LD_LIBRARY_PATH. Fixing this should also fix: #1403 and possibly #1412 ??

Sample Code

def __do_open(ql: Qiling, absvpath: str, flags: int, mode: int) -> int:
    flags &= 0xffffffff
    mode &= 0xffffffff

    # look for the next available fd slot
    idx = next((i for i in range(NR_OPEN) if ql.os.fd[i] is None), -1)

    if idx == -1:
        return -EMFILE

    if ql.arch.type is QL_ARCH.ARM and ql.os.type is not QL_OS.QNX:
        mode = 0

    # translate emulated os open flags into host os open flags
    flags = ql_open_flag_mapping(ql, flags)

    try:
        ql.os.fd[idx] = ql.os.fs_mapper.open_ql_file(absvpath, flags, mode)
    except QlSyscallError:
        return -1

    return idx


def ql_syscall_open(ql: Qiling, filename: int, flags: int, mode: int):
    vpath = ql.os.utils.read_cstring(filename)
    absvpath = ql.os.path.virtual_abspath(vpath)

    regreturn = __do_open(ql, absvpath, flags, mode)

    ql.log.debug(f'open("{absvpath}", {flags:#x}, 0{mode:o}) = {regreturn}')

    return regreturn

Expected behavior
Open should return actual open error code like before.

Proposed Change
Something like the following will work, or something similar from 1.4.5.

def __do_open(ql: Qiling, absvpath: str, flags: int, mode: int) -> int:
    flags &= 0xffffffff
    mode &= 0xffffffff

    # look for the next available fd slot
    idx = next((i for i in range(NR_OPEN) if ql.os.fd[i] is None), -1)

    if idx == -1:
        return -EMFILE

    if ql.arch.type is QL_ARCH.ARM and ql.os.type is not QL_OS.QNX:
        mode = 0

    # translate emulated os open flags into host os open flags
    flags = ql_open_flag_mapping(ql, flags)

    try:
        ql.os.fd[idx] = ql.os.fs_mapper.open_ql_file(absvpath, flags, mode)
    except QlSyscallError as e:
        return -e.errno

    return idx

[elicn]

Hi there and thanks for reporting this.
Are you sure this is a real issue? Maybe a test case that supports it?

According to POSIX manuals -1 should be returned on error and the program is resonsible to read errno and decide what how to handle the error: "On error, -1 is returned and errno is set to indicate the error".

It is true that we don't set errno anywhere there (and maybe we should), but the fix doesn't seem to be the one you suggested.

[iMoD1998]

Hello, you are correct about libc implementation returning -1 and setting errno but this is the syscall version (has no access to errno) so the error is returned which is then set to errno in libc's wrapper.

This does cause problems as specifically musl libc version 1.2.0 when trying to search through the libary paths will specifically check for ENOENT or something of the sort but will fail outright if it encounters EPERM aka -1.

If you see the mentioned issues i believe for the same reason they are having the same problem.

In the past open has been correct which i why i say regression, one of my emulations has failed from 1.4.5 to 1.4.6 due to this.

[elicn]

Can you please point me to a resource I can look into it? (syscall vs. libc wrapped)
The linked issues are written in Chinese, which I don't understand, so I can't comment.

[iMoD1998]

Yep sure, hopefully musl source would suffice. This is not exclusive to just open, most libc funcs that set errno are done with return value from syscall.

So if you see the implmentation of open in musl:
https://git.musl-libc.org/cgit/musl/tree/src/fcntl/open.c

It calls __syscall_ret.

Which is defined as:
https://git.musl-libc.org/cgit/musl/tree/src/internal/syscall_ret.c

All syscall errors are negative, so they flip the sign and set it to errno.

Which is why in my proposed change i return -errno.

[iMoD1998]

You can also test this on your host machine by calling the syscall directly avoiding libc and see the return value. I can provide that too if you wish.

[iMoD1998]

I believe this is the commit that caused the regression:
0be4620#diff-8dd9355d1cdbad23bf5d2257c46c7fb8310a570a2f45302ce9cf54fed680fa63

[elicn]

@iMoD1998, I opened a PR with the necessary fixes.
For some reason it fails on Python 3.12, so if you are not using Python 3.12 you are welcome to pull these changes before they get merged.

[iMoD1998]

Thanks i appricate it.

[elicn]

Hi @iMoD1998
A fix has been committed to dev branch. I am tentatively closing the issue now.
Feel free to check out the updated code and re-open this issue if the problem persists.