pebbe / zmq4

A Go interface to ZeroMQ version 4
BSD 2-Clause "Simplified" License
1.17k stars 163 forks source link

zmq_ctx_new error handling can potentially fail #128

Closed luccasmenezes closed 6 years ago

luccasmenezes commented 6 years ago

Sometimes the following code in zmq4.go can lead to errors even though zmq_ctx_new returned successfully:

var err error
defaultCtx = &Context{}
defaultCtx.ctx, err = C.zmq_ctx_new()
if defaultCtx.ctx == nil || err != nil {
    initContextError = fmt.Errorf("Init of ZeroMQ context failed: %v", errget(err))
    return
}
defaultCtx.opened = true

Since it's checking for defaultCtx.ctx == nil || err != nil instead of defaultCtx.ctx == nil && err != nil, loading a library that sets errno to something other than 0 can make the if statement to return true and cause an error on client applications.

According to http://api.zeromq.org/4-2:zmq-ctx-new in case of errors, the function returns NULL and then sets errno to something else, so I believe the correct way of checking errors for this function is first checking whether zmq_ctx_new returned NULL and then checking the err variable.

I stumbled across this bug when compiling zmq 4.2.2 with support to libpgm (./configure --with-libpgm) and tried to use the go binding inside a Ubuntu 16.04 docker container.

When initializing zmq with pgm the function pgm_init is called and it tries to load the file /etc/protocols which doesn't exist inside the default Ubuntu 16.04 docker image this behavior causes errno = 2 to be set and it later triggers the code above and causes err != nil condition to be evaluated to true even though ctx is valid.

I'm also submitting a PR for review.

pebbe commented 6 years ago

I have looked through the code, and as far as I can see, this is the only location where this is an issue. In all other cases I ignore the value of err if the regular return value does not indicate an error.

I won't use your PR. I will just remove the check for err != nil.

I wil do the same in zmq3 and zmq2.

Thank you.

luccasmenezes commented 6 years ago

Thanks Peter! I closed the PR.