ci: Run fuzz testing test cases (bitcoin-core/qa-assets) under valgrind to catch memory errors #18912

pull practicalswift wants to merge 1 commits into bitcoin:master from practicalswift:fuzzers-under-valgrind changing 1 files +5 −0
  1. practicalswift commented at 5:20 am on May 8, 2020: contributor

    Re-introduce the Travis valgrind fuzzing job which was removed by PR #18899. The removal seems to have been made by accident since the removed job does not appear to be the source of the problem the PR set out to fix.

    Run fuzz testing test cases (bitcoin-core/qa-assets) under valgrind.

    This would have caught util: Avoid potential uninitialized read in FormatISO8601DateTime(int64_t) by checking gmtime_s/gmtime_r return value (#18162) and similar cases.

    This fuzzing job was introduced in #18166.

  2. fanquake added the label Tests on May 8, 2020
  3. DrahtBot commented at 11:15 am on May 8, 2020: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #19179 ([WIP RFC DONOTMERGE] ci: Run ci configs on cirrus by MarcoFalke)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  4. practicalswift commented at 8:03 pm on May 8, 2020: contributor
    FWIW: The added valgrind Travis fuzz job is twice as fast as the slowest Travis job we’re currently using, so it should be entirely safe to re-add this one from a build-time perspective :)
  5. practicalswift commented at 4:45 am on May 9, 2020: contributor

    Pinging in reviewers of #18899 (“travis: Remove valgrind “) since that PR was locked for further peer review comments leaving the question of fuzz job removal unanswered.

    Friendly ping @jamesob, @jnewbery, @promag, @sipa, @laanwj and @MarcoFalke: would you mind reviewing this related PR too? :)

  6. MarcoFalke commented at 11:57 am on May 12, 2020: member
    I am no longer maintaining travis for Bitcoin Core, so I don’t have an opinion on this. Personally, I moved my ci configs and scripts to https://github.com/MarcoFalke/btc_nightly . This config is one of many more that is run every day.
  7. practicalswift commented at 3:15 pm on May 12, 2020: contributor

    @MarcoFalke Understood! Thanks for clarifying. Could you clarify if the valgrind fuzzing job was causing any problems prior to the removal, or if it just slipped in with the rest of the valgrind removal? :)

    If it was causing any issues that would be good to know for the reviewers of this PR to have the full picture. IIRC the valgrind fuzzing job has never caused any issues for me at least :)

  8. jnewbery commented at 0:55 am on May 13, 2020: member
    Concept ACK. The fuzzing under valgrind job seems to complete much more quickly then the functional tests under ASAN/TSAN jobs.
  9. MarcoFalke commented at 2:10 pm on May 14, 2020: member

    It is not a specific test that causes problems, but if we start adding a new travis build for any configuration that could possibly find an issue, it is going to slow down everything in the same way.

    For example we could add an alpine build (#18110) to check against musl libc, we could add a centos 7 build to check the minimum supported version of boost (#18943), we could add a *bsd run to check compilation against *bsd, we could add a build to check against the guix binaries, we could add a build to run the fuzzers on musl libc, we could …

    At some point we need to take into account the cost of a build against the value it adds.

    I think as part of every pull request we should only run essential builds that, if failed, would bring the project development to a halt. For example breaking the gui build, or the --disable-wallet build would break most dev environments, either immediately or later when debugging a --disable-wallet bug via git bisect. Another example would be breaking the unit tests or functional tests. Intermittent issues are excluded from here, because it is impossible to protect against them.

    If, however the build is broken for a few days on a system that is not run by developers (e.g. centos 7) or alpine linux, it would not bring the development to a halt.

    If more builds are added to travis, then someone needs to pay for them. Personally, I think the value we get out of travis is not worth the money we spend on them. Partly because they fail to address our issues like #18868 .

    Whoever is going to renew (or not renew) the contract for the next 12 months should take this into account.

  10. practicalswift commented at 6:29 pm on May 14, 2020: contributor

    It is not a specific test that causes problems, but if we start adding a new travis build for any configuration that could possibly find an issue, it is going to slow down everything in the same way.

    For example we could add an alpine build (#18110) to check against musl libc, we could add a centos 7 build to check the minimum supported version of boost (#18943), we could add a *bsd run to check compilation against *bsd, we could add a build to check against the guix binaries, we could add a build to run the fuzzers on musl libc, we could …

    That is true, but note that not all issues are equally important for protecting our user’s funds :)

    The now removed valgrind fuzzing job helps us catch security issues whereas the examples you quote would help us catch non-security issues.

    (Meta comment about our peer review process: I fully understand the frustration with Travis but TBH I think it was a mistake to lock #18899 immediately after merge without answering the question that was raised during peer review about the surprising fuzzing job removal.)

  11. DrahtBot added the label Needs rebase on May 21, 2020
  12. practicalswift force-pushed on May 21, 2020
  13. practicalswift commented at 2:13 pm on May 21, 2020: contributor
    Rebased :)
  14. DrahtBot removed the label Needs rebase on May 21, 2020
  15. DrahtBot added the label Needs rebase on Jun 19, 2020
  16. MarcoFalke commented at 5:08 pm on June 19, 2020: member
    (note: needs rebase on 37ae687 before merge)
  17. ci: Run fuzz testing test cases (bitcoin-core/qa-assets) under valgrind to catch memory errors 3f686d1a28
  18. practicalswift force-pushed on Jun 21, 2020
  19. practicalswift commented at 9:03 pm on June 21, 2020: contributor

    @MarcoFalke

    Thanks! Now rebased :)

  20. DrahtBot removed the label Needs rebase on Jun 21, 2020
  21. MarcoFalke merged this on Jun 25, 2020
  22. MarcoFalke closed this on Jun 25, 2020
  23. practicalswift deleted the branch on Apr 10, 2021
  24. DrahtBot locked this on Aug 18, 2022


practicalswift DrahtBot MarcoFalke jnewbery

Labels
Tests

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 06:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me