Skip to content
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

fix: check nil before calling clearTimer #1869

Merged
merged 1 commit into from
Aug 2, 2023
Merged

fix: check nil before calling clearTimer #1869

merged 1 commit into from
Aug 2, 2023

Conversation

vpavlin
Copy link
Member

@vpavlin vpavlin commented Aug 1, 2023

Description

When setting up RPC server failed and a clean up process started, Filter v2 protocol attempted to call clearTimer on uninitialized attribute which caused SIGSEGV: Illegal storage access. (Attempt to read from nil?)

Changes

  • check if maintenanceTask is Nil before calling clearTimer

How to test

  1. Run Waku node as described in bug: nwaku randomly fails to start #1866
 docker run --network host -it statusteam/nim-waku:v0.18.0 --relay=true --filter=true --lightpush=true --rpc-admin=true --rpc-port=8545 --rpc-address=0.0.0.0 --websocket-support=true --nodekey=ca18f11664fed73af38b11ffc3ccdbdd0738c475488dfab18a91f43a1f78711a
  1. Run the same command again in separate terminal - you'll get SIGSEGV because it fails to setup RPC server (conflicting port) and the clean up hits this issue
  2. Compile wakunode2 from this PR and run it instead - you will get the actual error log and the clean up runs normally
ERR 2023-08-01 20:35:07.556+02:00 5/7 Starting node and protocols failed     topics="wakunode main" tid=34662 file=wakunode2.nim:92 error="failed to start waku node: (98) Address already in use"

Issue

closes #1866

Copy link
Contributor

@SionoiS SionoiS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 🦀 in me says to always use options.

nil values are stupid and I hate them.

Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for it!

@vpavlin vpavlin merged commit 2fc4884 into master Aug 2, 2023
13 checks passed
@vpavlin vpavlin deleted the fix/cleartimer branch August 2, 2023 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: nwaku randomly fails to start
3 participants