-
Notifications
You must be signed in to change notification settings - Fork 35
native build for coverity enablement #163
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
base: develop
Are you sure you want to change the base?
Conversation
| name: Build utopia 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/setup_dependencies.sh | ||
| ./cov_docker_script/setup_dependencies.sh | ||
| chmod +x cov_docker_script/build_native.sh | ||
| ./cov_docker_script/build_native.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 11 days ago
To fix the problem, explicitly restrict the GITHUB_TOKEN permissions in this workflow to the minimum required. Since the job only checks out the repository and runs build scripts, read access to repository contents is sufficient; no write permissions or additional scopes are clearly needed from the snippet.
The best, least-intrusive fix is to add a permissions block at the job level for build-utopia-on-pr. This keeps the change local to this workflow and documents exactly what the job needs. Concretely, in .github/workflows/native-build.yml, under build-utopia-on-pr: (same indentation level as name: and runs-on:), add:
permissions:
contents: readNo new imports or external definitions are needed; this is purely a YAML configuration change inside the existing workflow file and does not alter the functional steps of the job.
-
Copy modified lines R13-R14
| @@ -10,6 +10,8 @@ | ||
| build-utopia-on-pr: | ||
| name: Build utopia 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces a comprehensive, generic build system for native builds with Coverity enablement. The system provides reusable scripts that can be adopted by any RDK-B component by simply customizing a JSON configuration file.
Key changes:
- Modular bash script framework with utility functions for building dependencies and native components
- JSON-driven configuration supporting multiple build systems (autotools, CMake, Meson, custom commands)
- Complete documentation with usage examples and troubleshooting guides
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
cov_docker_script/setup_dependencies.sh |
Orchestrates cloning, header copying, and building of external dependencies |
cov_docker_script/component_config.json |
Defines all dependencies, build settings, and patches in JSON format |
cov_docker_script/common_external_build.sh |
Main entry point that runs the complete 2-step build pipeline |
cov_docker_script/common_build_utils.sh |
Shared utility library with logging, path handling, and build functions |
cov_docker_script/build_native.sh |
Builds the native component after dependencies are set up |
cov_docker_script/README.md |
Comprehensive documentation covering usage, configuration, and troubleshooting |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| log "Copying headers: $src → $dst" | ||
| if ! find "$src" -maxdepth 1 -name "*.h" -exec cp {} "$dst/" \; 2>/dev/null; then | ||
| warn "No headers found in $src" | ||
| fi | ||
| else | ||
| warn "Header source not found: $src" |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The warning message about not finding headers may be misleading if the source path is intentionally empty or if headers are expected to be in subdirectories. Consider making this message more informative by indicating whether the source path exists as a directory but contains no headers, or if the path itself doesn't exist.
| log "Copying headers: $src → $dst" | |
| if ! find "$src" -maxdepth 1 -name "*.h" -exec cp {} "$dst/" \; 2>/dev/null; then | |
| warn "No headers found in $src" | |
| fi | |
| else | |
| warn "Header source not found: $src" | |
| # Collect header files directly under the source directory | |
| mapfile -t headers < <(find "$src" -maxdepth 1 -type f -name "*.h" 2>/dev/null) | |
| if ((${#headers[@]} == 0)); then | |
| # Check if headers exist only in subdirectories | |
| if find "$src" -mindepth 2 -type f -name "*.h" -print -quit 2>/dev/null | grep -q .; then | |
| warn "No headers copied from $src: header files (*.h) appear to be only in subdirectories (this function copies only top-level headers)." | |
| else | |
| warn "Header source directory exists but contains no .h files at the top level: $src" | |
| fi | |
| return 0 | |
| fi | |
| log "Copying headers: $src → $dst" | |
| if ! cp "${headers[@]}" "$dst/"; then | |
| warn "Failed to copy one or more headers from $src to $dst" | |
| fi | |
| else | |
| warn "Header source directory not found: $src" |
| "type": "autotools", | ||
| "configure_options": [ | ||
| "CPPFLAGS=-I$HOME/usr/include/rdkb -I/usr/include/tirpc -I/usr/include/dbus-1.0 -I/usr/lib/x86_64-linux-gnu/dbus-1.0/include -DSAFEC_DUMMY_API -D_GNU_SOURCE -Dget_current_wan_ifname=__get_current_wan_ifname_stub", | ||
| "CFLAGS=-include stdint.h -include errno.h -include rdk_logger.h -Wno-cpp", |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CFLAGS include -Wno-cpp which disables warnings from preprocessor directives. This can hide important warnings from #warning directives that might indicate configuration issues or deprecated functionality. Unless there's a specific reason to suppress these warnings, they should be enabled to catch potential issues during compilation.
| "CFLAGS=-include stdint.h -include errno.h -include rdk_logger.h -Wno-cpp", | |
| "CFLAGS=-include stdint.h -include errno.h -include rdk_logger.h", |
| mkdir -p "$build_dir" | ||
|
|
||
| step "Running cmake" | ||
| if ! eval "cmake -S . -B $build_dir $cmake_flags"; then |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of eval here poses a command injection risk if $cmake_flags contains malicious content from a compromised config file. Consider using safer alternatives such as passing the flags as an array or validating the input more carefully.
| if ! eval "cmake -S . -B $build_dir $cmake_flags"; then | |
| local cmake_args=() | |
| if [[ -n "$cmake_flags" ]]; then | |
| # Split cmake_flags into an array of arguments without using eval | |
| read -r -a cmake_args <<< "$cmake_flags" | |
| fi | |
| if ! cmake -S . -B "$build_dir" "${cmake_args[@]}"; then |
| local cmd | ||
| cmd=$(jq -r ".dependencies.repos[$index].build.commands[$i]" "$config_file") | ||
| step "Executing: $cmd" | ||
| if ! eval "$cmd"; then |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of eval here poses a command injection risk if commands from the config file contain malicious content. Since commands are read directly from the config file without validation, a compromised config could execute arbitrary commands. Consider validating or sanitizing the input, or using a safer command execution method.
| if ! eval "$cmd"; then | |
| # Split the command string into an array of arguments and execute | |
| local cmd_array=() | |
| read -r -a cmd_array <<< "$cmd" | |
| if ! "${cmd_array[@]}"; then |
| "configure_options": [ | ||
| "CPPFLAGS=-I$HOME/usr/include/rdkb -I/usr/include/tirpc -I/usr/include/dbus-1.0 -I/usr/lib/x86_64-linux-gnu/dbus-1.0/include -DSAFEC_DUMMY_API -D_GNU_SOURCE -Dget_current_wan_ifname=__get_current_wan_ifname_stub", | ||
| "CFLAGS=-include stdint.h -include errno.h -include rdk_logger.h -Wno-cpp", | ||
| "LDFLAGS=-L$HOME/usr/local/lib -L$HOME/usr/local/lib/x86_64-linux-gnu -Wl,-rpath,$HOME/usr/local/lib/x86_64-linux-gnu -Wl,--no-as-needed -Wl,--allow-shlib-undefined -Wl,--unresolved-symbols=ignore-all", |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The linker flags include --allow-shlib-undefined and --unresolved-symbols=ignore-all, which suppress important linker errors about undefined symbols. This can mask real issues and lead to runtime failures that would have been caught at link time. These flags should only be used if absolutely necessary and with clear documentation explaining why they are needed.
| "LDFLAGS=-L$HOME/usr/local/lib -L$HOME/usr/local/lib/x86_64-linux-gnu -Wl,-rpath,$HOME/usr/local/lib/x86_64-linux-gnu -Wl,--no-as-needed -Wl,--allow-shlib-undefined -Wl,--unresolved-symbols=ignore-all", | |
| "LDFLAGS=-L$HOME/usr/local/lib -L$HOME/usr/local/lib/x86_64-linux-gnu -Wl,-rpath,$HOME/usr/local/lib/x86_64-linux-gnu -Wl,--no-as-needed", |
| { | ||
| "name": "jsoncpp", | ||
| "repo": "https://github.com/open-source-parsers/jsoncpp.git", | ||
| "branch": "master", |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the master branch instead of a specific tag or commit SHA can lead to unreproducible builds and potential security issues. The master branch may receive updates that break compatibility or introduce vulnerabilities. Consider pinning to a specific version tag or commit SHA for reproducible and stable builds.
| "branch": "master", | |
| "branch": "1.9.5", |
| # 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 |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The find command uses || true to suppress all errors, which means that actual errors (like permission denied, filesystem issues, etc.) will be silently ignored. While this may be intentional to handle cases where no libraries are found, it makes debugging more difficult. Consider logging a message if the find command fails or if no files are copied.
| # 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 | |
| # Check if there are any library files to install | |
| if ! find "$COMPONENT_DIR" \( -name "*.so*" -o -name "*.a" -o -name "*.la*" \) \( -type f -o -type l \) -print -quit | grep -q .; then | |
| log "No libraries found in $COMPONENT_DIR to install into $LIB_PATH" | |
| echo "" | |
| return 0 | |
| fi | |
| # Find and copy all library files (shared objects, static, libtool archives) | |
| if ! find "$COMPONENT_DIR" \( -name "*.so*" -o -name "*.a" -o -name "*.la*" \) \( -type f -o -type l \) -exec cp -Pv {} "$LIB_PATH/" \; ; then | |
| log "Warning: errors occurred while installing libraries from $COMPONENT_DIR to $LIB_PATH" | |
| 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 |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of eval here can be dangerous if the $configure_flags variable contains malicious content. Since this comes from the config file, if the config file is compromised or contains unexpected special characters, it could lead to command injection. Consider using array-based command execution or validating the input more carefully.
| if ! eval "./configure $configure_flags"; then | |
| # Safely handle configure flags without using eval to avoid command injection | |
| local configure_args=() | |
| if [[ -n "$configure_flags" ]]; then | |
| # Basic validation: reject obvious shell control/meta characters | |
| if [[ "$configure_flags" =~ [\;\&\|\`\>\<\(\)] ]]; then | |
| err "Invalid characters found in configure flags" | |
| popd >/dev/null | |
| return 1 | |
| fi | |
| # Split flags into an array on whitespace | |
| read -r -a configure_args <<< "$configure_flags" | |
| fi | |
| if ! ./configure "${configure_args[@]}"; then |
| # Export configure options as environment variables | ||
| for option in "${configure_options[@]}"; do | ||
| export "$option" | ||
| done | ||
|
|
||
| if ! ./configure; then | ||
| err "Configure failed" | ||
| return 1 | ||
| fi | ||
| ok "Configure completed" | ||
| echo "" | ||
|
|
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function exports environment variables but doesn't unexport them when the function completes or if errors occur. The export statement on line 132 exports each configure option as an environment variable, which may pollute the environment or cause unexpected behavior in subsequent operations. Consider using local scope or ensuring cleanup of these exports.
| # Export configure options as environment variables | |
| for option in "${configure_options[@]}"; do | |
| export "$option" | |
| done | |
| if ! ./configure; then | |
| err "Configure failed" | |
| return 1 | |
| fi | |
| ok "Configure completed" | |
| echo "" | |
| # Export configure options as environment variables, track them for cleanup | |
| local exported_vars=() | |
| for option in "${configure_options[@]}"; do | |
| export "$option" | |
| # Extract variable name before '=' (if present) for later unsetting | |
| local var_name="${option%%=*}" | |
| exported_vars+=("$var_name") | |
| done | |
| if ! ./configure; then | |
| err "Configure failed" | |
| # Cleanup exported variables on error | |
| for var in "${exported_vars[@]}"; do | |
| unset "$var" | |
| done | |
| return 1 | |
| fi | |
| ok "Configure completed" | |
| echo "" | |
| # Cleanup exported variables after successful configure | |
| for var in "${exported_vars[@]}"; do | |
| unset "$var" | |
| done | |
| clone_repo() { | ||
| local name="$1" repo="$2" branch="$3" dest="$4" | ||
|
|
||
| if [[ -d "$dest" ]]; then | ||
| warn "$name already exists, skipping clone" | ||
| return 0 | ||
| fi | ||
|
|
||
| log "Cloning $name (branch: $branch)" | ||
| if ! git clone --branch "$branch" "$repo" "$dest" --depth 1; then | ||
| err "Failed to clone $name" | ||
| return 1 | ||
| fi | ||
| ok "$name cloned successfully" | ||
| return 0 | ||
| } |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clone_repo clones and then builds external dependency repositories by mutable branch reference (git clone --branch "$branch" "$repo" ...) without any integrity verification or pinning to an immutable revision. In CI or other automated environments, a compromised or hijacked dependency repo/branch can change code executed during autogen.sh/configure/make (and other build steps), leading to a full supply-chain compromise of the build and any accessible secrets. To reduce this risk, require external dependencies to be pinned to specific commit SHAs or signed tags and/or verify expected hashes/signatures before building, especially for third-party repositories.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| { | ||
| "name": "common-library", | ||
| "repo": "https://github.com/rdkcentral/common-library.git", | ||
| "branch": "feature/native_build_for_coverity", |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The branch "feature/native_build_for_coverity" appears to be a temporary feature branch. For production use, this should be changed to a stable branch like "main", "master", or a tagged version to ensure build reproducibility and avoid issues if the feature branch is deleted after merging.
| "branch": "feature/native_build_for_coverity", | |
| "branch": "main", |
| if ! python3 -c " | ||
| import sys | ||
| with open('$file', 'r') as f: | ||
| content = f.read() | ||
| content = content.replace('''$search''', '''$replace''') | ||
| with open('$file', 'w') as f: | ||
| f.write(content) | ||
| "; then |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Python script used for patching lacks proper error handling and escaping. Single quotes in the search or replace strings could break the Python code. Consider using a more robust approach such as writing the search and replace strings to temporary files and reading them in Python, or using proper escaping/quoting mechanisms.
| step "Running autogen.sh" | ||
| chmod +x autogen.sh | ||
| # Set NOCONFIGURE to prevent autogen.sh from automatically running configure | ||
| if ! NOCONFIGURE=1 ./autogen.sh; then |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The NOCONFIGURE environment variable is set but may not be respected by all autogen.sh scripts. After setting NOCONFIGURE=1, it should be explicitly unset or the script should verify whether autogen.sh actually skipped configure. Otherwise, configure might run with incorrect or missing environment variables.
| # Export configure options as environment variables | ||
| for option in "${configure_options[@]}"; do | ||
| export "$option" | ||
| done | ||
|
|
||
| if ! ./configure; then |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Configure options are being exported as environment variables directly, but this approach is problematic. If an option like "CPPFLAGS=-I/path" is exported, it creates an environment variable named "CPPFLAGS=-I/path" (including the value), not CPPFLAGS with value "-I/path". The options should be parsed to separate variable names from their values, or passed directly to configure instead.
| # Export configure options as environment variables | |
| for option in "${configure_options[@]}"; do | |
| export "$option" | |
| done | |
| if ! ./configure; then | |
| # Split configure options into environment assignments and configure arguments | |
| local env_options=() | |
| local configure_args=() | |
| for option in "${configure_options[@]}"; do | |
| if [[ "$option" == *=* ]]; then | |
| env_options+=("$option") | |
| else | |
| configure_args+=("$option") | |
| fi | |
| done | |
| # Run configure with the collected environment variables and arguments | |
| if ! env "${env_options[@]}" ./configure "${configure_args[@]}"; then |
| 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 |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using eval with user-controlled input from the JSON configuration file is a security risk. The configure_flags, cmake_flags, and commands are executed through eval without sanitization, which could lead to command injection if the JSON file is compromised or contains malicious content. Consider using safer alternatives or implementing input validation.
| if ! eval "./configure $configure_flags"; then | |
| # Safely split configure_flags into an array to avoid command injection via eval | |
| local cfg_args=() | |
| if [[ -n "$configure_flags" ]]; then | |
| read -r -a cfg_args <<< "$configure_flags" | |
| fi | |
| if ! ./configure "${cfg_args[@]}"; then |
| step "Running autogen.sh" | ||
| chmod +x ./autogen.sh | ||
| # Set NOCONFIGURE to prevent autogen.sh from automatically running configure | ||
| if ! NOCONFIGURE=1 ./autogen.sh; then |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The NOCONFIGURE environment variable is set but not unset after autogen.sh completes. This could affect subsequent operations if they check for this variable. Consider unsetting it after use or limiting its scope.
| chmod +x cov_docker_script/setup_dependencies.sh | ||
| ./cov_docker_script/setup_dependencies.sh | ||
| chmod +x cov_docker_script/build_native.sh |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The workflow unnecessarily uses chmod to make the scripts executable when they should already have the executable bit set in the repository. If the scripts are not executable in the repository, they should be committed with the correct permissions. Additionally, common_external_build.sh could be used instead to run the complete pipeline in a single command.
| 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 |
| mkdir -p "$build_dir" | ||
|
|
||
| step "Running cmake" | ||
| if ! eval "cmake -S . -B $build_dir $cmake_flags"; then |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using eval with user-controlled input from the JSON configuration file is a security risk. The cmake_flags are executed through eval without sanitization, which could lead to command injection if the JSON file is compromised or contains malicious content. Consider using safer alternatives or implementing input validation.
| if ! eval "cmake -S . -B $build_dir $cmake_flags"; then | |
| # Basic validation to reduce risk of command injection via cmake_flags | |
| if [[ "$cmake_flags" =~ [\;\&\|\`\$\>\<] ]]; then | |
| err "Invalid characters detected in cmake_flags" | |
| popd >/dev/null | |
| return 1 | |
| fi | |
| 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 |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using eval with user-controlled input from the JSON configuration file is a security risk. The meson_flags are executed through eval without sanitization, which could lead to command injection if the JSON file is compromised or contains malicious content. Consider using safer alternatives or implementing input validation.
| if ! eval "meson setup $build_dir $meson_flags"; then | |
| if ! meson setup "$build_dir" $meson_flags; then |
| "repo": "https://github.com/rdkcentral/rbus.git", | ||
| "branch": "v2.7.0", | ||
| "header_paths": [ | ||
| { "source": "include", "destination": "$HOME/usr/include/rdkb/rbus" }, |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The path expansion only handles $HOME variable, but paths in the JSON contain literal "$HOME" strings that might not expand correctly in all contexts. The expand_path function should be consistently applied to all paths read from the configuration file before use, or the JSON should use environment variable syntax that bash will expand naturally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }, | ||
| { | ||
| "name": "halinterface", | ||
| "repo": "https://code.rdkcentral.com/r/rdkb/components/opensource/ccsp/halinterface", |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The repository URL uses HTTP instead of HTTPS. This is a security risk as it makes the connection vulnerable to man-in-the-middle attacks. Use HTTPS for all repository URLs.
| log "Cloning $name (branch: $branch)" | ||
| if ! git clone --branch "$branch" "$repo" "$dest" --depth 1; then | ||
| err "Failed to clone $name" | ||
| return 1 |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The git clone uses --depth 1 for shallow cloning, which is efficient for CI/CD. However, if a specific commit or tag needs to be checked out (not just a branch), this will fail. Consider adding support for checking out specific commits by omitting --depth 1 when a commit hash is specified instead of a branch name.
| log "Cloning $name (branch: $branch)" | |
| if ! git clone --branch "$branch" "$repo" "$dest" --depth 1; then | |
| err "Failed to clone $name" | |
| return 1 | |
| # If a commit hash is provided instead of a branch, avoid shallow clone | |
| # so that the requested commit is guaranteed to be available locally. | |
| if [[ "$branch" =~ ^[0-9a-f]{7,40}$ ]]; then | |
| log "Cloning $name (commit: $branch)" | |
| if ! git clone "$repo" "$dest"; then | |
| err "Failed to clone $name" | |
| return 1 | |
| fi | |
| if ! git -C "$dest" checkout "$branch"; then | |
| err "Failed to check out commit $branch for $name" | |
| return 1 | |
| fi | |
| else | |
| log "Cloning $name (branch: $branch)" | |
| if ! git clone --branch "$branch" "$repo" "$dest" --depth 1; then | |
| err "Failed to clone $name" | |
| return 1 | |
| fi |
|
|
||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v3 |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The workflow uses 'actions/checkout@v3' which is an older version. Consider updating to 'actions/checkout@v4' for the latest features and security updates.
| uses: actions/checkout@v3 | |
| uses: actions/checkout@v4 |
| copy_libraries "$repo_dir" "$USR_DIR/local/lib" | ||
| copy_libraries "$repo_dir" "$USR_DIR/lib" |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Libraries are being copied to both locations indiscriminately. This could result in redundant copies and wasted disk space. Consider copying libraries to only one primary location, or add logic to check if the source directory contains libraries before attempting the copy operation.
| "configure_options": [ | ||
| "CPPFLAGS=-I$HOME/usr/include/rdkb -I/usr/include/tirpc -I/usr/include/dbus-1.0 -I/usr/lib/x86_64-linux-gnu/dbus-1.0/include -DSAFEC_DUMMY_API -D_GNU_SOURCE -Dget_current_wan_ifname=__get_current_wan_ifname_stub", | ||
| "CFLAGS=-include stdint.h -include errno.h -include rdk_logger.h -Wno-cpp", | ||
| "LDFLAGS=-L$HOME/usr/local/lib -L$HOME/usr/local/lib/x86_64-linux-gnu -Wl,-rpath,$HOME/usr/local/lib/x86_64-linux-gnu -Wl,--no-as-needed -Wl,--allow-shlib-undefined -Wl,--unresolved-symbols=ignore-all", |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The LDFLAGS include '--unresolved-symbols=ignore-all' which instructs the linker to ignore all unresolved symbols. This is dangerous as it will hide legitimate linking errors and can lead to runtime failures that are difficult to debug. Consider using '--allow-shlib-undefined' alone or being more selective about which symbols to ignore.
| "LDFLAGS=-L$HOME/usr/local/lib -L$HOME/usr/local/lib/x86_64-linux-gnu -Wl,-rpath,$HOME/usr/local/lib/x86_64-linux-gnu -Wl,--no-as-needed -Wl,--allow-shlib-undefined -Wl,--unresolved-symbols=ignore-all", | |
| "LDFLAGS=-L$HOME/usr/local/lib -L$HOME/usr/local/lib/x86_64-linux-gnu -Wl,-rpath,$HOME/usr/local/lib/x86_64-linux-gnu -Wl,--no-as-needed -Wl,--allow-shlib-undefined", |
| ./cov_docker_script/build_native.sh | ||
|
|
||
| env: | ||
| GITHUB_TOKEN: ${{ secrets.RDKCM_RDKE }} |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using secrets.RDKCM_RDKE as GITHUB_TOKEN is unusual. The standard GitHub Actions token is accessed via secrets.GITHUB_TOKEN. If RDKCM_RDKE is a custom token with specific permissions, it should be documented why it's needed. If it's meant to be the standard token, use secrets.GITHUB_TOKEN instead.
| GITHUB_TOKEN: ${{ secrets.RDKCM_RDKE }} | |
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} |
| fi | ||
|
|
||
| log "Cloning $name (branch: $branch)" | ||
| if ! git clone --branch "$branch" "$repo" "$dest" --depth 1; then |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The clone_repo helper clones dependencies from the repo/branch values in component_config.json without pinning them to immutable identifiers or performing any integrity verification. For external third-party repositories this means every build pulls a mutable branch tip, so a compromised or hijacked upstream repo can inject arbitrary code that will be built and executed in your CI environment. To reduce supply-chain risk, require dependencies to be referenced by immutable identifiers (e.g., commit SHA or verified tag) and/or enforce checksum or signature verification for fetched sources, especially when builds have access to secrets or production artifacts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| check_dependencies() { | ||
| local required_tools=("git" "jq" "gcc" "make") | ||
| local missing=() | ||
|
|
||
| for tool in "${required_tools[@]}"; do | ||
| if ! command -v "$tool" &> /dev/null; then | ||
| missing+=("$tool") | ||
| fi | ||
| done | ||
|
|
||
| if [ ${#missing[@]} -gt 0 ]; then | ||
| err "Missing required tools: ${missing[*]}" | ||
| err "Please install them before continuing" | ||
| return 1 | ||
| fi | ||
| return 0 | ||
| } |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function name 'check_dependencies' is ambiguous in the context of a dependency setup script. It actually checks for required system tools (git, jq, gcc, make), not project dependencies. Consider renaming to 'check_required_tools' or 'validate_system_tools' for clarity.
| export PARENT_BUILD_DIR="$BUILD_DIR" | ||
| export PARENT_USR_DIR="$USR_DIR" | ||
|
|
||
| pushd "$repo_dir" >/dev/null || return 1 | ||
| if ! "$full_script"; then | ||
| err "Build script failed" | ||
| popd >/dev/null | ||
| return 1 | ||
| fi | ||
| popd >/dev/null | ||
|
|
||
| unset PARENT_BUILD_DIR PARENT_USR_DIR |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The export statements for PARENT_BUILD_DIR and PARENT_USR_DIR are only set when build type is 'script', but they could be useful for all build types. Additionally, there's an unset at the end which only applies to the script case. Consider whether these variables should be available more broadly or document why they're script-specific.
| local src dst | ||
| src=$(jq -r ".dependencies.repos[$index].header_paths[$i].source" "$CONFIG_FILE") | ||
| dst=$(jq -r ".dependencies.repos[$index].header_paths[$i].destination" "$CONFIG_FILE") | ||
|
|
||
| copy_headers "$repo_dir/$src" "$dst" |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable USR_DIR is referenced but may not be properly expanded in the JSON string. When paths like "$HOME/usr/include/rdkb/rbus" are read from JSON, the $HOME variable is in the JSON string itself, not a shell variable. This could lead to inconsistent behavior depending on whether expand_path is called on these destination paths.
| local src dst | |
| src=$(jq -r ".dependencies.repos[$index].header_paths[$i].source" "$CONFIG_FILE") | |
| dst=$(jq -r ".dependencies.repos[$index].header_paths[$i].destination" "$CONFIG_FILE") | |
| copy_headers "$repo_dir/$src" "$dst" | |
| local src dst dst_expanded | |
| src=$(jq -r ".dependencies.repos[$index].header_paths[$i].source" "$CONFIG_FILE") | |
| dst=$(jq -r ".dependencies.repos[$index].header_paths[$i].destination" "$CONFIG_FILE") | |
| # Expand common environment variables that may appear in JSON paths | |
| dst_expanded="$dst" | |
| dst_expanded="${dst_expanded//\$\{HOME\}/$HOME}" | |
| dst_expanded="${dst_expanded//\$HOME/$HOME}" | |
| dst_expanded="${dst_expanded//\$\{USR_DIR\}/$USR_DIR}" | |
| dst_expanded="${dst_expanded//\$USR_DIR/$USR_DIR}" | |
| copy_headers "$repo_dir/$src" "$dst_expanded" |
| # Export configure options as environment variables | ||
| for option in "${configure_options[@]}"; do | ||
| export "$option" | ||
| done | ||
|
|
||
| if ! ./configure; then |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The configure script is being exported as an environment variable with the export statement, but configure options like "CPPFLAGS=-I..." are being treated as variable assignments. This approach may not work correctly because ./configure expects these as arguments or environment variables set before the command, not exported. Consider setting them before the configure command without export, or passing them as arguments to configure.
| # Export configure options as environment variables | |
| for option in "${configure_options[@]}"; do | |
| export "$option" | |
| done | |
| if ! ./configure; then | |
| # Split configure options into environment assignments and CLI arguments | |
| local config_env=() | |
| local config_args=() | |
| for option in "${configure_options[@]}"; do | |
| # Treat VAR=value (that does not start with "--") as an env assignment; everything else as an argument | |
| if [[ "$option" == *=* && "$option" != --* ]]; then | |
| config_env+=("$option") | |
| else | |
| config_args+=("$option") | |
| fi | |
| done | |
| # Run configure with the computed environment and arguments | |
| if ! env "${config_env[@]}" ./configure "${config_args[@]}"; then |
| local cmd | ||
| cmd=$(jq -r ".dependencies.repos[$index].build.commands[$i]" "$config_file") | ||
| step "Executing: $cmd" | ||
| if ! eval "$cmd"; then |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using eval with user-controlled commands from the JSON config file poses a security risk. If a command contains malicious code, it will be executed. Consider validating the commands or using a safer execution method.
| [[ -d "$BUILD_DIR" ]] && rm -rf "$BUILD_DIR" | ||
| [[ -d "$USR_DIR" ]] && rm -rf "$USR_DIR" |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent quoting in the rm commands. The BUILD_DIR and USR_DIR variables should be quoted to prevent word splitting and glob expansion in case the paths contain spaces or special characters.
No description provided.