src/backend.c: null pointer dereference suspected by coverity

[chipitsine]

Tool Name and Version

coverity

Code Report

/src/backend.c: 891 in alloc_dst_address()
885                     if (!(s->flags & SF_ASSIGNED))
886                             return SRV_STATUS_INTERNAL;
887     
888                     if (!sockaddr_alloc(ss, NULL, 0))
889                             return SRV_STATUS_INTERNAL;
890     
>>>     CID 1596090:  Null pointer dereferences  (FORWARD_NULL)
>>>     Dereferencing null pointer "srv".
891                     **ss = srv->addr;
892                     set_host_port(*ss, srv->svc_port);
893                     if (!is_addr(*ss)) {
894                             /* if the server has no address, we use the same address
895                              * the client asked, which is handy for remapping ports
896                              * locally on multiple addresses at once. Nothing is done

Additional Information

No response

Output of haproxy -vv

no

[Darlelet]

Caused by 68cf3959b345ec9d596d40b01eb416d2bef6522c

Looks like false positive. Prior to this commit srv pointer was retrieved using __objt_server(s->target) under assign_server_address() function. Now alloc_dst_address() takes srv pointer as parameter which is retrieved in parent function (connect_server()) using objt_server(s->target).

The only difference between __objt_server(s->target) and objt_server(s->target) is that the latter is a safe accessor that may return NULL if provided invalid object type (s->target). Perhaps connect_server() is ambiguous whether srv may be NULL or not at some point (are srv != NULL checks needed there? I mean can srv really be NULL in connect_server() at some point), and because of that the ambiguity flows down to alloc_dst_address(). But from alloc_dst_address() point of view, if the unsafe accessor used to work then the safe one will :)

[a-denoyelle]

srv instance can indeed be NULL in connect_server() when using transparent proxy or dispatch. But this is a marginal case.

[a-denoyelle]

However, regarding alloc_dst_address(), the flag SF_ASSIGNED should guarantee that srv instance is properly assigned to the stream.

[wtarreau]

Maybe placing an ASSUME_NONNULL(srv) somewhere would help ? (sometimes it can even help the compiler produce better code).

[a-denoyelle]

I'll add it to be more explicit.