Add CodeQL Workflow for Code Security Analysis

[b4yuan]

Summary

This pull request introduces a CodeQL workflow to enhance the security analysis of this repository.

What is CodeQL

CodeQL is a static analysis tool that helps identify and mitigate security vulnerabilities. It is primarily intra-function but does provide some support for inter-function analysis. By integrating CodeQL into a GitHub Actions workflow, it can proactively identify and address potential issues before they become security threats.

For more information on CodeQL and how to interpret its results, refer to the GitHub documentation and the CodeQL documentation (https://codeql.github.com/ and https://codeql.github.com/docs/).

What this PR does

We added a new CodeQL workflow file (.github/workflows/codeql.yml) that

• Runs on every pull request (functionality to run on every push to main branches is included as a comment for convenience).
• Runs daily.
• Excludes queries with a high false positive rate or low-severity findings.
• Does not display results for git submodules, focusing only on our own codebase.

Validation

To validate the functionality of this workflow, we have run several test scans on the codebase and reviewed the results. The workflow successfully compiles the project, identifies issues, and provides actionable insights while reducing noise by excluding certain queries and third-party code.

Using the workflow results

If this pull request is merged, the CodeQL workflow will be automatically run on every push to the main branch and on every pull request to the main branch. To view the results of these code scans, follow these steps:

1. Under the repository name, click on the Security tab.
2. In the left sidebar, click Code scanning alerts.

Is this a good idea?

We are researchers at Purdue University in the USA. We are studying the potential benefits and costs of using CodeQL on open-source repositories of embedded software.

We wrote up a report of our findings so far. The TL;DR is “CodeQL outperforms the other freely-available static analysis tools, with fairly low false positive rates and lots of real defects”. You can read about the report here: https://arxiv.org/abs/2310.00205

Review of engineering hazards

License: see the license at https://github.com/github/codeql-cli-binaries/blob/main/LICENSE.md:

Here's what you may also do with the Software, but only with an Open Source Codebase and subject to the License Restrictions provisions below:

Perform analysis on the Open Source Codebase.

If the Open Source Codebase is hosted and maintained on GitHub.com, generate CodeQL databases for or during automated analysis, CI, or CD.

False positives: We find that around 20% of errors are false positives, but that these FPs are polarized and only a few rules contribute to most FPs. We find that the top rules contributing to FPs are: cpp/uninitialized-local, cpp/missing-check-scanf, cpp/suspicious-pointer-scaling, cpp/unbounded-write, cpp/constant-comparison, and cpp/inconsistent-null-check. Adding a filter to filter out certain rules that contribute to a high FP rate can be done simply in the workflow file.

[github-advanced-security]

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

[meyraud705]

First you can actually download the analysis result here: https://github.com/IVOES/SDL/actions/runs/7866065015 (artifact at the bottom of the page), link from bot is a 404 error.

I took a look at the analysis, here what I think:

• SDL can run on a multitude of platform. How does codeQL handle multiple build config? With clang-tidy you need to do 1 run per build config.

• SDL has its own libc functions (scanf -> SDL_scanf). There are macros in SDL for clang-tidy: https://github.com/libsdl-org/SDL/blob/main/include/SDL3/SDL_stdinc.h#L727. How to tell codeQL about these functions?

• How to tell codeQL to ignore a line of code. With clang-tidy we can use /* NOLINT(error-id) */.

• Some files are from external sources and should not be checked. There is a list of them in https://github.com/libsdl-org/SDL/blob/main/cmake/3rdparty.cmake.

Warnings found by codeQL:

⚠️ Theses file are indeed missing header guard (missing-header-guard):
src/SDL_properties_c.h
src/events/SDL_scancode_tables_c.h
src/hidapi/SDL_hidapi_c.h
src/joystick/SDL_steam_virtual_gamepad.h
src/joystick/hidapi/SDL_hidapi_nintendo.h
src/joystick/hidapi/SDL_hidapi_rumble.h
src/locale/SDL_syslocale.h
src/misc/SDL_sysurl.h
src/video/SDL_blit_auto.h
src/video/offscreen/SDL_offscreenevents_c.h
src/video/offscreen/SDL_offscreenframebuffer_c.h
src/video/wayland/SDL_waylandmouse.h
src/video/x11/edid.h
src/video/yuv2rgb/yuv_rgb_internal.h

include-non-header
src/hidapi/SDL_hidapi_linux.h: Not a bug.

irregular-enum-init, trivial-switch, commented-out-code, fixme-comment and long-switch:
No bug here, just style and conventions.

local-variable-hides-global-variable
src/video/SDL_video.c:238: Global variable is declared after local variables, so I think it is fine.

empty-block and empty-if:
Comment would be nice in these.

unused-static-variable
src/video/khronos/vulkan/vulkan_core.h: From khronos header, should be ignored.

constant-comparison:
src/file/SDL_rwops.c:676: "comparison is always false": this warning is correct for 64 bit but not for 32 bit.
src/libm/*.c: external file that should be ignored.

equality-on-floats:
I think it checks if new value is different than previous one or avoid division by 0.

toctou-race-condition:
We are only checking if the file exist. stat() call could be removed.

stack-address-escape:
src/SDL_assert.c: Unless __func__ or __FUNCTION__ are stack variable, this is wrong.

src/audio/SDL_audio.c:
False positive: the condition pending.next is false if pending_tail point to a stack address.

src/video/SDL_blit.c:
False positive: address of src_surface is stored in src_surface->map->info.src_surface.

A stack trace would be nice here to see where the stack variable is declared. I tracked it myself:
SDL_surface.c:1585: SDL_ConvertPixelsAndColorspace() declare SDL_Surface src_surface; on the stack.
SDL_surface.c:1652: SDL_ConvertPixelsAndColorspace() pass address of src_surface to SDL_BlitSurfaceUnchecked();
SDL_surface.c:702: SDL_BlitSurfaceUnchecked() calls SDL_MapSurface();
SDL_Pixels.c:1487: SDL_MapSurface() calls SDL_CalculateBlit();
SDL_blit.c:216: address of stack varialbe is stored in map->info.src_surface

src/video/SDL_pixels.c:
False positive: Address of dst_surface is stored in src_surface->map->dst. dst_surface and src_surface have the same lifetime so I think it is fine.

inconsistent-null-check:
src/core/linux/SDL_ibus.c:
False positive: check is done in IBus_CheckConnection()

For all others I think value is never NULL (if initialisation fails, we do not process further).

uncontrolled-process-operation:
String from environment is passed to dlopen. I think that is a functionality we want.

uncontrolled-allocation-size:
⚠️ Overflow possible in SDL_UpdatedAudioDeviceFormat() because device->sample_frames may come from environment in GetDefaultSampleFramesFromFreq().

unbound-write:
src/filesystem/unix/SDL_filesystem.c: This is fine. strcpy and strcat should be replaced by their SDL version though.

[szsam]

SDL can run on a multitude of platform. How does codeQL handle multiple build config? With clang-tidy you need to do 1 run per build config.

CodeQL also requires 1 run per build config. We can modify the Workflow file to build the project for all platform supported and let CodeQL monitor the build process.

SDL has its own libc functions (scanf -> SDL_scanf). There are macros in SDL for clang-tidy: https://github.com/libsdl-org/SDL/blob/main/include/SDL3/SDL_stdinc.h#L727. How to tell codeQL about these functions?

We can use the same approach as clang-tidy. Define a macro like _CODEQL_SCANNING_ and modify SDL_stdinc.h

#if (defined(__clang_analyzer__) || defined(_CODEQL_SCANNING_)) && !defined(SDL_DISABLE_ANALYZE_MACROS)
...
#endif

How to tell codeQL to ignore a line of code. With clang-tidy we can use /* NOLINT(error-id) */.

Unfortunately, CodeQL currently is missing an inline mechanism to suppress warnings: github/codeql#11427.

Some files are from external sources and should not be checked. There is a list of them in https://github.com/libsdl-org/SDL/blob/main/cmake/3rdparty.cmake.

We can easily filter them out in the Workflow step Filter out unwanted errors and warnings.

[meyraud705]

Yes, please make these changes. I have no power to accept this PR, but i will look at the warnings and it has more chance to be accepted if something interesting is found.

[slouken]

This didn't really add value and caused a bunch of work verifying false positives. I think we'll decline this PR, thanks though!