Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Address some Coverity warnings #3394

Merged
merged 1 commit into from
Jan 15, 2025

Conversation

matt335672
Copy link
Member

@matt335672 matt335672 commented Jan 13, 2025

This PR addresses the following Coverity issues, which are not publically available:-

CID Description File
468106 Buffer overrun in libipm_msg_in_wait_available() libipm/libipm_recv.c
468113 Possible integer overflow in g_get_open_fds() common/os_calls.c
468121 Possible integer overflow in g_set_wait_obj() common/os_calls.c
468142 Possible string overrun in g_atoix() common/string_calls.c
468150 Possible memory leak in start_x_server() sesman/sesexec/session.c

468106

In summary, trans_get_wait_objs() can write a maximum of two values to the array (for TLS connections), and there's only space for one. In practice this can't happen. Simplest fix is to make the buffer big enough for two values.

468113

Coverity can't be sure that subtracting the int parameters min and max passed to the function won't result in integer overflow when max - min is calculated. Extra checks are added to the parameters and the call to sysconf(_SC_OPEN_MAX) to mitigate this. In practice, this won't happen, as this function is only used in testing.

468121

Coverity can't be sure that adding the result of a write() to an int won't cause integer overflow. In practice it won't but a simple extra check should make this clear to Coverity.

468142

Coverity is complaining that if "0" (which is two bytes long) is passed to this code as a pointed-to value by str :-

    if (*str == '0' && tolower(*(str + 1)) == 'x')
    {
        str += 2;
        base = 16;
    }

that str could be incremented beyond the end of the string. Coverity is unable to see that the second byte of "0" (i.e. a '\0' character) cannot pass the test tolower(<byte>) == 'x'. This is therefor a false-positive, and marked as such.

468150

A codepath in this function results in the passwd_file not being freed. This is a confirmed defect.

I can't currently run Coverity locally, so these are very much a best-shot at getting these addressed.

@matt335672
Copy link
Member Author

I'm going to merge this in the absence of any feedback, to see how the Coverity scan copes with this.

I'll then address a few more, and put a PR together for back-porting to v0.10.x

@matt335672 matt335672 merged commit 30b47ba into neutrinolabs:devel Jan 15, 2025
14 checks passed
@matt335672 matt335672 deleted the more_coverity_fixes branch January 15, 2025 09:50
@matt335672
Copy link
Member Author

This merge has broken the CI - namely the 32-bit builds have stopped working with the following error:-

The following packages have unmet dependencies:
 shim-signed : Depends: grub-efi-amd64-signed (>= 1.191~) but it is not going to be installed or
                        grub-efi-arm64-signed (>= 1.191~) but it is not installable or
                        base-files (< 12.3)
               Depends: grub-efi-amd64-signed (>= 1.187.2~) but it is not going to be installed or
                        grub-efi-arm64-signed (>= 1.187.2~) but it is not installable
E: Error, pkgProblemResolver::Resolve generated breaks, this may be caused by held packages.

It was working fine yesterday, so something has changed on the Github end.

This was referenced Jan 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant