RDKB-62985 RDKB-62986: Native build for Coverity#39
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR introduces a comprehensive native build system for Coverity static analysis of RDK-B components. The build system is designed to be generic and reusable across different RDK-B components.
Changes:
- Added a complete automated build pipeline with dependency management, header/library copying, and component building
- Introduced JSON-based configuration for defining dependencies, build settings, and patches
- Created GitHub Actions workflow for automated native builds on pull requests
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| cov_docker_script/setup_dependencies.sh | Main script for cloning and building dependency repositories |
| cov_docker_script/component_config.json | JSON configuration defining dependencies and build settings for advanced-security component |
| cov_docker_script/common_external_build.sh | Orchestration script that runs the complete 2-step build pipeline |
| cov_docker_script/common_build_utils.sh | Shared utility library with common functions for logging, building, and file operations |
| cov_docker_script/build_native.sh | Script for building the native component after dependencies are set up |
| cov_docker_script/README.md | Comprehensive documentation covering usage, configuration, and troubleshooting |
| .github/workflows/native-build.yml | GitHub Actions workflow for automated native builds |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,241 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
The shebang line has unnecessary leading whitespace. Shell scripts should start with the shebang on the first column without any leading spaces.
cov_docker_script/build_native.sh
Outdated
| # Expand variables safely | ||
| command=$(eval echo "$command") | ||
|
|
||
| log " [$((i+1))/$cmd_count] $description" | ||
| if eval "$command"; then |
There was a problem hiding this comment.
The command string is evaluated using eval with user-controlled input from the JSON configuration file after shell variable expansion. This creates a command injection vulnerability if the JSON file contains malicious commands. Consider using a safer approach that doesn't involve eval, or at minimum, validate and sanitize the command input.
| # Expand variables safely | |
| command=$(eval echo "$command") | |
| log " [$((i+1))/$cmd_count] $description" | |
| if eval "$command"; then | |
| # Expand environment variables without executing arbitrary code | |
| command=$(printf '%s' "$command" | envsubst) | |
| log " [$((i+1))/$cmd_count] $description" | |
| if bash -c "$command"; then |
| mkdir -p "$build_dir" | ||
|
|
||
| step "Running cmake" | ||
| if ! eval "cmake -S . -B $build_dir $cmake_flags"; then |
There was a problem hiding this comment.
Using eval with cmake_flags creates a command injection vulnerability. The flags from the JSON configuration file are directly evaluated as shell commands. Consider using safer argument passing methods.
| if ! eval "cmake -S . -B $build_dir $cmake_flags"; then | |
| if ! cmake -S . -B "$build_dir" $cmake_flags; then |
| pushd "$repo_dir" >/dev/null || return 1 | ||
|
|
||
| step "Running meson setup" | ||
| if ! eval "meson setup $build_dir $meson_flags"; then |
There was a problem hiding this comment.
Using eval with meson_flags creates a command injection vulnerability. Consider using safer methods to pass these flags.
| if ! eval "meson setup $build_dir $meson_flags"; then | |
| # Split meson_flags into an array and pass as separate arguments without eval | |
| local meson_flags_array=() | |
| if [[ -n "$meson_flags" ]]; then | |
| # shellcheck disable=SC2206 # intentional word splitting of flags | |
| meson_flags_array=($meson_flags) | |
| fi | |
| if ! meson setup "$build_dir" "${meson_flags_array[@]}"; then |
| local cmd | ||
| cmd=$(jq -r ".dependencies.repos[$index].build.commands[$i]" "$config_file") | ||
| step "Executing: $cmd" | ||
| if ! eval "$cmd"; then |
There was a problem hiding this comment.
Commands from the JSON configuration are executed using eval, allowing arbitrary command execution. This is a significant security vulnerability as the JSON file could contain malicious commands.
| if ! eval "$cmd"; then | |
| if ! bash -c "$cmd"; then |
.github/workflows/native-build.yml
Outdated
| chmod +x cov_docker_script/setup_dependencies.sh | ||
| ./cov_docker_script/setup_dependencies.sh | ||
| chmod +x cov_docker_script/build_native.sh |
There was a problem hiding this comment.
The workflow explicitly sets execute permissions on scripts that should already be executable in the repository. If the scripts aren't executable in the repository, they should be committed with the correct permissions. Alternatively, if this is intentional to ensure permissions, it would be clearer to do this for all scripts (common_external_build.sh is not made executable here but is mentioned in the README as the recommended entry point).
| chmod +x cov_docker_script/setup_dependencies.sh | |
| ./cov_docker_script/setup_dependencies.sh | |
| chmod +x cov_docker_script/build_native.sh | |
| ./cov_docker_script/setup_dependencies.sh |
.github/workflows/native-build.yml
Outdated
| chmod +x cov_docker_script/setup_dependencies.sh | ||
| ./cov_docker_script/setup_dependencies.sh | ||
| chmod +x cov_docker_script/build_native.sh | ||
| ./cov_docker_script/build_native.sh | ||
|
|
There was a problem hiding this comment.
The workflow runs the scripts individually rather than using common_external_build.sh which is documented in the README as the recommended entry point for complete builds. This inconsistency could lead to confusion and may result in different behavior between manual builds and CI builds.
| chmod +x cov_docker_script/setup_dependencies.sh | |
| ./cov_docker_script/setup_dependencies.sh | |
| chmod +x cov_docker_script/build_native.sh | |
| ./cov_docker_script/build_native.sh | |
| chmod +x common_external_build.sh | |
| ./common_external_build.sh |
|
|
||
| if [[ -d "$src" ]]; then | ||
| log "Copying headers: $src → $dst" | ||
| if ! find "$src" -maxdepth 1 -name "*.h" -exec cp {} "$dst/" \; 2>/dev/null; then |
There was a problem hiding this comment.
The find command uses -maxdepth 1 to copy only headers from the top level directory, but many projects organize headers in subdirectories. This means nested headers won't be copied, which could cause build failures if dependencies are in subdirectories. Consider using recursive copying or documenting this limitation.
| if ! find "$src" -maxdepth 1 -name "*.h" -exec cp {} "$dst/" \; 2>/dev/null; then | |
| if ! find "$src" -type f -name "*.h" -exec cp {} "$dst/" \; 2>/dev/null; then |
| echo -e "$content" > "$file" | ||
| if [[ $? -ne 0 ]]; then |
There was a problem hiding this comment.
Error code checking is inconsistent. The function returns 1 on error at line 114 but checks
| echo -e "$content" > "$file" | |
| if [[ $? -ne 0 ]]; then | |
| if ! echo -e "$content" > "$file"; then |
| "type": "autotools", | ||
| "configure_options": [ | ||
| "CPPFLAGS=-I$HOME/usr/include/rdkb/ -I/usr/include/ -I/usr/include/dbus-1.0 -I/usr/lib/x86_64-linux-gnu/dbus-1.0/include -I/usr/include/cjson -DSAFEC_DUMMY_API -DCONFIG_VENDOR_NAME", | ||
| "LDFLAGS=-L$HOME/usr/local/lib/ -Wl,--allow-shlib-undefined -Wl,--unresolved-symbols=ignore-all" |
There was a problem hiding this comment.
The linker flags use --allow-shlib-undefined and --unresolved-symbols=ignore-all which suppress link-time errors. While this might be necessary for Coverity analysis, it masks real linking problems and could lead to runtime failures. This is a potentially risky configuration that should be documented or reconsidered.
| "LDFLAGS=-L$HOME/usr/local/lib/ -Wl,--allow-shlib-undefined -Wl,--unresolved-symbols=ignore-all" | |
| "LDFLAGS=-L$HOME/usr/local/lib/" |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 17 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| copy_python_files_generic() { | ||
| local src_dir="${PYTHON_SRC_DIR:-$HOME/build}" | ||
| local dst_dir="${PYTHON_DST_DIR:-$HOME/usr/include/rdkb}" | ||
| if [[ -n "$src_dir" && -n "$dst_dir" ]]; then | ||
| log "[PYTHON COPY] Scanning for Python files in: $src_dir" | ||
| mkdir -p "$dst_dir" | ||
| local py_files | ||
| py_files=$(find "$src_dir" -type f -name "*.py") | ||
| local count=0 | ||
| if [[ -n "$py_files" ]]; then | ||
| log "[PYTHON COPY] Copying Python files to: $dst_dir" | ||
| while IFS= read -r file; do | ||
| cp "$file" "$dst_dir/" | ||
| count=$((count+1)) | ||
| done <<< "$py_files" | ||
| ok "[PYTHON COPY] $count Python file(s) copied to $dst_dir" | ||
| else | ||
| warn "[PYTHON COPY] No Python files found in $src_dir" | ||
| fi | ||
| else | ||
| warn "[PYTHON COPY] Source or destination directory not set. Skipping copy." | ||
| fi | ||
| } |
There was a problem hiding this comment.
The function copy_python_files_generic is never called from common_build_utils.sh or setup_dependencies.sh. It's only called from build_native.sh. Since this function is specific to the native build process and not a general utility, it would be more appropriate to move it to build_native.sh or document why it's in the common utilities despite only being used in one place.
cov_docker_script/build_native.sh
Outdated
| HEADER_PATH=$(expand_path "$(jq -r '.native_component.include_path' "$CONFIG_FILE")") | ||
| LIB_PATH=$(expand_path "$(jq -r '.native_component.lib_output_path' "$CONFIG_FILE")") | ||
|
|
There was a problem hiding this comment.
Missing validation for the return value of jq commands. If jq fails to parse the JSON or the expected field doesn't exist, the variable will be empty, which could lead to unexpected behavior or errors later in the script. Consider adding validation after jq commands to ensure the expected values are present.
| HEADER_PATH=$(expand_path "$(jq -r '.native_component.include_path' "$CONFIG_FILE")") | |
| LIB_PATH=$(expand_path "$(jq -r '.native_component.lib_output_path' "$CONFIG_FILE")") | |
| RAW_HEADER_PATH=$(jq -r '.native_component.include_path' "$CONFIG_FILE") | |
| RAW_LIB_PATH=$(jq -r '.native_component.lib_output_path' "$CONFIG_FILE") | |
| # Validate component configuration values | |
| if [[ -z "$COMPONENT_NAME" || "$COMPONENT_NAME" == "null" ]]; then | |
| err "Failed to read 'native_component.name' from config: $CONFIG_FILE" | |
| exit 1 | |
| fi | |
| if [[ -z "$BUILD_TYPE" || "$BUILD_TYPE" == "null" ]]; then | |
| err "Failed to read 'native_component.build.type' from config: $CONFIG_FILE" | |
| exit 1 | |
| fi | |
| if [[ -z "$RAW_HEADER_PATH" || "$RAW_HEADER_PATH" == "null" ]]; then | |
| err "Failed to read 'native_component.include_path' from config: $CONFIG_FILE" | |
| exit 1 | |
| fi | |
| if [[ -z "$RAW_LIB_PATH" || "$RAW_LIB_PATH" == "null" ]]; then | |
| err "Failed to read 'native_component.lib_output_path' from config: $CONFIG_FILE" | |
| exit 1 | |
| fi | |
| HEADER_PATH=$(expand_path "$RAW_HEADER_PATH") | |
| LIB_PATH=$(expand_path "$RAW_LIB_PATH") |
| }, | ||
| { | ||
| "file": "$HOME/usr/include/rdkb/ccsp_message_bus.h", | ||
| "search": "typedef struct _CCSP_MESSAGE_BUS_CONNECTION", | ||
| "replace": "typedef struct DBusLoop DBusLoop;\n\ntypedef struct _CCSP_MESSAGE_BUS_CONNECTION" |
There was a problem hiding this comment.
The source_patches array contains duplicate patches for the same change. Patches at indices 0 and 1 both attempt to add the same DBusLoop forward declaration to ccsp_message_bus.h, but target different file paths. The second patch targets a file in $HOME/usr/include/rdkb/ which suggests it's patching the already-copied header. This duplication is confusing and could lead to patch failures if the first patch succeeds but the second one doesn't find the expected content. Consider removing the duplicate patch or clarifying why both are needed.
| }, | |
| { | |
| "file": "$HOME/usr/include/rdkb/ccsp_message_bus.h", | |
| "search": "typedef struct _CCSP_MESSAGE_BUS_CONNECTION", | |
| "replace": "typedef struct DBusLoop DBusLoop;\n\ntypedef struct _CCSP_MESSAGE_BUS_CONNECTION" |
| "build": { | ||
| "type": "autotools", | ||
| "configure_options": [ | ||
| "CPPFLAGS=-I$HOME/usr/include/rdkb/ -I/usr/include/ -I/usr/include/dbus-1.0 -I/usr/lib/x86_64-linux-gnu/dbus-1.0/include -I/usr/include/cjson -DSAFEC_DUMMY_API -DCONFIG_VENDOR_NAME", |
There was a problem hiding this comment.
The configure options include -DSAFEC_DUMMY_API and -DCONFIG_VENDOR_NAME as preprocessor flags, but CONFIG_VENDOR_NAME is defined without a value. This will define it as 1 by default, which may not be the intended behavior. If a specific vendor name is required, it should be defined with a value like -DCONFIG_VENDOR_NAME='"SomeVendor"'. Otherwise, clarify if defining it without a value is intentional.
| "CPPFLAGS=-I$HOME/usr/include/rdkb/ -I/usr/include/ -I/usr/include/dbus-1.0 -I/usr/lib/x86_64-linux-gnu/dbus-1.0/include -I/usr/include/cjson -DSAFEC_DUMMY_API -DCONFIG_VENDOR_NAME", | |
| "CPPFLAGS=-I$HOME/usr/include/rdkb/ -I/usr/include/ -I/usr/include/dbus-1.0 -I/usr/lib/x86_64-linux-gnu/dbus-1.0/include -I/usr/include/cjson -DSAFEC_DUMMY_API -DCONFIG_VENDOR_NAME='\\\"VendorName\\\"'", |
| mkdir -p "$build_dir" | ||
|
|
||
| step "Running cmake" | ||
| if ! eval "cmake -S . -B $build_dir $cmake_flags"; then |
There was a problem hiding this comment.
Using eval to execute cmake flags is unsafe and can lead to command injection if the JSON configuration contains malicious input. An attacker who can modify the JSON could execute arbitrary commands. Consider using a safer approach such as properly quoting arguments or using an array with direct execution instead of eval.
| if ! eval "cmake -S . -B $build_dir $cmake_flags"; then | |
| # Build cmake argument list safely without using eval | |
| local cmake_args=() | |
| cmake_args+=("-S" ".") | |
| cmake_args+=("-B" "$build_dir") | |
| if [[ -n "$cmake_flags" ]]; then | |
| # Split cmake_flags on whitespace into separate arguments | |
| # shellcheck disable=SC2206 | |
| cmake_args+=($cmake_flags) | |
| fi | |
| if ! cmake "${cmake_args[@]}"; then |
cov_docker_script/build_native.sh
Outdated
| # Export configure options as environment variables | ||
| for option in "${configure_options[@]}"; do | ||
| export "$option" | ||
| done | ||
|
|
||
| if ! ./configure; then |
There was a problem hiding this comment.
The configure command is invoked without any arguments after the environment variable exports failed. Since the export loop doesn't properly set environment variables (see related issue), and no arguments are passed to configure, the build will not use the intended CPPFLAGS and LDFLAGS. Consider passing configure_options directly as arguments to the configure command.
| # Export configure options as environment variables | |
| for option in "${configure_options[@]}"; do | |
| export "$option" | |
| done | |
| if ! ./configure; then | |
| # Pass configure options directly as arguments to configure | |
| if ! ./configure "${configure_options[@]}"; then |
.github/workflows/native-build.yml
Outdated
| ./cov_docker_script/build_native.sh | ||
|
|
||
| env: | ||
| GITHUB_TOKEN: ${{ secrets.RDKCM_RDKE }} No newline at end of file |
There was a problem hiding this comment.
The GITHUB_TOKEN environment variable is set but never used in the workflow. If this token is needed for accessing private repositories during the build, it should be passed to the git clone operations. Otherwise, this secret exposure is unnecessary and should be removed to follow the principle of least privilege.
| ./cov_docker_script/build_native.sh | |
| env: | |
| GITHUB_TOKEN: ${{ secrets.RDKCM_RDKE }} | |
| ./cov_docker_script/build_native.sh |
cov_docker_script/build_native.sh
Outdated
|
|
||
| # Find and copy all library files (shared objects, static, libtool archives) | ||
| find "$COMPONENT_DIR" \( -name "*.so*" -o -name "*.a" -o -name "*.la*" \) \( -type f -o -type l \) -exec cp -Pv {} "$LIB_PATH/" \; 2>/dev/null || true | ||
|
|
There was a problem hiding this comment.
The install_libraries function searches the entire COMPONENT_DIR tree for library files, which could be inefficient for large repositories and may copy unintended libraries from subdirectories. Consider limiting the search to specific build output directories (e.g., .libs, build/lib) or making the search paths configurable in the JSON configuration.
| # Find and copy all library files (shared objects, static, libtool archives) | |
| find "$COMPONENT_DIR" \( -name "*.so*" -o -name "*.a" -o -name "*.la*" \) \( -type f -o -type l \) -exec cp -Pv {} "$LIB_PATH/" \; 2>/dev/null || true | |
| # Determine library search paths from config, with sensible defaults | |
| local library_search_paths search_path abs_path found_any | |
| library_search_paths=$(jq -r '.native_component.build.library_search_paths[]? // empty' "$CONFIG_FILE" 2>/dev/null || true) | |
| # If no search paths are configured, fall back to common build output directories | |
| if [[ -z "$library_search_paths" ]]; then | |
| library_search_paths=".libs | |
| build/lib" | |
| fi | |
| found_any=false | |
| for search_path in $library_search_paths; do | |
| # Resolve relative paths against COMPONENT_DIR | |
| if [[ "$search_path" = /* ]]; then | |
| abs_path="$search_path" | |
| else | |
| abs_path="$COMPONENT_DIR/$search_path" | |
| fi | |
| if [[ -d "$abs_path" ]]; then | |
| # Find and copy all library files (shared objects, static, libtool archives) | |
| find "$abs_path" \( -name "*.so*" -o -name "*.a" -o -name "*.la*" \) \( -type f -o -type l \) -exec cp -Pv {} "$LIB_PATH/" \; 2>/dev/null || true | |
| found_any=true | |
| fi | |
| done | |
| if [[ "$found_any" != true ]]; then | |
| log "No libraries found to install from configured search paths" | |
| fi |
| step "Running configure" | ||
| # Ensure PKG_CONFIG_PATH is set for configure | ||
| export PKG_CONFIG_PATH="${HOME}/usr/local/lib/pkgconfig:${HOME}/usr/lib/pkgconfig:${PKG_CONFIG_PATH:-}" | ||
| if ! eval "./configure $configure_flags"; then |
There was a problem hiding this comment.
Using eval to execute configure flags is unsafe and can lead to command injection if the JSON configuration contains malicious input. Since configure_flags comes directly from the JSON file, an attacker who can modify the JSON could execute arbitrary commands. Consider using a safer approach such as properly quoting arguments or using an array with direct execution instead of eval.
| if ! eval "./configure $configure_flags"; then | |
| # Safely pass configure flags as arguments without using eval | |
| local configure_args=() | |
| if [[ -n "$configure_flags" ]]; then | |
| # Split configure_flags into an array of arguments without executing them | |
| configure_args=($configure_flags) | |
| fi | |
| if ! ./configure "${configure_args[@]}"; then |
.github/workflows/native-build.yml
Outdated
| chmod +x cov_docker_script/setup_dependencies.sh | ||
| ./cov_docker_script/setup_dependencies.sh | ||
| chmod +x cov_docker_script/build_native.sh | ||
| ./cov_docker_script/build_native.sh |
There was a problem hiding this comment.
The chmod commands are redundant since bash scripts can be executed directly by passing them to bash, or the executable bit can be set in the repository. Additionally, setup_dependencies.sh and build_native.sh already have shebangs and should have executable permissions set in git. Consider either removing the chmod commands or setting the executable bit when committing the scripts to the repository.
| chmod +x cov_docker_script/setup_dependencies.sh | |
| ./cov_docker_script/setup_dependencies.sh | |
| chmod +x cov_docker_script/build_native.sh | |
| ./cov_docker_script/build_native.sh | |
| bash cov_docker_script/setup_dependencies.sh | |
| bash cov_docker_script/build_native.sh |
sowmiyachelliah
changed the title
sowmiyachelliah
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cov_docker_script/README.md
Outdated
| "name": "your-component", | ||
| "build": { | ||
| "type": "autotools", | ||
| "configure_options_file": "cov_docker_script/configure_options.conf" |
There was a problem hiding this comment.
The example native_component configuration in this README shows using a configure_options_file field to hook up configure_options.conf, but the actual component_config.json for this component only uses an inline configure_options array and never references configure_options.conf. This mismatch is likely to confuse maintainers about which flags are actually applied; please either wire configure_options.conf into component_config.json (if supported by the build tools) or adjust the README/example so it reflects the configuration format you intend to use here.
| "configure_options_file": "cov_docker_script/configure_options.conf" | |
| "configure_options": [ | |
| "--with-foo", | |
| "--enable-bar" | |
| ] |
| "configure_options": [ | ||
| "CPPFLAGS=-I$HOME/usr/include/rdkb/ -I/usr/include/ -I/usr/include/dbus-1.0 -I/usr/lib/x86_64-linux-gnu/dbus-1.0/include -I/usr/include/cjson -DSAFEC_DUMMY_API -DCONFIG_VENDOR_NAME", | ||
| "LDFLAGS=-L$HOME/usr/local/lib/ -Wl,--allow-shlib-undefined -Wl,--unresolved-symbols=ignore-all" | ||
| ] |
There was a problem hiding this comment.
This native_component.build.configure_options array inlines a minimal set of CPPFLAGS/LDFLAGS, but the repository also includes a much more complete configure_options.conf file that is not referenced anywhere in this JSON. Given the README’s example configuration uses a configure_options_file field to plug in configure_options.conf, it’s unclear whether this component is intentionally ignoring that file or whether it was omitted by mistake; please either reference configure_options.conf here (if that’s required by the build system) or remove/trim the unused config file to avoid confusion about which flags are in effect.
| "configure_options": [ | |
| "CPPFLAGS=-I$HOME/usr/include/rdkb/ -I/usr/include/ -I/usr/include/dbus-1.0 -I/usr/lib/x86_64-linux-gnu/dbus-1.0/include -I/usr/include/cjson -DSAFEC_DUMMY_API -DCONFIG_VENDOR_NAME", | |
| "LDFLAGS=-L$HOME/usr/local/lib/ -Wl,--allow-shlib-undefined -Wl,--unresolved-symbols=ignore-all" | |
| ] | |
| "configure_options_file": "configure_options.conf" |
cov_docker_script/README.md
Outdated
| - Invokes `common_external_build.sh` without arguments (dependencies manage their own configuration) | ||
| - Does **NOT** clean up `build_tools_workflows` (may be used by multiple dependencies) | ||
| - Typically called automatically during dependency setup, not manually | ||
|
|
||
| **What it does:** | ||
| 1. Clones `build_tools_workflows` if not present (or verifies it exists) | ||
| 2. Verifies `common_external_build.sh` is present | ||
| 3. Runs `common_external_build.sh` from build_tools_workflows |
There was a problem hiding this comment.
In the "Key Differences from run_native_build.sh" section, this bullet states that run_external_build.sh "invokes common_external_build.sh without arguments", but the actual script calls common_external_build.sh with both the component_config.json path and the native component directory as arguments. Please update the README to accurately describe the arguments passed by run_external_build.sh so the documentation matches the implemented behavior.
| - Invokes `common_external_build.sh` without arguments (dependencies manage their own configuration) | |
| - Does **NOT** clean up `build_tools_workflows` (may be used by multiple dependencies) | |
| - Typically called automatically during dependency setup, not manually | |
| **What it does:** | |
| 1. Clones `build_tools_workflows` if not present (or verifies it exists) | |
| 2. Verifies `common_external_build.sh` is present | |
| 3. Runs `common_external_build.sh` from build_tools_workflows | |
| - Invokes `common_external_build.sh` with the `component_config.json` path and the native component directory as arguments (dependencies manage their own configuration via these inputs) | |
| - Does **NOT** clean up `build_tools_workflows` (may be used by multiple dependencies) | |
| - Typically called automatically during dependency setup, not manually | |
| **What it does:** | |
| 1. Clones `build_tools_workflows` if not present (or verifies it exists) | |
| 2. Verifies `common_external_build.sh` is present | |
| 3. Runs `common_external_build.sh` from build_tools_workflows, passing the `component_config.json` path and the native component directory |
| name: Build advanced-security component in github rdkcentral | ||
| runs-on: ubuntu-latest | ||
| container: | ||
| image: ghcr.io/rdkcentral/docker-rdk-ci:latest | ||
|
|
||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v3 | ||
|
|
||
| - name: native build | ||
| run: | | ||
| chmod +x cov_docker_script/run_setup_dependencies.sh | ||
| ./cov_docker_script/run_setup_dependencies.sh | ||
| chmod +x cov_docker_script/run_native_build.sh | ||
| ./cov_docker_script/run_native_build.sh | ||
| env: | ||
| GITHUB_TOKEN: ${{ secrets.RDKCM_RDKE }} |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 12 days ago
To fix the problem, explicitly define minimal GITHUB_TOKEN permissions for this workflow or job, instead of relying on inherited repository/organization defaults. Since this workflow only checks out the repository and runs build scripts, it likely only needs read access to repository contents.
The best minimal fix without changing existing functionality is to add a permissions block at the job level for build-advanced-security-on-pr, specifying contents: read. This constrains the default GITHUB_TOKEN to read-only for repository contents, while leaving the custom GITHUB_TOKEN environment variable (secrets.RDKCM_RDKE) unchanged. Concretely, in .github/workflows/native-build.yml, under jobs:, inside the build-advanced-security-on-pr: job and at the same indentation level as runs-on, add:
permissions:
contents: readNo new methods, imports, or additional definitions are needed; this is a pure YAML configuration change.
| @@ -10,6 +10,8 @@ | ||
| build-advanced-security-on-pr: | ||
| name: Build advanced-security component in github rdkcentral | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: read | ||
| container: | ||
| image: ghcr.io/rdkcentral/docker-rdk-ci:latest | ||
|
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cov_docker_script/README.md
Outdated
| #### Alternative: Single-Script Build (All-in-One) | ||
|
|
||
| If you prefer to run everything in a single command: | ||
|
|
||
| ```bash | ||
| # Run setup dependencies + native build in one script | ||
| ./cov_docker_script/run_external_build.sh | ||
|
|
||
| # Clean build | ||
| CLEAN_BUILD=true ./cov_docker_script/run_external_build.sh | ||
| ``` | ||
|
|
||
| **Note:** `run_external_build.sh` performs both dependency setup and component build in one execution. While primarily designed to be invoked by the dependency setup process when specified in `component_config.json` (see [run_external_build.sh](#3-run_external_buildsh) section), it can also be used directly for the main component as a convenience script that handles the complete build pipeline. | ||
|
|
||
| #### Individual Steps | ||
|
|
||
| ```bash |
There was a problem hiding this comment.
The Quick Start section describes run_external_build.sh as a single-script pipeline that "performs both dependency setup and native build in one execution" and shows it being run directly, but the rest of this README (and the script header in run_external_build.sh) describe it as an external/dependency build wrapper that normally expects run_setup_dependencies.sh to be executed first. This conflicting guidance can confuse users about whether run_external_build.sh is meant as the main entry point for the component or only for dependency builds. Please clarify the intended usage and align the Quick Start example and notes with the actual behavior (and with how build_tools_workflows expects this wrapper to be used).
| #### Alternative: Single-Script Build (All-in-One) | |
| If you prefer to run everything in a single command: | |
| ```bash | |
| # Run setup dependencies + native build in one script | |
| ./cov_docker_script/run_external_build.sh | |
| # Clean build | |
| CLEAN_BUILD=true ./cov_docker_script/run_external_build.sh | |
| ``` | |
| **Note:** `run_external_build.sh` performs both dependency setup and component build in one execution. While primarily designed to be invoked by the dependency setup process when specified in `component_config.json` (see [run_external_build.sh](#3-run_external_buildsh) section), it can also be used directly for the main component as a convenience script that handles the complete build pipeline. | |
| #### Individual Steps | |
| ```bash | |
| #### Dependency Build Wrapper Usage (`run_external_build.sh`) | |
| `run_external_build.sh` is **not** the main entry point for building your component. It is a helper wrapper that is normally invoked by the `build_tools_workflows` tooling when building external dependency components listed in `component_config.json`. | |
| For typical usage (both local and CI), you only need to call: | |
| - `./cov_docker_script/run_setup_dependencies.sh` | |
| - `./cov_docker_script/run_native_build.sh` | |
| You generally do **not** need to run `run_external_build.sh` manually. If you do use it directly (for example, when debugging an external dependency build), run it from that dependency’s repository in the same way that `build_tools_workflows` would invoke it, and only after its required dependencies have been set up. | |
| #### Individual Steps | |
| ```bash | |
| ```bash |
| log "Cloning build_tools_workflows (develop)" | ||
| cd "$NATIVE_COMPONENT_DIR" | ||
| git clone -b develop "$BUILD_TOOLS_REPO_URL" || { err "Clone failed"; exit 1; } | ||
| ok "Repository cloned" |
There was a problem hiding this comment.
This script clones and uses build tooling from https://github.com/rdkcentral/build_tools_workflows pinned only to the mutable develop branch via git clone -b develop. Because the cloned scripts (such as setup_dependencies.sh) are executed as part of the build with full access to the workspace and potentially CI secrets, a compromise or forced-push of that branch could inject arbitrary code into your build artifacts. To reduce supply-chain risk, fetch this dependency via an immutable reference (for example, a specific commit SHA or signed release) and update it only through reviewed changes.
| # Clone build_tools_workflows if it doesn't exist | ||
| if [[ ! -d "$BUILD_TOOLS_DIR" ]]; then | ||
| log "build_tools_workflows not found, cloning repository..." | ||
| cd "$NATIVE_COMPONENT_DIR" | ||
| git clone -b develop "$BUILD_TOOLS_REPO_URL" || { err "Clone failed"; exit 1; } | ||
| ok "Repository cloned successfully" |
There was a problem hiding this comment.
This script clones and executes build tooling from https://github.com/rdkcentral/build_tools_workflows using git clone -b develop, which pins to a mutable branch rather than an immutable identifier. Since common_external_build.sh from that repository is then executed with full access to the build workspace, an attacker who compromises or force-pushes the develop branch could run arbitrary code in your build pipeline. To harden the supply chain, pin this dependency to a specific immutable reference (such as a commit SHA or signed release artifact) before executing its scripts.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| name: Build advanced-security component in github rdkcentral | ||
| runs-on: ubuntu-latest | ||
| container: | ||
| image: ghcr.io/rdkcentral/docker-rdk-ci:latest |
There was a problem hiding this comment.
Using the latest tag makes CI non-reproducible and can break builds when the image changes. Pin the container image to a specific version tag or immutable digest (e.g., @sha256:...) so Coverity/native builds are stable over time.
| image: ghcr.io/rdkcentral/docker-rdk-ci:latest | |
| image: ghcr.io/rdkcentral/docker-rdk-ci:1.5.0 |
|
|
||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v3 |
There was a problem hiding this comment.
actions/checkout@v3 is outdated. Update to actions/checkout@v4 (or the repository-standard pinned major) to pick up security and performance fixes.
| uses: actions/checkout@v3 | |
| uses: actions/checkout@v4 |
| chmod +x build_tools_workflows/cov_docker_script/build_native.sh | ||
| ./build_tools_workflows/cov_docker_script/build_native.sh ./cov_docker_script/component_config.json "$(pwd)" | ||
| env: | ||
| GITHUB_TOKEN: ${{ secrets.RDKCM_RDKE }} |
There was a problem hiding this comment.
Overriding GITHUB_TOKEN with a separate secret increases the risk of accidental token exposure (e.g., script logging) and can also break PR builds from forks where secrets aren’t available. Prefer the built-in GitHub token (${{ github.token }} / ${{ secrets.GITHUB_TOKEN }}) or pass a separate token under a different env var name with the minimum required permissions.
| GITHUB_TOKEN: ${{ secrets.RDKCM_RDKE }} | |
| GITHUB_TOKEN: ${{ github.token }} | |
| RDKCM_RDKE_TOKEN: ${{ secrets.RDKCM_RDKE }} |
| "_version": "2.0", | ||
| "_description": "Defines dependencies and build settings for the native component", | ||
|
|
||
| "dependencies": { |
There was a problem hiding this comment.
JSON indentation is inconsistent here (this key doesn’t align with surrounding fields). Reformatting the file with a consistent JSON formatter will make future edits and reviews less error-prone.
| "dependencies": { | |
| "dependencies": { |
| "header_paths": [ | ||
| { "source": "source/cosa/include", "destination": "$HOME/usr/include/rdkb" }, | ||
| { "source": "source/cosa/include/linux", "destination": "$HOME/usr/include/rdkb/linux" }, | ||
| { "source": "source/cosa/include/linux", "destination": "$HOME/usr/include/rdkb/" }, |
There was a problem hiding this comment.
The same source directory is copied to two different destinations, including the include root (.../rdkb/). This can unintentionally flatten/duplicate headers and change include resolution order. Keep only the canonical destination (likely .../rdkb/linux) or copy only specific headers into the root if that’s required.
| { "source": "source/cosa/include/linux", "destination": "$HOME/usr/include/rdkb/" }, |
| [LDFLAGS] | ||
| -L$HOME/usr/local/lib/ | ||
| -Wl,--allow-shlib-undefined | ||
| -Wl,--unresolved-symbols=ignore-all |
There was a problem hiding this comment.
These linker flags can mask real link errors (--unresolved-symbols=ignore-all) and may produce incomplete/incorrect binaries, which can also reduce the value of Coverity results. If these are only needed for Coverity capture, consider scoping them to a Coverity-only mode or adding a clear comment explaining why they’re required and what risks they introduce; otherwise, prefer failing fast on unresolved symbols.
| -Wl,--unresolved-symbols=ignore-all |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: native build | ||
| run: | | ||
| # Trust the workspace | ||
| git config --global --add safe.directory '*' |
There was a problem hiding this comment.
Setting safe.directory to '*' disables git’s ownership safety checks globally for all repos in the container, which is broader than needed. Prefer scoping this to the checked-out workspace only (e.g., add the specific working directory) so other directories remain protected.
| git config --global --add safe.directory '*' | |
| git config --global --add safe.directory "$GITHUB_WORKSPACE" |
|
|
||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v3 |
There was a problem hiding this comment.
actions/checkout@v3 is outdated and misses fixes/features added in newer major versions. Bump to the current supported major version (and consider pinning to a commit SHA for supply-chain hardening).
| uses: actions/checkout@v3 | |
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 |
| -DXDNS_ENABLE | ||
|
|
||
| # MoCA features | ||
| -DMOCA_DIAGONISTIC |
There was a problem hiding this comment.
Correct spelling of MOCA_DIAGONISTIC to MOCA_DIAGNOSTIC (assuming this is intended to mean 'diagnostic'). If the codebase uses the misspelled macro, consider supporting both temporarily for compatibility.
| -DMOCA_DIAGONISTIC | |
| -DMOCA_DIAGNOSTIC | |
| -DMOCA_DIAGONISTIC=MOCA_DIAGNOSTIC |
| [LDFLAGS] | ||
| -L$HOME/usr/local/lib/ | ||
| -Wl,--allow-shlib-undefined | ||
| -Wl,--unresolved-symbols=ignore-all |
There was a problem hiding this comment.
These linker flags can mask real link errors by allowing/ignoring unresolved symbols. If the goal is only to get a native build far enough for analysis, consider restricting this to specific known problematic libs or making it conditional (e.g., only for Coverity runs), so normal native builds still fail fast on genuine link issues.
| -Wl,--unresolved-symbols=ignore-all | |
| # NOTE: Avoid masking unresolved symbols; keep this disabled so real link errors are reported. | |
| # -Wl,--unresolved-symbols=ignore-all |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # MoCA-related | ||
| -DCONFIG_SYSTEM_MOCA | ||
| -DMOCA_HOME_ISOLATION | ||
| -DMOCA_DIAGONISTIC |
There was a problem hiding this comment.
Possible typo in macro name: MOCA_DIAGONISTIC is likely intended to be MOCA_DIAGNOSTIC. If the code checks the correctly spelled macro, this flag won’t enable the intended behavior.
| -DMOCA_DIAGONISTIC | |
| -DMOCA_DIAGNOSTIC |
| -L$HOME/usr/local/lib/ | ||
| -Wl,--allow-shlib-undefined | ||
| -Wl,--unresolved-symbols=ignore-all No newline at end of file |
There was a problem hiding this comment.
These linker flags suppress unresolved-symbol failures, which can hide real link problems and reduce the value of CI (and potentially Coverity) by allowing incomplete binaries. Prefer failing the build on unresolved symbols; if suppression is required for specific optional libs, scope it narrowly (e.g., only for affected targets) rather than globally in LDFLAGS.
| -L$HOME/usr/local/lib/ | |
| -Wl,--allow-shlib-undefined | |
| -Wl,--unresolved-symbols=ignore-all | |
| -L$HOME/usr/local/lib/ |
| "source_patches": [ | ||
| { | ||
| "file": "source/ccsp/include/ccsp_message_bus.h", | ||
| "search": "typedef struct _CCSP_MESSAGE_BUS_CONNECTION", | ||
| "replace": "typedef struct DBusLoop DBusLoop;\n\ntypedef struct _CCSP_MESSAGE_BUS_CONNECTION" | ||
| }, | ||
| { | ||
| "file": "$HOME/usr/include/rdkb/ccsp_message_bus.h", | ||
| "search": "typedef struct _CCSP_MESSAGE_BUS_CONNECTION", | ||
| "replace": "typedef struct DBusLoop DBusLoop;\n\ntypedef struct _CCSP_MESSAGE_BUS_CONNECTION" |
There was a problem hiding this comment.
Do we require this source patch field ? could you please check how moca-agent and utopia is used ?
There was a problem hiding this comment.
Is this configuration taken from do_compile log ?
3 participants
Reason for change: Enable coverity scan using native build.
Test Procedure: All the checks should pass in github
Risks: Low
Priority: P1
Signed-off-by: sowmiya_chelliah@comcast.com