6 new issues identified by coverity

[chipitsine]

please let me known which of them are false positive

** CID 1430474:  Uninitialized variables  (UNINIT)
/src/log.c: 1804 in __do_send_log()


________________________________________________________________________________________________________
*** CID 1430474:  Uninitialized variables  (UNINIT)
/src/log.c: 1804 in __do_send_log()
1798                            sent = fd_write_frag_line(*plogfd, logsrv->maxlen, msg_header, nbelem, &msg, 1, 1);
1799            }
1800            else {
1801                    int i = 0;
1802                    int totlen = logsrv->maxlen;
1803     
>>>     CID 1430474:  Uninitialized variables  (UNINIT)
>>>     Using uninitialized value "nbelem".
1804                    for (i = 0 ; i < nbelem ; i++ ) {
1805                            iovec[i].iov_base = msg_header[i].ptr;
1806                            iovec[i].iov_len  = msg_header[i].len;
1807                            if (totlen <= iovec[i].iov_len) {
1808                                    iovec[i].iov_len = totlen;
1809                                    totlen = 0;

** CID 1430473:    (CHECKED_RETURN)
/src/proto_udp.c: 291 in udp_bind_listener()
/src/proto_udp.c: 293 in udp_bind_listener()
/src/proto_udp.c: 233 in udp_bind_listener()


________________________________________________________________________________________________________
*** CID 1430473:    (CHECKED_RETURN)
/src/proto_udp.c: 291 in udp_bind_listener()
285                             err |= ERR_WARN;
286                     }
287             }
288     #endif
289     #if defined(IPV6_V6ONLY)
290             if (listener->options & LI_O_V6ONLY)
>>>     CID 1430473:    (CHECKED_RETURN)
>>>     Calling "setsockopt(fd, IPPROTO_IPV6, 26, &one, 4U)" without checking return value. This library function may fail and return an error code.
291                     setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, &one, sizeof(one));
292             else if (listener->options & LI_O_V4V6)
293                     setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, &zero, sizeof(zero));
294     #endif
295     
296             if (bind(fd, (struct sockaddr *)&addr_inet, listener->proto->sock_addrlen) < 0) {
/src/proto_udp.c: 293 in udp_bind_listener()
287             }
288     #endif
289     #if defined(IPV6_V6ONLY)
290             if (listener->options & LI_O_V6ONLY)
291                     setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, &one, sizeof(one));
292             else if (listener->options & LI_O_V4V6)
>>>     CID 1430473:    (CHECKED_RETURN)
>>>     Calling "setsockopt(fd, IPPROTO_IPV6, 26, &zero, 4U)" without checking return value. This library function may fail and return an error code.
293                     setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, &zero, sizeof(zero));
294     #endif
295     
296             if (bind(fd, (struct sockaddr *)&addr_inet, listener->proto->sock_addrlen) < 0) {
297                     err |= ERR_RETRYABLE | ERR_ALERT;
298                     msg = "cannot bind socket";
/src/proto_udp.c: 233 in udp_bind_listener()
227     
228     #ifdef SO_REUSEPORT
229             /* OpenBSD and Linux 3.9 support this. As it's present in old libc versions of
230              * Linux, it might return an error that we will silently ignore.
231              */
232             if (global.tune.options & GTUNE_USE_REUSEPORT)
>>>     CID 1430473:    (CHECKED_RETURN)
>>>     Calling "setsockopt(fd, 1, 15, &one, 4U)" without checking return value. This library function may fail and return an error code.
233                     setsockopt(fd, SOL_SOCKET, SO_REUSEPORT, &one, sizeof(one));
234     #endif
235     
236             if (listener->options & LI_O_FOREIGN) {
237                     switch (addr_inet.ss_family) {
238                     case AF_INET:

** CID 1430472:  Insecure data handling  (TAINTED_SCALAR)


________________________________________________________________________________________________________
*** CID 1430472:  Insecure data handling  (TAINTED_SCALAR)
/src/log.c: 3547 in syslog_fd_handler()
3541                            buf->data = ret;
3542     
3543                            /* update counters */
3544                            _HA_ATOMIC_ADD(&cum_log_messages, 1);
3545                            proxy_inc_fe_conn_ctr(l, l->bind_conf->frontend);
3546     
>>>     CID 1430472:  Insecure data handling  (TAINTED_SCALAR)
>>>     Passing tainted variable "buf->area" to a tainted sink.
3547                            parse_log_message(buf->area, buf->data, &level, &facility, metadata, &message, &size);
3548     
3549                            process_send_log(&l->bind_conf->frontend->logsrvs, level, facility, metadata, message, size);
3550     
3551                    } while (--max_accept);
3552            }

** CID 1430471:  Null pointer dereferences  (FORWARD_NULL)
/src/log.c: 1805 in __do_send_log()


________________________________________________________________________________________________________
*** CID 1430471:  Null pointer dereferences  (FORWARD_NULL)
/src/log.c: 1805 in __do_send_log()
1799            }
1800            else {
1801                    int i = 0;
1802                    int totlen = logsrv->maxlen;
1803     
1804                    for (i = 0 ; i < nbelem ; i++ ) {
>>>     CID 1430471:  Null pointer dereferences  (FORWARD_NULL)
>>>     Dereferencing null pointer "msg_header".
1805                            iovec[i].iov_base = msg_header[i].ptr;
1806                            iovec[i].iov_len  = msg_header[i].len;
1807                            if (totlen <= iovec[i].iov_len) {
1808                                    iovec[i].iov_len = totlen;
1809                                    totlen = 0;
1810                                    break;

** CID 1430470:  Null pointer dereferences  (FORWARD_NULL)
/src/log.c: 3524 in syslog_fd_handler()


________________________________________________________________________________________________________
*** CID 1430470:  Null pointer dereferences  (FORWARD_NULL)
/src/log.c: 3524 in syslog_fd_handler()
3518     
3519            if (fdtab[fd].ev & FD_POLL_IN) {
3520     
3521                    if (!fd_recv_ready(fd))
3522                            return;
3523     
>>>     CID 1430470:  Null pointer dereferences  (FORWARD_NULL)
>>>     Dereferencing null pointer "l".
3524                    max_accept = l->maxaccept ? l->maxaccept : 1;
3525     
3526                    do {
3527                            /* Source address */
3528                            struct sockaddr_storage saddr = {0};
3529                            socklen_t saddrlen;

** CID 1430469:    (DEADCODE)
/src/log.c: 1714 in build_log_header()
/src/log.c: 1510 in build_log_header()
/src/log.c: 1646 in build_log_header()


________________________________________________________________________________________________________
*** CID 1430469:    (DEADCODE)
/src/log.c: 1714 in build_log_header()
1708                    case LOG_FORMAT_PRIO:
1709                    case LOG_FORMAT_SHORT:
1710                    case LOG_FORMAT_TIMED:
1711                    case LOG_FORMAT_ISO:
1712                    case LOG_FORMAT_RAW:
1713                            break;
>>>     CID 1430469:    (DEADCODE)
>>>     Execution cannot reach this statement: "case LOG_FORMAT_UNSPEC:".
1714                    case LOG_FORMAT_UNSPEC:
1715                    case LOG_FORMATS:
1716                            ABORT_NOW();
1717            }
1718     
1719            return hdr_ctx.ist_vector;
/src/log.c: 1510 in build_log_header()
1504                            *hdr_ctx.ist_vector[*nbelem].ptr = '<';
1505                            hdr_ctx.ist_vector[(*nbelem)++].len = &hdr_ctx.priority_buffer[5] - hdr_ctx.ist_vector[0].ptr;
1506                            break;
1507                    case LOG_FORMAT_ISO:
1508                    case LOG_FORMAT_RAW:
1509                            break;
>>>     CID 1430469:    (DEADCODE)
>>>     Execution cannot reach this statement: "case LOG_FORMAT_UNSPEC:".
1510                    case LOG_FORMAT_UNSPEC:
1511                    case LOG_FORMATS:
1512                            ABORT_NOW();
1513            }
1514     
1515     
/src/log.c: 1646 in build_log_header()
1640                            hdr_ctx.ist_vector[(*nbelem)++] = ist2(timeofday_as_iso_us(1), LOG_ISOTIME_MAXLEN + 1);
1641                            break;
1642                    case LOG_FORMAT_PRIO:
1643                    case LOG_FORMAT_SHORT:
1644                    case LOG_FORMAT_RAW:
1645                            break;
>>>     CID 1430469:    (DEADCODE)
>>>     Execution cannot reach this statement: "case LOG_FORMAT_UNSPEC:".
1646                    case LOG_FORMAT_UNSPEC:
1647                    case LOG_FORMATS:
1648                            ABORT_NOW();
1649            }
1650     
1651            /* prepare other meta data, stored into a max of 10 elems */

[wtarreau]

@EmericBr I'd like you to have a look at these as I suspect at least some of them are real bugs.

[capflam]

• CID 1430474 : It is a false positive. The code is not obvious but nbelem is only uninitialized for loggers using ring buffers. In this case, the variable is not used.

• CID 1430473 : There are many other places where calls to setsockopt are not checked. These one should not fail.

• CID 1430472 : I don't see anything strange and I don't really understand the coverity complaint here. I guess it is a false positive.

• CID 1430471 : Same than CID 1430474.

• CID 1430470 : false positive. l variable is checked at the function beginning.

• CID 1430469 : false positive. The code is dead but avoids a warning during compilation about not handled enumeration value. I guess it may be replaced by a "default" case to make coverity happy. But honestly, that's not really useful.

[TimWolla]

* CID 1430470 : false positive. `l` variable is checked at the function beginning.

objt_listener can possibly return NULL. Should it be replaced with __objt_listener?

[capflam]

Yes, __objt_listener can be used because the fd's owner is always a listener. Otherwise it is a bug. There is an explicit test to abort if l is NULL at the function beginning. I guess Coverity is unable to understand the ABORT_NOW() macro. Adding a return statement just after it may also be a workaround to help Coverity. But it remains a false positive. There is no bug here.

[TimWolla]

There is an explicit test to abort if l is NULL at the function beginning.

Ah, indeed. I missed that one.

Adding a return statement just after it may also be a workaround to help Coverity.

Coverity should probably be teached that this kills the process. @chipitsine Do you know whether some define can be used to check for coverity? Then the definition could possibly be adjusted to something like:

#ifdef COVERITY
#define ABORT_NOW() abort()
#else
#define ABORT_NOW() (*(volatile int*)1=0)
#endif

[chipitsine]

we run coverity using our own build process. coverity does not know nothing about it, I'd give 100% that it does not define any macros (because we start build process)

but we can add define to our coverity scan

[chipitsine]

I think, we can try coverity modeling. I'll have a look

[chipitsine]

@TimWolla , what do you think, if we pass #define ABORT_NOW() abort() via command line build (preprocessor) parameter in coverity job?

[TimWolla]

I guess that should work, yes. Make sure to take care of the #includes. But I am certainly not an authoritative figure in that regard.

[chipitsine]

I've marked issues in Coverity.
I'll send a patch with define soon

[chipitsine]

it works if I use SILENT_DEFINE

make SILENT_DEFINE=-D\'ABORT_NOW\(\)=abort\(\)\' CC=clang TARGET=$TARGET $FLAGS 51DEGREES_SRC=$FIFTYONEDEGREES_SRC

it fails with just DEFINE

make DEFINE=-D\'ABORT_NOW\(\)=abort\(\)\' CC=clang TARGET=$TARGET $FLAGS 51DEGREES_SRC=$FIFTYONEDEGREES_SRC
...
/bin/sh: -c: line 5: syntax error near unexpected token `('
/bin/sh: -c: line 5: `      -DBUILD_CFLAGS='"-O2 -g -Wall -Wextra -Wdeclaration-after-statement -fwrapv -Wno-address-of-packed-member -Wno-unused-label -Wno-sign-compare -Wno-unused-parameter -Wno-missing-field-initializers -Wno-string-plus-int -Wtype-limits -Wshift-negative-value -Wnull-dereference -D'ABORT_NOW()=abort()'"' \'
make: *** [Makefile:881: src/haproxy.o] Error 1

I'll try to fix DEFINE escaping. if I won't be lucky, I'll send patch with SILENT_DEFINE

[wtarreau]

Hi Ilya,

I suggest that you don't waste your time trying to fix DEFINE escaping. I gave up years ago, it involves escaping for shell, makefile and C at the same time. The further you go, the more it requires the user to be extra careful and that was not worth it. Just use SILENT_DEFINE for this, that's perfectly fine. Just my two cents ;-)

[wtarreau]

By the way, you can do something simpler, along what Tim proposed above. Instead of testing for COVERITY, test for DEBUG_USE_ABORT and set this variable in the DEBUG one at build time. Do not forget to mention it in the makefile where other ones are enumerated. No need to go into details, those using DEBUG are responsible for knowing the consequences, hence reading the code.

[chipitsine]

changes applied in d344910

closing this issue