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

peer: panic early on queued nil messages #3213

Merged
merged 1 commit into from
Nov 13, 2023
Merged

Conversation

jrick
Copy link
Member

@jrick jrick commented Nov 13, 2023

#3212 (running a commit from late June) appears to be a situation very similar to what was accidentally created by 3aaaf3f (Oct 30) and fixed by c1e4cdd (Oct 31). Since something else has been sending nil messages before, and we may not have found and fixed that caller too, let's go ahead and add this panic now to find any still-existing and future incorrect uses of this API.

@jrick
Copy link
Member Author

jrick commented Nov 13, 2023

There's another possible nil message panic that this check is not able to detect, which is if the interface variable is non-nil but contains a concrete wire message type with a nil value. I'm not sure of any great way to detect this, but it would be nice.

@ukane-philemon
Copy link

which is if the interface variable is non-nil but contains a concrete wire message type with a nil value. I'm not sure of any great way to detect this, but it would be nice.

We can cast and check for known types that implement the interface. msg, ok := interfaceVar.(type); if msg == nil { // do something? };

@jrick
Copy link
Member Author

jrick commented Nov 13, 2023

yes but you would need a check for every message type in the type switch.

@davecgh
Copy link
Member

davecgh commented Nov 13, 2023

There's another possible nil message panic that this check is not able to detect, which is if the interface variable is non-nil but contains a concrete wire message type with a nil value. I'm not sure of any great way to detect this, but it would be nice.

It can be done with reflect, but that's pretty heavy. Another way would be to create a helper func that simply tries to type cast it to all wire types and checks against nil. That wouldn't be robust against new wire types, but it would still be quite an improvement over nothing I think.

@jrick
Copy link
Member Author

jrick commented Nov 13, 2023

I did a quick audit of the codebase and didn't spot any codepaths where it was obviously doing something that might cause that issue, i.e. nothing like

var tx *wire.MsgTx
if cond {
    // maybe assign tx
}
// queue tx without checking tx == nil
p.QueueMessage(tx, nil)

peer/peer.go Outdated Show resolved Hide resolved
@davecgh
Copy link
Member

davecgh commented Nov 13, 2023

I did a quick audit of the codebase and didn't spot any codepaths where it was obviously doing something that might cause that issue, i.e. nothing like

I was doing the same and didn't spot anything either.

@davecgh davecgh added this to the 1.9.0 milestone Nov 13, 2023
If the doneChan is nil, the nil message can be safely ignored.  However, this
is still a bug in the caller, so a warning with stacktrace is logged.

If the doneChan is non-nil, panicing is the only reasonable option to prevent
the program continuing on in an unknown state.

The peer code would already panic previously in wire.WriteMessageN for many
queued message types, and introducing a new panic call here should not
introduce any new crashes that would not have occurred before.
@davecgh davecgh merged commit 3df2b50 into decred:master Nov 13, 2023
2 checks passed
@jrick jrick deleted the early_panic branch November 13, 2023 16:54
@davecgh davecgh mentioned this pull request Nov 13, 2023
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.

3 participants