quantile: nil deref #561
quantile: nil deref #561
Conversation
| @@ -76,7 +76,7 @@ func (e *E2eProcessingLatencyAggregate) Less(i, j int) bool { | |||
|
|
|||
| func (e *E2eProcessingLatencyAggregate) Add(e2 *E2eProcessingLatencyAggregate, N int) *E2eProcessingLatencyAggregate { | |||
| if e == nil { | |||
| *e = *e2 | |||
| e = &(*e2) | |||
There was a problem hiding this comment.
This copies and then assigns the (new) address of e2, which is eventually returned by Add and then assigned over the original caller.
Gimme a bit to document that this method is side-effect free (and actually make the other branch in this method act that way instead of overwriting the receiver).
|
recommend reviewing via https://github.com/bitly/nsq/pull/561/files?w=1 |
| @@ -74,36 +74,43 @@ func (e *E2eProcessingLatencyAggregate) Less(i, j int) bool { | |||
| return e.Percentiles[i]["percentile"] > e.Percentiles[j]["percentile"] | |||
| } | |||
|
|
|||
| // Add merges two E2eProcessingLatencyAggregate by averaging the percentiles | |||
| // | |||
| // It is side-effect free and does not modify the receiver or the parameter | |||
There was a problem hiding this comment.
As written this function does modify the receiver.
There was a problem hiding this comment.
And actually, why is this desirable? Why not just make this func (e *E2eProcessingLatencyAggregate) Add(e2 *E2eProcessingLatencyAggregate, N int) and have it do an in-place update? From looking at the callers it seems like they're all doing x = x.Add(y) anyway.
There was a problem hiding this comment.
I would be fine with this approach, too (as it would also now be internally consistent).
... and I thought this would be possible too, even if x is initially nil, and I might be missing something simple and obvious, but I couldn't seem to get it to work the way I would have expected.
There was a problem hiding this comment.
No, Go is pass-by-value always so that's not possible.
My suggestion is to change the calling code so that the receiver is never nil. I'm happy to send over a counter-PR to fix this bug, if you like.
There was a problem hiding this comment.
I'll update - thanks for the help!
|
@cespare check that latest commit |
| p = e.Percentiles | ||
| p[i]["quantile"] = value["quantile"] | ||
| // Add merges the passed E2eProcessingLatencyAggregate into the receiver | ||
| // by averaging the percentiles |
There was a problem hiding this comment.
How about
// Add merges e2 into e by averaging the percentiles.
|
LGTM |
`Add()` intended to be side-effect free when called on a `nil` receiver.
|
ready @jehiah |
See commit comments.
nsqadminandnsq_statare the only bundledapps that end up using this code path and currently panic.
cc @cespare
RFR @jehiah