From: Alan T. DeKok Date: Tue, 24 Oct 2023 19:08:57 +0000 (-0400) Subject: rework macros to be clearer X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=de3753cb44d9640baa178297f261f003d04246c8;p=thirdparty%2Ffreeradius-server.git rework macros to be clearer much less repetition == fewer mistakes, and hopefully less confusion from Coverity --- diff --git a/src/protocols/tacacs/base.c b/src/protocols/tacacs/base.c index 0c9f7e9947f..721369084be 100644 --- a/src/protocols/tacacs/base.c +++ b/src/protocols/tacacs/base.c @@ -313,35 +313,22 @@ ssize_t fr_tacacs_length(uint8_t const *buffer, size_t buffer_len) return length; } -static void print_hex(fr_log_t const *log, char const *file, int line, char const *prefix, uint8_t const *data, size_t datalen, uint8_t const *end) +static void print_hex(fr_log_t const *log, char const *file, int line, char const *prefix, uint8_t const *data, size_t datalen) { - if ((data + datalen) > end) { - fr_assert(data <= end); - - if (data > end) return; - - fr_log(log, L_DBG, file, line, "%s TRUNCATED field says %zu, but only %zu left in the packet", - prefix, datalen, end - data); - - datalen = end - data; - } - if (!datalen) return; fr_log_hex(log, L_DBG, file, line, data, datalen, "%s", prefix); } -static void print_ascii(fr_log_t const *log, char const *file, int line, char const *prefix, uint8_t const *data, size_t datalen, uint8_t const *end) +static void print_ascii(fr_log_t const *log, char const *file, int line, char const *prefix, uint8_t const *data, size_t datalen) { uint8_t const *p; - fr_assert((data + datalen) <= end); - if (!datalen) return; if (datalen > 80) { hex: - print_hex(log, file, line, prefix, data, datalen, end); + print_hex(log, file, line, prefix, data, datalen); return; } @@ -352,75 +339,100 @@ static void print_ascii(fr_log_t const *log, char const *file, int line, char co fr_log(log, L_DBG, file, line, "%s %.*s", prefix, (int) datalen, (char const *) data); } +#define CHECK(_length) do { \ + size_t plen = _length; \ + if ((size_t) (end - p) < plen) { \ + fr_log_hex(log, L_DBG, file, line, p, end - p, "%s", " TRUNCATED "); \ + return; \ + } \ + data = p; \ + data_len = plen; \ + p += plen; \ + } while (0) + +#undef ASCII +#define ASCII(_prefix, _field) do { \ + CHECK(_field); \ + print_ascii(log, file, line, _prefix, data, data_len); \ + } while (0) + +#undef HEXIT +#define HEXIT(_prefix, _field) do { \ + CHECK(_field); \ + print_hex(log, file, line, _prefix, data, data_len); \ + } while (0) + +#define PRINT(_fmt, ...) fr_log(log, L_DBG, file, line, _fmt, ## __VA_ARGS__) + static void print_args(fr_log_t const *log, char const *file, int line, size_t arg_cnt, uint8_t const *argv, uint8_t const *start, uint8_t const *end) { - size_t i; + size_t i, data_len; uint8_t const *p; + uint8_t const *data; char prefix[64]; if (argv + arg_cnt > end) { - fr_log(log, L_DBG, file, line, " ARG cnt overflows packet"); + PRINT(" ARG cnt overflows packet"); return; } p = start; for (i = 0; i < arg_cnt; i++) { if (p == end) { - fr_log(log, L_DBG, file, line, " ARG[%zu] is at EOF", i); + PRINT(" ARG[%zu] is at EOF", i); return; } - if ((p + argv[i]) > end) { - fr_log(log, L_DBG, file, line, " ARG[%zu] overflows packet", i); - print_hex(log, file, line, " ", p, end - p, end); + if ((end - p) < argv[i]) { + PRINT(" ARG[%zu] overflows packet", i); + print_hex(log, file, line, " ", p, end - p); return; } snprintf(prefix, sizeof(prefix), " arg[%zu] ", i); prefix[21] = '\0'; - print_ascii(log, file, line, prefix, p, argv[i], end); - - p += argv[i]; + ASCII(prefix, argv[i]); } } void _fr_tacacs_packet_log_hex(fr_log_t const *log, fr_tacacs_packet_t const *packet, size_t packet_len, char const *file, int line) { - size_t length; + size_t length, data_len; uint8_t const *p = (uint8_t const *) packet; uint8_t const *hdr, *end, *args; + uint8_t const *data; end = ((uint8_t const *) packet) + packet_len; if (packet_len < 12) { - print_hex(log, file, line, "header ", p, packet_len, end); + print_hex(log, file, line, "header ", p, packet_len); return; } /* * It has to be at least 12 bytes long. */ - fr_log(log, L_DBG, file, line, " major %u", (p[0] & 0xf0) >> 4); - fr_log(log, L_DBG, file, line, " minor %u", (p[0] & 0x0f)); + PRINT(" major %u", (p[0] & 0xf0) >> 4); + PRINT(" minor %u", (p[0] & 0x0f)); - fr_log(log, L_DBG, file, line, " type %02x", p[1]); - fr_log(log, L_DBG, file, line, " seq_no %02x", p[2]); - fr_log(log, L_DBG, file, line, " flags %02x", p[3]); + PRINT(" type %02x", p[1]); + PRINT(" seq_no %02x", p[2]); + PRINT(" flags %02x", p[3]); - fr_log(log, L_DBG, file, line, " sessid %08x", fr_nbo_to_uint32(p + 4)); - fr_log(log, L_DBG, file, line, " length %08x", fr_nbo_to_uint32(p + 8)); + PRINT(" sessid %08x", fr_nbo_to_uint32(p + 4)); + PRINT(" length %08x", fr_nbo_to_uint32(p + 8)); - fr_log(log, L_DBG, file, line, " body"); + PRINT(" body"); length = fr_nbo_to_uint32(p + 8); if ((p[3] & 0x01) == 0) { - fr_log(log, L_DBG, file, line, " ... encrypted ..."); + PRINT(" ... encrypted ..."); return; } if (length > 65535) { - fr_log(log, L_DBG, file, line, " TOO LARGE"); + PRINT(" TOO LARGE"); return; } @@ -428,190 +440,111 @@ void _fr_tacacs_packet_log_hex(fr_log_t const *log, fr_tacacs_packet_t const *pa hdr = p; if ((p + length) != end) { - fr_log(log, L_DBG, file, line, "length field does not match input packet length %08lx", packet_len - 12); + PRINT("length field does not match input packet length %08lx", packet_len - 12); return; } -#define OVERFLOW8(_field, _name) do { \ - if ((p + _field) > end) { \ - fr_log(log, L_DBG, file, line, " " STRINGIFY(_name) " overflows packet!"); \ - return; \ - } \ - p += _field; \ - } while (0) - -#define OVERFLOW16(_field, _name) do { \ - if ((p + fr_nbo_to_uint16(_field)) > end) { \ - fr_log(log, L_DBG, file, line, " " STRINGIFY(_name) " overflows packet!"); \ - return; \ - } \ - p += fr_nbo_to_uint16(_field); \ - } while (0) - #define REQUIRE(_length) do { \ - if ((end - hdr) < _length) { \ - print_hex(log, file, line, " TRUNCATED ", hdr, end - hdr, end); \ + size_t plen = _length; \ + if ((size_t) (end - hdr) < plen) { \ + print_hex(log, file, line, " TRUNCATED ", hdr, end - hdr); \ return; \ } \ + p = hdr + plen; \ } while (0) switch (packet->hdr.type) { default: - print_hex(log, file, line, " data ", p, length, end); + print_hex(log, file, line, " data ", p, length); return; case FR_TAC_PLUS_AUTHEN: if (packet_is_authen_start_request(packet)) { - fr_log(log, L_DBG, file, line, " authentication-start"); + PRINT(" authentication-start"); REQUIRE(8); - fr_log(log, L_DBG, file, line, " action %02x", hdr[0]); - fr_log(log, L_DBG, file, line, " priv_lvl %02x", hdr[1]); - fr_log(log, L_DBG, file, line, " authen_type %02x", hdr[2]); - fr_log(log, L_DBG, file, line, " authen_service %02x", hdr[3]); - fr_log(log, L_DBG, file, line, " user_len %02x", hdr[4]); - fr_log(log, L_DBG, file, line, " port_len %02x", hdr[5]); - fr_log(log, L_DBG, file, line, " rem_addr_len %02x", hdr[6]); - fr_log(log, L_DBG, file, line, " data_len %02x", hdr[7]); - p = hdr + 8; - - /* - * Do some sanity checks on the lengths. - */ - OVERFLOW8(hdr[4], user_len); - OVERFLOW8(hdr[5], port_len); - OVERFLOW8(hdr[6], rem_addr_len); - OVERFLOW8(hdr[7], data_len); - - p = hdr + 8; - if (p >= end) return; + PRINT(" action %02x", hdr[0]); + PRINT(" priv_lvl %02x", hdr[1]); + PRINT(" authen_type %02x", hdr[2]); + PRINT(" authen_service %02x", hdr[3]); + PRINT(" user_len %02x", hdr[4]); + PRINT(" port_len %02x", hdr[5]); + PRINT(" rem_addr_len %02x", hdr[6]); + PRINT(" data_len %02x", hdr[7]); - print_ascii(log, file, line, " user ", p, hdr[4], end); - p += hdr[4]; - - print_ascii(log, file, line, " port ", p, hdr[5], end); - p += hdr[5]; - - print_ascii(log, file, line, " rem_addr ", p, hdr[6], end); - p += hdr[6]; - - print_hex(log, file, line, " data ", p, hdr[7], end); /* common auth flows */ + ASCII(" user ", hdr[4]); + ASCII(" port ", hdr[5]); + ASCII(" rem_addr ", hdr[6]); + HEXIT(" data ", hdr[7]); /* common auth flows */ } else if (packet_is_authen_continue(packet)) { - fr_log(log, L_DBG, file, line, " authentication-continue"); + PRINT(" authentication-continue"); REQUIRE(5); - fr_log(log, L_DBG, file, line, " user_msg_len %04x", fr_nbo_to_uint16(hdr)); - fr_log(log, L_DBG, file, line, " data_len %04x", fr_nbo_to_uint16(hdr + 2)); - fr_log(log, L_DBG, file, line, " flags %02x", hdr[4]); - p = hdr + 5; - - /* - * Do some sanity checks on the lengths. - */ - OVERFLOW16(hdr, user_msg_len); - OVERFLOW16(hdr + 2, data_len); - - p = hdr + 5; - if (p >= end) return; - - print_ascii(log, file, line, " user_msg ", p, fr_nbo_to_uint16(hdr), end); - p += fr_nbo_to_uint16(hdr + 2); + PRINT(" user_msg_len %04x", fr_nbo_to_uint16(hdr)); + PRINT(" data_len %04x", fr_nbo_to_uint16(hdr + 2)); + PRINT(" flags %02x", hdr[4]); - print_hex(log, file, line, " data ", p, fr_nbo_to_uint16(hdr + 2), end); + ASCII(" user_msg ", fr_nbo_to_uint16(hdr)); + HEXIT(" data ", fr_nbo_to_uint16(hdr + 2)); } else { fr_assert(packet_is_authen_reply(packet)); - fr_log(log, L_DBG, file, line, " authentication-reply"); + PRINT(" authentication-reply"); REQUIRE(6); - fr_log(log, L_DBG, file, line, " status %02x", hdr[0]); - fr_log(log, L_DBG, file, line, " flags %02x", hdr[1]); - fr_log(log, L_DBG, file, line, " server_msg_len %04x", fr_nbo_to_uint16(hdr + 2)); - fr_log(log, L_DBG, file, line, " data_len %04x", fr_nbo_to_uint16(hdr + 4)); - p = hdr + 6; + PRINT(" status %02x", hdr[0]); + PRINT(" flags %02x", hdr[1]); + PRINT(" server_msg_len %04x", fr_nbo_to_uint16(hdr + 2)); + PRINT(" data_len %04x", fr_nbo_to_uint16(hdr + 4)); - /* - * Do some sanity checks on the lengths. - */ - OVERFLOW16(hdr + 2, server_msg_len); - OVERFLOW16(hdr + 4, data_len); - - p = hdr + 6; - if (p >= end) return; - - print_ascii(log, file, line, " server_msg ", p, fr_nbo_to_uint16(hdr + 2), end); - p += fr_nbo_to_uint16(hdr + 2); - - print_hex(log, file, line, " data ", p, fr_nbo_to_uint16(hdr + 4), end); + ASCII(" server_msg ", fr_nbo_to_uint16(hdr + 2)); + HEXIT(" data ", fr_nbo_to_uint16(hdr + 4)); } break; case FR_TAC_PLUS_AUTHOR: if (packet_is_author_request(packet)) { - fr_log(log, L_DBG, file, line, " authorization-request"); + PRINT(" authorization-request"); REQUIRE(8); - fr_log(log, L_DBG, file, line, " auth_method %02x", hdr[0]); - fr_log(log, L_DBG, file, line, " priv_lvl %02x", hdr[1]); - fr_log(log, L_DBG, file, line, " authen_type %02x", hdr[2]); - fr_log(log, L_DBG, file, line, " authen_service %02x", hdr[3]); - fr_log(log, L_DBG, file, line, " user_len %02x", hdr[4]); - fr_log(log, L_DBG, file, line, " port_len %02x", hdr[5]); - fr_log(log, L_DBG, file, line, " rem_addr_len %02x", hdr[6]); - fr_log(log, L_DBG, file, line, " arg_cnt %02x", hdr[7]); - p = hdr + 8; + PRINT(" auth_method %02x", hdr[0]); + PRINT(" priv_lvl %02x", hdr[1]); + PRINT(" authen_type %02x", hdr[2]); + PRINT(" authen_service %02x", hdr[3]); + PRINT(" user_len %02x", hdr[4]); + PRINT(" port_len %02x", hdr[5]); + PRINT(" rem_addr_len %02x", hdr[6]); + PRINT(" arg_cnt %02x", hdr[7]); args = p; - OVERFLOW8(hdr[4], user_len); - OVERFLOW8(hdr[5], port_len); - OVERFLOW8(hdr[6], rem_addr_len); - OVERFLOW8(hdr[7], arg_cnt); - - print_hex(log, file, line, " argc ", p, hdr[7], end); - - p = hdr + 8 + hdr[7]; - print_ascii(log, file, line, " user ", p, hdr[4], end); - p += hdr[4]; - - print_ascii(log, file, line, " port ", p, hdr[5], end); - p += hdr[5]; - - print_ascii(log, file, line, " rem_addr ", p, hdr[6], end); - p += hdr[6]; + HEXIT(" argc ", hdr[7]); + ASCII(" user ", hdr[4]); + ASCII(" port ", hdr[5]); + ASCII(" rem_addr ", hdr[6]); print_args(log, file, line, hdr[7], args, p, end); } else { - fr_log(log, L_DBG, file, line, " authorization-reply"); + PRINT(" authorization-reply"); fr_assert(packet_is_author_reply(packet)); REQUIRE(6); - fr_log(log, L_DBG, file, line, " status %02x", hdr[0]); - fr_log(log, L_DBG, file, line, " arg_cnt %02x", hdr[1]); - fr_log(log, L_DBG, file, line, " server_msg_len %04x", fr_nbo_to_uint16(hdr + 2)); - fr_log(log, L_DBG, file, line, " data_len %04x", fr_nbo_to_uint16(hdr + 4)); - p = hdr + 6; + PRINT(" status %02x", hdr[0]); + PRINT(" arg_cnt %02x", hdr[1]); + PRINT(" server_msg_len %04x", fr_nbo_to_uint16(hdr + 2)); + PRINT(" data_len %04x", fr_nbo_to_uint16(hdr + 4)); args = p; - OVERFLOW8(hdr[1], arg_cnt); - OVERFLOW16(hdr + 2, server_msg_len); - OVERFLOW16(hdr + 4, data_len); - - print_hex(log, file, line, " argc ", p, hdr[1], end); - - p = hdr + 6 + hdr[1]; - print_ascii(log, file, line, " server_msg ", p, fr_nbo_to_uint16(hdr + 2), end); - p += fr_nbo_to_uint16(hdr + 2); - - print_ascii(log, file, line, " data ", p, fr_nbo_to_uint16(hdr + 4), end); - p += fr_nbo_to_uint16(hdr + 4); + HEXIT(" argc ", hdr[1]); + ASCII(" server_msg ", fr_nbo_to_uint16(hdr + 2)); + ASCII(" data ", fr_nbo_to_uint16(hdr + 4)); print_args(log, file, line, hdr[1], args, p, end); } @@ -619,67 +552,44 @@ void _fr_tacacs_packet_log_hex(fr_log_t const *log, fr_tacacs_packet_t const *pa case FR_TAC_PLUS_ACCT: if (packet_is_acct_request(packet)) { - fr_log(log, L_DBG, file, line, " accounting-request"); + PRINT(" accounting-request"); REQUIRE(9); - fr_log(log, L_DBG, file, line, " flags %02x", hdr[0]); - fr_log(log, L_DBG, file, line, " auth_method %02x", hdr[1]); - fr_log(log, L_DBG, file, line, " priv_lvl %02x", hdr[2]); - fr_log(log, L_DBG, file, line, " authen_type %02x", hdr[3]); - fr_log(log, L_DBG, file, line, " authen_service %02x", hdr[4]); - fr_log(log, L_DBG, file, line, " user_len %02x", hdr[5]); - fr_log(log, L_DBG, file, line, " port_len %02x", hdr[6]); - fr_log(log, L_DBG, file, line, " rem_addr_len %02x", hdr[7]); - fr_log(log, L_DBG, file, line, " arg_cnt %02x", hdr[8]); - p = hdr + 8; + PRINT(" flags %02x", hdr[0]); + PRINT(" auth_method %02x", hdr[1]); + PRINT(" priv_lvl %02x", hdr[2]); + PRINT(" authen_type %02x", hdr[3]); + PRINT(" authen_service %02x", hdr[4]); + PRINT(" user_len %02x", hdr[5]); + PRINT(" port_len %02x", hdr[6]); + PRINT(" rem_addr_len %02x", hdr[7]); + PRINT(" arg_cnt %02x", hdr[8]); args = p; - OVERFLOW8(hdr[4], arg_cnt); - OVERFLOW8(hdr[5], user_len); - OVERFLOW8(hdr[6], port_len); - OVERFLOW8(hdr[7], rem_addr_len); - - print_hex(log, file, line, " argc ", p, hdr[8], end); - - p = hdr + 8 + hdr[7]; - print_ascii(log, file, line, " user ", p, hdr[4], end); - p += hdr[5]; - - print_ascii(log, file, line, " port ", p, hdr[5], end); - p += hdr[6]; - - print_ascii(log, file, line, " rem_addr ", p, hdr[6], end); - p += hdr[7]; + HEXIT(" argc ", hdr[8]); + ASCII(" user ", hdr[5]); + ASCII(" port ", hdr[6]); + ASCII(" rem_addr ", hdr[7]); print_args(log, file, line, hdr[8], args, p, end); } else { - fr_log(log, L_DBG, file, line, " accounting-reply"); + PRINT(" accounting-reply"); fr_assert(packet_is_acct_reply(packet)); - fr_log(log, L_DBG, file, line, " authentication-reply"); + PRINT(" authentication-reply"); REQUIRE(5); - fr_log(log, L_DBG, file, line, " server_msg_len %04x", fr_nbo_to_uint16(hdr)); - fr_log(log, L_DBG, file, line, " data_len %04x", fr_nbo_to_uint16(hdr + 2)); - fr_log(log, L_DBG, file, line, " status %02x", hdr[0]); - p = hdr + 5; - - /* - * Do some sanity checks on the lengths. - */ - OVERFLOW16(hdr, server_msg_len); - OVERFLOW16(hdr + 2, data_len); + PRINT(" server_msg_len %04x", fr_nbo_to_uint16(hdr)); + PRINT(" data_len %04x", fr_nbo_to_uint16(hdr + 2)); + PRINT(" status %02x", hdr[0]); - p = hdr + 5; - if (p >= end) return; - - print_ascii(log, file, line, " server_msg ", p, fr_nbo_to_uint16(hdr), end); - p += fr_nbo_to_uint16(hdr); - - print_hex(log, file, line, " data ", p, fr_nbo_to_uint16(hdr + 2), end); + ASCII(" server_msg ", fr_nbo_to_uint16(hdr)); + HEXIT(" data ", fr_nbo_to_uint16(hdr + 2)); } break; } + + fr_assert(p == end); }