From: Klement Sekera Date: Thu, 2 Dec 2021 16:36:34 +0000 (+0000) Subject: api: improve REPLY_MACRO safety X-Git-Tag: v22.06-rc0~138 X-Git-Url: https://gerrit.fd.io/r/gitweb?a=commitdiff_plain;h=0eb83f484cc74b7ab4e26f6811a3e98442a25e7d;p=vpp.git api: improve REPLY_MACRO safety Improve vppapigen to generate per-message #define indicating whether said message is dynamically sized (due to VLA or string) or not. Use these #defines in REPLY_MACROs to prevent improper usage. Fix existing improper REPLY_MACRO* usage. Type: improvement Change-Id: Ia77aaf9f6cf3ed68ea21075a4cc8deda78a68651 Signed-off-by: Klement Sekera --- diff --git a/src/tools/vppapigen/vppapigen_c.py b/src/tools/vppapigen/vppapigen_c.py index d0f63fbdb0c..1e18a83e65e 100644 --- a/src/tools/vppapigen/vppapigen_c.py +++ b/src/tools/vppapigen/vppapigen_c.py @@ -24,6 +24,7 @@ VAT2 tests. ''' import datetime +import itertools import os import time import sys @@ -1256,7 +1257,7 @@ def generate_include_types(s, module, stream): filename = i.filename.replace('plugins/', '') write('#include <{}_types.h>\n'.format(filename)) - for o in s['types'] + s['Define']: + for o in itertools.chain(s['types'], s['Define']): tname = o.__class__.__name__ if tname == 'Using': if 'length' in o.alias: @@ -1315,6 +1316,7 @@ def generate_include_types(s, module, stream): .format(b, o.name)) write('} vl_api_%s_t;\n' % o.name) + write(f'#define VL_API_{o.name.upper()}_IS_CONSTANT_SIZE ({0 if o.vla else 1})\n\n') for t in s['Define']: write('#define VL_API_{ID}_CRC "{n}_{crc:08x}"\n' diff --git a/src/vlibapi/api_helper_macros.h b/src/vlibapi/api_helper_macros.h index 5c4dd9a6fd5..6b5b37ae746 100644 --- a/src/vlibapi/api_helper_macros.h +++ b/src/vlibapi/api_helper_macros.h @@ -33,20 +33,26 @@ endian_fp = am->msg_endian_handlers[t + (REPLY_MSG_ID_BASE)]; \ (*endian_fp) (rmp); -#define REPLY_MACRO(t) \ -do { \ - vl_api_registration_t *rp; \ - rp = vl_api_client_index_to_registration (mp->client_index); \ - if (rp == 0) \ - return; \ - \ - rmp = vl_msg_api_alloc (sizeof (*rmp)); \ - rmp->_vl_msg_id = htons((t)+(REPLY_MSG_ID_BASE)); \ - rmp->context = mp->context; \ - rmp->retval = ntohl(rv); \ - \ - vl_api_send_msg (rp, (u8 *)rmp); \ -} while(0); +#define REPLY_MACRO(msg) \ + do \ + { \ + STATIC_ASSERT ( \ + msg##_IS_CONSTANT_SIZE, \ + "REPLY_MACRO can only be used with constant size messages, " \ + "use REPLY_MACRO[3|4]* instead"); \ + vl_api_registration_t *rp; \ + rp = vl_api_client_index_to_registration (mp->client_index); \ + if (rp == 0) \ + return; \ + \ + rmp = vl_msg_api_alloc (sizeof (*rmp)); \ + rmp->_vl_msg_id = htons (msg + (REPLY_MSG_ID_BASE)); \ + rmp->context = mp->context; \ + rmp->retval = ntohl (rv); \ + \ + vl_api_send_msg (rp, (u8 *) rmp); \ + } \ + while (0); #define REPLY_MACRO_END(t) \ do \ @@ -65,20 +71,30 @@ do { \ } \ while (0); -#define REPLY_MACRO2(t, body) \ -do { \ - vl_api_registration_t *rp; \ - rp = vl_api_client_index_to_registration (mp->client_index); \ - if (rp == 0) \ - return; \ - \ - rmp = vl_msg_api_alloc (sizeof (*rmp)); \ - rmp->_vl_msg_id = htons((t)+(REPLY_MSG_ID_BASE)); \ - rmp->context = mp->context; \ - rmp->retval = ntohl(rv); \ - do {body;} while (0); \ - vl_api_send_msg (rp, (u8 *)rmp); \ -} while(0); +#define REPLY_MACRO2(t, body) \ + do \ + { \ + STATIC_ASSERT ( \ + t##_IS_CONSTANT_SIZE, \ + "REPLY_MACRO2 can only be used with constant size messages, " \ + "use REPLY_MACRO[3|4]* instead"); \ + vl_api_registration_t *rp; \ + rp = vl_api_client_index_to_registration (mp->client_index); \ + if (rp == 0) \ + return; \ + \ + rmp = vl_msg_api_alloc (sizeof (*rmp)); \ + rmp->_vl_msg_id = htons ((t) + (REPLY_MSG_ID_BASE)); \ + rmp->context = mp->context; \ + rmp->retval = ntohl (rv); \ + do \ + { \ + body; \ + } \ + while (0); \ + vl_api_send_msg (rp, (u8 *) rmp); \ + } \ + while (0); #define REPLY_MACRO2_END(t, body) \ do \ @@ -230,20 +246,26 @@ do { \ } \ while (0); -#define REPLY_MACRO3(t, n, body) \ -do { \ - vl_api_registration_t *rp; \ - rp = vl_api_client_index_to_registration (mp->client_index); \ - if (rp == 0) \ - return; \ - \ - rmp = vl_msg_api_alloc (sizeof (*rmp) + n); \ - rmp->_vl_msg_id = htons((t)+(REPLY_MSG_ID_BASE)); \ - rmp->context = mp->context; \ - rmp->retval = ntohl(rv); \ - do {body;} while (0); \ - vl_api_send_msg (rp, (u8 *)rmp); \ -} while(0); +#define REPLY_MACRO3(t, n, body) \ + do \ + { \ + vl_api_registration_t *rp; \ + rp = vl_api_client_index_to_registration (mp->client_index); \ + if (rp == 0) \ + return; \ + \ + rmp = vl_msg_api_alloc (sizeof (*rmp) + (n)); \ + rmp->_vl_msg_id = htons ((t) + (REPLY_MSG_ID_BASE)); \ + rmp->context = mp->context; \ + rmp->retval = ntohl (rv); \ + do \ + { \ + body; \ + } \ + while (0); \ + vl_api_send_msg (rp, (u8 *) rmp); \ + } \ + while (0); #define REPLY_MACRO3_END(t, n, body) \ do \ @@ -619,62 +641,65 @@ bad_bd_id: \ ; \ } while (0); -#define pub_sub_handler(lca,UCA) \ -static void vl_api_want_##lca##_t_handler ( \ - vl_api_want_##lca##_t *mp) \ -{ \ - vpe_api_main_t *vam = &vpe_api_main; \ - vpe_client_registration_t *rp; \ - vl_api_want_##lca##_reply_t *rmp; \ - uword *p; \ - i32 rv = 0; \ - \ - p = hash_get (vam->lca##_registration_hash, mp->client_index); \ - if (p) { \ - if (mp->enable_disable) { \ - clib_warning ("pid %d: already enabled...", ntohl(mp->pid)); \ - rv = VNET_API_ERROR_INVALID_REGISTRATION; \ - goto reply; \ - } else { \ - rp = pool_elt_at_index (vam->lca##_registrations, p[0]); \ - pool_put (vam->lca##_registrations, rp); \ - hash_unset (vam->lca##_registration_hash, \ - mp->client_index); \ - goto reply; \ - } \ - } \ - if (mp->enable_disable == 0) { \ - clib_warning ("pid %d: already disabled...", mp->pid); \ - rv = VNET_API_ERROR_INVALID_REGISTRATION; \ - goto reply; \ - } \ - pool_get (vam->lca##_registrations, rp); \ - rp->client_index = mp->client_index; \ - rp->client_pid = mp->pid; \ - hash_set (vam->lca##_registration_hash, rp->client_index, \ - rp - vam->lca##_registrations); \ - \ -reply: \ - REPLY_MACRO (VL_API_WANT_##UCA##_REPLY); \ -} \ - \ -static clib_error_t * vl_api_want_##lca##_t_reaper (u32 client_index) \ -{ \ - vpe_api_main_t *vam = &vpe_api_main; \ - vpe_client_registration_t *rp; \ - uword *p; \ - \ - p = hash_get (vam->lca##_registration_hash, client_index); \ - if (p) \ - { \ - rp = pool_elt_at_index (vam->lca##_registrations, p[0]); \ - pool_put (vam->lca##_registrations, rp); \ - hash_unset (vam->lca##_registration_hash, client_index); \ - } \ - return (NULL); \ -} \ - \ -VL_MSG_API_REAPER_FUNCTION (vl_api_want_##lca##_t_reaper); \ +#define pub_sub_handler(lca, UCA) \ + static void vl_api_want_##lca##_t_handler (vl_api_want_##lca##_t *mp) \ + { \ + vpe_api_main_t *vam = &vpe_api_main; \ + vpe_client_registration_t *rp; \ + vl_api_want_##lca##_reply_t *rmp; \ + uword *p; \ + i32 rv = 0; \ + \ + p = hash_get (vam->lca##_registration_hash, mp->client_index); \ + if (p) \ + { \ + if (mp->enable_disable) \ + { \ + clib_warning ("pid %d: already enabled...", ntohl (mp->pid)); \ + rv = VNET_API_ERROR_INVALID_REGISTRATION; \ + goto reply; \ + } \ + else \ + { \ + rp = pool_elt_at_index (vam->lca##_registrations, p[0]); \ + pool_put (vam->lca##_registrations, rp); \ + hash_unset (vam->lca##_registration_hash, mp->client_index); \ + goto reply; \ + } \ + } \ + if (mp->enable_disable == 0) \ + { \ + clib_warning ("pid %d: already disabled...", mp->pid); \ + rv = VNET_API_ERROR_INVALID_REGISTRATION; \ + goto reply; \ + } \ + pool_get (vam->lca##_registrations, rp); \ + rp->client_index = mp->client_index; \ + rp->client_pid = mp->pid; \ + hash_set (vam->lca##_registration_hash, rp->client_index, \ + rp - vam->lca##_registrations); \ + \ + reply: \ + REPLY_MACRO (VL_API_WANT_##UCA##_REPLY); \ + } \ + \ + static clib_error_t *vl_api_want_##lca##_t_reaper (u32 client_index) \ + { \ + vpe_api_main_t *vam = &vpe_api_main; \ + vpe_client_registration_t *rp; \ + uword *p; \ + \ + p = hash_get (vam->lca##_registration_hash, client_index); \ + if (p) \ + { \ + rp = pool_elt_at_index (vam->lca##_registrations, p[0]); \ + pool_put (vam->lca##_registrations, rp); \ + hash_unset (vam->lca##_registration_hash, client_index); \ + } \ + return (NULL); \ + } \ + \ + VL_MSG_API_REAPER_FUNCTION (vl_api_want_##lca##_t_reaper); #define foreach_registration_hash \ _(interface_events) \ diff --git a/src/vnet/session/session_api.c b/src/vnet/session/session_api.c index 2121d2075e6..767a24aa170 100644 --- a/src/vnet/session/session_api.c +++ b/src/vnet/session/session_api.c @@ -696,25 +696,28 @@ vl_api_app_attach_t_handler (vl_api_app_attach_t * mp) done: /* *INDENT-OFF* */ - REPLY_MACRO2 (VL_API_APP_ATTACH_REPLY, ({ - if (!rv) - { - ctrl_thread = n_workers ? 1 : 0; - segp = (fifo_segment_t *) a->segment; - rmp->app_index = clib_host_to_net_u32 (a->app_index); - rmp->app_mq = fifo_segment_msg_q_offset (segp, 0); - rmp->vpp_ctrl_mq = fifo_segment_msg_q_offset (rx_mqs_seg, ctrl_thread); - rmp->vpp_ctrl_mq_thread = ctrl_thread; - rmp->n_fds = n_fds; - rmp->fd_flags = fd_flags; - if (vec_len (segp->ssvm.name)) - { - vl_api_vec_to_api_string (segp->ssvm.name, &rmp->segment_name); - } - rmp->segment_size = segp->ssvm.ssvm_size; - rmp->segment_handle = clib_host_to_net_u64 (a->segment_handle); - } - })); + REPLY_MACRO3 ( + VL_API_APP_ATTACH_REPLY, + ((!rv) ? vec_len (((fifo_segment_t *) a->segment)->ssvm.name) : 0), ({ + if (!rv) + { + ctrl_thread = n_workers ? 1 : 0; + segp = (fifo_segment_t *) a->segment; + rmp->app_index = clib_host_to_net_u32 (a->app_index); + rmp->app_mq = fifo_segment_msg_q_offset (segp, 0); + rmp->vpp_ctrl_mq = + fifo_segment_msg_q_offset (rx_mqs_seg, ctrl_thread); + rmp->vpp_ctrl_mq_thread = ctrl_thread; + rmp->n_fds = n_fds; + rmp->fd_flags = fd_flags; + if (vec_len (segp->ssvm.name)) + { + vl_api_vec_to_api_string (segp->ssvm.name, &rmp->segment_name); + } + rmp->segment_size = segp->ssvm.ssvm_size; + rmp->segment_handle = clib_host_to_net_u64 (a->segment_handle); + } + })); /* *INDENT-ON* */ if (n_fds) @@ -780,22 +783,25 @@ vl_api_app_worker_add_del_t_handler (vl_api_app_worker_add_del_t * mp) /* *INDENT-OFF* */ done: - REPLY_MACRO2 (VL_API_APP_WORKER_ADD_DEL_REPLY, ({ - rmp->is_add = mp->is_add; - rmp->wrk_index = clib_host_to_net_u32 (args.wrk_map_index); - rmp->segment_handle = clib_host_to_net_u64 (args.segment_handle); - if (!rv && mp->is_add) - { - rmp->app_event_queue_address = - fifo_segment_msg_q_offset ((fifo_segment_t *) args.segment, 0); - rmp->n_fds = n_fds; - rmp->fd_flags = fd_flags; - if (vec_len (args.segment->name)) - { - vl_api_vec_to_api_string (args.segment->name, &rmp->segment_name); - } - } - })); + REPLY_MACRO3 ( + VL_API_APP_WORKER_ADD_DEL_REPLY, + ((!rv && mp->is_add) ? vec_len (args.segment->name) : 0), ({ + rmp->is_add = mp->is_add; + rmp->wrk_index = clib_host_to_net_u32 (args.wrk_map_index); + rmp->segment_handle = clib_host_to_net_u64 (args.segment_handle); + if (!rv && mp->is_add) + { + rmp->app_event_queue_address = + fifo_segment_msg_q_offset ((fifo_segment_t *) args.segment, 0); + rmp->n_fds = n_fds; + rmp->fd_flags = fd_flags; + if (vec_len (args.segment->name)) + { + vl_api_vec_to_api_string (args.segment->name, + &rmp->segment_name); + } + } + })); /* *INDENT-ON* */ if (n_fds)