sync: guard semaphore grant self-move #35249

pull CruzMolina wants to merge 1 commits into bitcoin:master from CruzMolina:semaphore-grant-self-move changing 2 files +24 −5
  1. CruzMolina commented at 10:30 PM on May 8, 2026: none

    Problem

    CountingSemaphoreGrant::operator=(CountingSemaphoreGrant&&) releases the currently held semaphore grant before taking ownership from the source grant.

    On self-move assignment, the source and destination are the same object, so this releases the held grant and leaves the object empty.

    Fix

    Guard move assignment against self-move, making it a no-op when source and destination alias.

    Add a regression test that verifies a self-moved grant remains held.

    Test

    git diff --check
    cmake --build build --target test_bitcoin -j 8
    build/bin/test_bitcoin --run_test=sync_tests
    
  2. sync: guard semaphore grant self-move
    CountingSemaphoreGrant::operator=(CountingSemaphoreGrant&&) releases the currently held grant before taking ownership from the source grant. On self-move assignment, the source and destination are the same object, so this releases the semaphore grant and leaves the object empty.
    
    Guard against self-move so a held grant remains held.
    
    Add a regression test for self-move assignment.
    0a3c13ddb7
  3. DrahtBot commented at 10:30 PM on May 8, 2026: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35249.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK kevkevinpal

    If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  4. kevkevinpal commented at 6:04 PM on May 9, 2026: contributor

    ACK 0a3c13d

    This looks like a safe change to me. It makes sense to avoid self-assignment. I tested the new test on master without the change to src/semaphore_grant.h and it fails as expected.

    Just a note I only see the = operator used twice for CountingSemaphoreGrant

    src/net.cpp|3022 col 19| grant = CountingSemaphoreGrant<>(*semAddnode, /*fTry=*/true);
    src/net.cpp|3068 col 26| pnode->grantOutbound = std::move(grant_outbound);
    

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: 2026-05-11 12:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me