Add coverity push script #165

paul-elliott-arm · 2024-04-25T09:53:37Z

Description

This is the script to allow us to push coverity builds from Jenkins (or anyone's machine come to that)

Functionality is documented within the script, but to test:

either:

export COVERITY_TOKEN=token
path/to/mbedtls/scripts/push_coverity_scan.py path/to/mbedtls -b origin/coverity_scan -e [email protected]

or

path/to/mbedtls/scripts/push_coverity_scan.py path/to/mbedtls -t token -b origin/coverity_scan -e [email protected]

The token can be found from the coverity web interface, and for the email address I would use your own email address for now (email address is unfortunately compulsary). Conversations are ongoing about which address to use for notifications in the future, the notification email will look like this:

 Your request for analysis of ARMmbed/mbedtls has been completed successfully.
    The results are available at <url>

    Build ID: 607763

    Analysis Summary:
       New defects found: 0
       Defects eliminated: 0

Should new defects be found, another email will be generated, but this will go to the whole team (this is specified within coverity itself)

WARNING - the mbedtls directory specified will be switched to the latest version of the branch specified, any outstanding differences will currently cause the script to fail (in my opinion better than forcing anything).

This constitutes stage 1 of getting coverity push back into CI, getting this script reviewed so that we can at least check its in the correct shape, then stage 2 will be adding the groovy and testing in the repo. I currently envisage this to be added shortly after the existing push from development to coverity_scan branches in the nightly runs.

Signed-off-by: Paul Elliott <[email protected]>

paul-elliott-arm · 2024-04-25T09:54:48Z

One possible improvment (although not yet required) would be to parameterise the project so that we can easily use this on other projects, however I'm not sure how many coverity scan projects we are allowed to have!

gilles-peskine-arm

I've read the code, but not tried to run it (I'll do that when I re-review). This looks good from a high-level view, but there are several places where I found the code hard to follow. Some places seem to reinvent standard library functions, and some places have difficult-to-understand error handling that looks like it could be simpler. Generally, in Python, prefer context handlers (with …) if possible.

coverity/push_coverity_scan.py

Signed-off-by: Paul Elliott <[email protected]>

Mistaken usage of hashlib.md5(), much simpler form is available. Signed-off-by: Paul Elliott <[email protected]>

-j without a number means unlimited, and is likely a bad idea on limited systems. Signed-off-by: Paul Elliott <[email protected]>

(Rather than empty string) Signed-off-by: Paul Elliott <[email protected]>

Signed-off-by: Paul Elliott <[email protected]>

Refactor previous function as now too small to really be required (error handling now done by exceptions thrown by run(), and only a single API call now required) Signed-off-by: Paul Elliott <[email protected]>

Signed-off-by: Paul Elliott <[email protected]>

Also make branch change recurse into submodules, as we now have one. Signed-off-by: Paul Elliott <[email protected]>

Use 'with' context, copy to backup file if required. Signed-off-by: Paul Elliott <[email protected]>

The addition of the framework necessitates this. Signed-off-by: Paul Elliott <[email protected]>

Missed as logic causes the script to just redownload the tools. Signed-off-by: Paul Elliott <[email protected]>

Instead of erroring out if the specified backup directory is non-existent, just create it. Signed-off-by: Paul Elliott <[email protected]>

Signed-off-by: Paul Elliott <[email protected]>

Useful for debug more than anything else. Signed-off-by: Paul Elliott <[email protected]>

.. and update documentation to add new options. Signed-off-by: Paul Elliott <[email protected]>

If config.h was changed upstream, it would be backed up prior to these changes being applied locally, and then restored into the wrong state. Signed-off-by: Paul Elliott <[email protected]>

gilles-peskine-arm

Thanks for the updates! On code inspection, I believe I now understand what the code is doing, and would be able to maintain it going forward.

I haven't tried running the script yet (and there's a typo that looks like it would fail midway).

I wonder how to test it: how far can we go before consuming a limited Coverity credit: is that consumed by the build request? By triggering? Could we have a test mode in the script that does everything except the credit-consuming step, and leaves behind or prints out all necessary information to do the credit-consuming step manually?

coverity/push_coverity_scan.py

gilles-peskine-arm · 2024-11-29T18:10:43Z

coverity/push_coverity_scan.py

+The token can either be passed in as an argument with -t, or via the environment
+in the variable 'COVERITY_TOKEN'
+
+Other options:


[non-blocker] It might be better to leave out the list of options here. They're already documented through the argparse calls and I fear that we'll forget to update the text here.

case in point...

gilles-peskine-arm · 2024-11-29T18:15:40Z

coverity/push_coverity_scan.py

+    coverity.
+    """

    os.chdir(mbedtls_dir)


[non-blocker] Because of the chdir call here, I'm not sure how the each command line arguments will be interpreted if they're relative paths, or if they even can be relative paths. It would be more robust to not call chdir and pass cwd=mbedtls_dir to subprocess.run calls.

I'm really unsure on this one, the scripts themselves generally assume being run from the root MBedTLS dir, so this is how I made it, I haven't noticed any issues up till now.

Moving the backup of config files fixed the previous issue, but created another one. Signed-off-by: Paul Elliott <[email protected]>

Signed-off-by: Paul Elliott <[email protected]>

Enable testing without consuming a coverity credit Signed-off-by: Paul Elliott <[email protected]>

paul-elliott-arm · 2024-12-06T15:48:39Z

I wonder how to test it: how far can we go before consuming a limited Coverity credit: is that consumed by the build request? By triggering? Could we have a test mode in the script that does everything except the credit-consuming step, and leaves behind or prints out all necessary information to do the credit-consuming step manually?

I have added -n/--no-upload which will cover this case. Should you use a backup directory for the generated tar file (rather than a temp file) this tar file can then be manually uploaded later, or you can just use this to test the tools download / build steps.

gilles-peskine-arm

I ran the script locally with --no-upload and it worked, producing a plausible-looking mbedtls-24-12-06.tar.gz in the --backupdir directory. So this looks good to me both on code reading and on execution apart from possibly the upload step. I'd still like to check that the upload step works for me while you're still partly around, but not today, and in the meantime I can approve this PR already, we can take it from there.

With the addition of tf-psa-crypto as a submodule, which itself has framework as a submodule, git submodule calls now need to be recursive Signed-off-by: Paul Elliott <[email protected]>

Required some re-work to have two files. May not entirely be the optimal way of doing this, but probably only worth more work if more files are required. Signed-off-by: Paul Elliott <[email protected]>

Update pre-build step due to the removal of check-generated-files.sh. Signed-off-by: Ronald Cron <[email protected]>

gilles-peskine-arm

LGTM apart from documentation that's slightly out of date.

I successfully submitted a build with

coverity/push_coverity_scan.py --backupdir ~/OUTGOING/coverity --branch dev --covtools ~/INCOMING/covtools --email … --token $(…) -v ~/z/dev/mbedtls/main

I'd like a few minor usability improvements, but I'd prefer to merge this first and then make a new pull request.

gilles-peskine-arm · 2025-04-07T17:19:56Z

coverity/push_coverity_scan.py

+* -o / --os: If specified, override the OS specification for the coverity tools
+    default is 64 bit linux
+* -p / --pre-build-step: Specify the command to run pre - build.
+    defaults to 'make clean'.


Suggested change

defaults to 'make clean'.

defaults to 'make neat'.

gilles-peskine-arm · 2025-04-07T17:20:15Z

coverity/push_coverity_scan.py

+The token can either be passed in as an argument with -t, or via the environment
+in the variable 'COVERITY_TOKEN'
+
+Other options:


case in point...

gilles-peskine-arm · 2025-04-07T17:55:47Z

coverity/push_coverity_scan.py

+    # TODO - Allow passing in version, description?
+    tar_file_path = pathlib.Path(tar_file_name)
+
+    build_post_data = [('version', ''),


Non-blocker: I'd like to record the git commit here.

gilles-peskine-arm · 2025-04-07T17:58:19Z

coverity/push_coverity_scan.py

+
+    os.chdir(mbedtls_dir)
+
+    # Ensure that given git directory is up to date.


Non-blocker: with --branch, we make sure that the given branch is checked out, and we make sure that it's the latest official version of that branch. But we don't make sure that there are no locally modified files. Should this script allow locally modified files?

gilles-peskine-arm · 2025-04-07T18:06:58Z

coverity/push_coverity_scan.py

+                    backup_path.mkdir()
+
+                backup_path = backup_path.resolve()
+                backup_path = backup_path / datetime.today().strftime('mbedtls-%y-%m-%d.tar.gz')


Non-blocker: a 2-digit year??

Suggested change

backup_path = backup_path / datetime.today().strftime('mbedtls-%y-%m-%d.tar.gz')

backup_path = backup_path / datetime.today().strftime('mbedtls-%Y-%m-%d.tar.gz')

gilles-peskine-arm · 2025-08-08T10:51:11Z

coverity/push_coverity_scan.py

+Required:
+
+1. Path to Mbed TLS directory
+2. A project coverity token (got from the Coverity site / Project Settings


Apparently you need to have “Maintainer/Owner” permission, not just “Contributor/Member”.

felixc-arm · 2025-08-08T12:36:44Z

Note: filter='data' on line 163 (tar_file.extractall(path=tools_dir, members=filter_root_tar_dir(tar_file), filter='data')) caused the following error when I was testing locally with python 3.10.12 on Ubuntu 22.04, removing it fixed the error.

  File "/home/felcon01/mbedtls/coverity/mbedtls-test/coverity/push_coverity_scan.py", line 381, in main
    download_coverity_scan_tools(logger, coverity_token, args.os, tools_path)
  File "/home/felcon01/mbedtls/coverity/mbedtls-test/coverity/push_coverity_scan.py", line 163, in download_coverity_scan_tools
    tar_file.extractall(path=tools_dir, members=filter_root_tar_dir(tar_file), filter='data')
  File "/usr/lib/python3.10/tarfile.py", line 2278, in extractall
    tarinfo = self._get_extract_tarinfo(member, filter_function, path)
  File "/usr/lib/python3.10/tarfile.py", line 2332, in _get_extract_tarinfo
    self._handle_fatal_error(e)
  File "/usr/lib/python3.10/tarfile.py", line 2330, in _get_extract_tarinfo
    tarinfo = filter_function(tarinfo, path)
  File "/usr/lib/python3.10/tarfile.py", line 819, in data_filter
    new_attrs = _get_filtered_attrs(member, dest_path, True)
  File "/usr/lib/python3.10/tarfile.py", line 806, in _get_filtered_attrs
    raise LinkOutsideDestinationError(member, target_path)
tarfile.LinkOutsideDestinationError: 'jdk21/legal/jdk.compiler/LICENSE' would link to '/home/felcon01/mbedtls/coverity/java.base/LICENSE', which is outside the destination```

For now, use the legacy/transition path of running make -f scripts/legacy.make In future, the script will need to be migrated to CMake. Signed-off-by: David Horstmann <[email protected]>

Add coverity push script

c930fe3

Signed-off-by: Paul Elliott <[email protected]>

paul-elliott-arm added enhancement New feature or request needs: review needs: reviewer size-s Estimated task size: small (~2d) priority-high labels Apr 25, 2024

paul-elliott-arm requested a review from bensze01 April 25, 2024 09:53

paul-elliott-arm self-assigned this Apr 25, 2024

paul-elliott-arm mentioned this pull request Apr 25, 2024

Add coverity push script Mbed-TLS/mbedtls#9051

Closed

4 tasks

gilles-peskine-arm requested changes May 7, 2024

View reviewed changes

gilles-peskine-arm added needs: work and removed needs: review needs: reviewer labels May 7, 2024

paul-elliott-arm added 16 commits May 13, 2024 17:57

Add return types for all functions

a1cd639

Signed-off-by: Paul Elliott <[email protected]>

Remove unnecessary function

d98e931

Mistaken usage of hashlib.md5(), much simpler form is available. Signed-off-by: Paul Elliott <[email protected]>

Change default build step to make -j$(nproc)

7270881

-j without a number means unlimited, and is likely a bad idea on limited systems. Signed-off-by: Paul Elliott <[email protected]>

Change no branch handling to none

4c5ddc2

(Rather than empty string) Signed-off-by: Paul Elliott <[email protected]>

Fix mispellings of trademark

c73f1b9

Signed-off-by: Paul Elliott <[email protected]>

Make comments more PEP257 compliant

ee04afc

Signed-off-by: Paul Elliott <[email protected]>

Remove temp file when downloading coverity tools

3c882db

Signed-off-by: Paul Elliott <[email protected]>

Rename misnamed query params

82201a5

Signed-off-by: Paul Elliott <[email protected]>

Switch over to using subprocess.run()

5ae850c

Refactor previous function as now too small to really be required (error handling now done by exceptions thrown by run(), and only a single API call now required) Signed-off-by: Paul Elliott <[email protected]>

Run pre-build step in a shell.

7e95fc9

Signed-off-by: Paul Elliott <[email protected]>

Add building of generated files to prebuild step.

2310f0c

Also make branch change recurse into submodules, as we now have one. Signed-off-by: Paul Elliott <[email protected]>

Improve temporary tar file creation logic

707fe27

Use 'with' context, copy to backup file if required. Signed-off-by: Paul Elliott <[email protected]>

Add git submodule update call

7f56626

The addition of the framework necessitates this. Signed-off-by: Paul Elliott <[email protected]>

Fix copypasta with tools directory

9cd43e5

Missed as logic causes the script to just redownload the tools. Signed-off-by: Paul Elliott <[email protected]>

Fix resolvable error with backup dir

2c8f84e

Instead of erroring out if the specified backup directory is non-existent, just create it. Signed-off-by: Paul Elliott <[email protected]>

Support other operating systems for coverity tools

0adbd0a

Signed-off-by: Paul Elliott <[email protected]>

paul-elliott-arm added 3 commits November 29, 2024 14:56

Add logging of config file backups

55d1723

Useful for debug more than anything else. Signed-off-by: Paul Elliott <[email protected]>

Fix alphabetical ordering of argument options

4dc88db

.. and update documentation to add new options. Signed-off-by: Paul Elliott <[email protected]>

Fix issue whereby config.h was left modified

77b9f2b

If config.h was changed upstream, it would be backed up prior to these changes being applied locally, and then restored into the wrong state. Signed-off-by: Paul Elliott <[email protected]>

paul-elliott-arm removed the needs: work label Nov 29, 2024

This was referenced Nov 29, 2024

Add generated file checks Mbed-TLS/TF-PSA-Crypto#50

Closed

Move tests/scripts/check-generated-files.sh to the framework Mbed-TLS/mbedtls-framework#71

Closed

gilles-peskine-arm requested changes Nov 29, 2024

View reviewed changes

gilles-peskine-arm added the needs: work label Nov 29, 2024

gilles-peskine-arm requested a review from davidhorstmann-arm November 29, 2024 18:29

paul-elliott-arm added 3 commits December 6, 2024 15:00

Fix copypasta

ca2d4b4

Moving the backup of config files fixed the previous issue, but created another one. Signed-off-by: Paul Elliott <[email protected]>

Log both stdout and stderr on CalledProcessError

fa1fbe2

Signed-off-by: Paul Elliott <[email protected]>

Add a --no-upload option

c15b488

Enable testing without consuming a coverity credit Signed-off-by: Paul Elliott <[email protected]>

gilles-peskine-arm previously approved these changes Dec 6, 2024

View reviewed changes

Add recursive flag to submodule update

2da0cef

With the addition of tf-psa-crypto as a submodule, which itself has framework as a submodule, git submodule calls now need to be recursive Signed-off-by: Paul Elliott <[email protected]>

paul-elliott-arm dismissed gilles-peskine-arm’s stale review via 2da0cef December 19, 2024 12:39

Backup tf-psa-crypto config file as well as mbedtls

dfb02ad

Required some re-work to have two files. May not entirely be the optimal way of doing this, but probably only worth more work if more files are required. Signed-off-by: Paul Elliott <[email protected]>

paul-elliott-arm removed the needs: work label Feb 11, 2025

Update pre-build step

d1f14be

Update pre-build step due to the removal of check-generated-files.sh. Signed-off-by: Ronald Cron <[email protected]>

gilles-peskine-arm requested changes Apr 7, 2025

View reviewed changes

gilles-peskine-arm assigned ronald-cron-arm and unassigned paul-elliott-arm Apr 7, 2025

davidhorstmann-arm mentioned this pull request Jul 17, 2025

Add coverity push script for 3.6 #215

Draft

gilles-peskine-arm reviewed Aug 8, 2025

View reviewed changes

davidhorstmann-arm mentioned this pull request Sep 30, 2025

Migrate Coverity to CMake #221

Open

Fix script to work with deprecated Make

2d6ee18

For now, use the legacy/transition path of running make -f scripts/legacy.make In future, the script will need to be migrated to CMake. Signed-off-by: David Horstmann <[email protected]>


		os.chdir(mbedtls_dir)

		# Ensure that given git directory is up to date.

	backup_path = backup_path / datetime.today().strftime('mbedtls-%y-%m-%d.tar.gz')
	backup_path = backup_path / datetime.today().strftime('mbedtls-%Y-%m-%d.tar.gz')

Add coverity push script #165

Are you sure you want to change the base?

Add coverity push script #165

Conversation

paul-elliott-arm commented Apr 25, 2024

Description

Uh oh!

paul-elliott-arm commented Apr 25, 2024

Uh oh!

gilles-peskine-arm left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gilles-peskine-arm Nov 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm Nov 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paul-elliott-arm Dec 6, 2024

Choose a reason for hiding this comment

Uh oh!

paul-elliott-arm commented Dec 6, 2024

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm Apr 7, 2025

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm Apr 7, 2025

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm Apr 7, 2025

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm Apr 7, 2025

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm Aug 8, 2025

Choose a reason for hiding this comment

Uh oh!

felixc-arm commented Aug 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gilles-peskine-arm left a comment •

edited

Loading

gilles-peskine-arm Nov 29, 2024 •

edited

Loading

gilles-peskine-arm Apr 7, 2025 •

edited

Loading

gilles-peskine-arm Nov 29, 2024 •

edited

Loading

gilles-peskine-arm Apr 7, 2025 •

edited

Loading