search

SparkPost / gosparkpost

SparkPost client library for the Go Programming Language
https://www.sparkpost.com/
Other
62 stars 40 forks source link

Segfault due to broken example and missing check #96

Closed matthewvalimaki closed 7 years ago

matthewvalimaki commented 7 years ago

I did copy & paste from README example and got a segfault:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x28 pc=0x69b3f6]

goroutine 60 [running]:
net/http.(*Client).deadline(0x0, 0xc42000e928, 0x410612, 0xc4202469c0)
    /usr/local/go/src/net/http/client.go:186 +0x26
net/http.(*Client).Do(0x0, 0xc420181800, 0xc4200101f0, 0xc420181800, 0x0)
    /usr/local/go/src/net/http/client.go:497 +0x89
github.com/SparkPost/gosparkpost.(*Client).DoRequest(0xc4200f5be8, 0xc15440, 0xc4200101f0, 0x9a19f8, 0x4, 0xc42029b7d0, 0x2e, 0xc4200aa800, 0xb47, 0x1612, ...)
    /home/email/lib/go/src/github.com/SparkPost/gosparkpost/common.go:196 +0x609
github.com/SparkPost/gosparkpost.(*Client).HttpPost(0xc4200f5be8, 0xc15440, 0xc4200101f0, 0xc42029b7d0, 0x2e, 0xc4200aa800, 0xb47, 0x1612, 0x30, 0xc42029ade0, ...)
    /home/email/lib/go/src/github.com/SparkPost/gosparkpost/common.go:112 +0x9f
github.com/SparkPost/gosparkpost.(*Client).SendContext(0xc4200f5be8, 0xc15440, 0xc4200101f0, 0xc4205883c0, 0x40e57f, 0x979400, 0xc420188e60, 0xc4200f5c18, 0xc420188e60)
    /home/email/lib/go/src/github.com/SparkPost/gosparkpost/transmissions.go:227 +0x292
github.com/SparkPost/gosparkpost.(*Client).Send(0xc4200f5be8, 0xc4205883c0, 0x979400, 0xc420188e60, 0x0, 0x0, 0x0)
    /home/email/lib/go/src/github.com/SparkPost/gosparkpost/transmissions.go:205 +0x5

The cause of this is https://github.com/SparkPost/gosparkpost/blob/master/common.go#L196 and the fact that README never initializes the c.Client unlike its unit test https://github.com/SparkPost/gosparkpost/blob/master/common_test.go#L30.

If I modify common.go line 196 to

        if c.Client == nil {
                c.Client = http.DefaultClient
        }                
        res, err := c.Client.Do(req)

the segfault goes away.

My suggestions are: 1) Append tests to include what README does. 2) Apply my common.go modification to guarantee client existence. Note that as c.Client is exported it can be set outside of init and thus my modification in DoRequest. Or perhaps make Client be client and force use of NewClient() which would then always do return &client{Client: http.DefaultClient}.

yargevad commented 7 years ago

Hi @matthewvalimaki - thanks for the report, that example is indeed busted right now!

DoRequest needs to remain concurrency-safe wrt Client, so I'm going to update Init to use http.DefaultClient if one isn't already set, and return an error from Client.DoRequest if Client.Client (heh) is nil, instead of panicking.