On Tue, Jan 9, 2024 at 10:32 AM Mark Michelson <[email protected]> wrote:
>
> A static analyzer determined that if pb->n_mac was 0, then the c_addrs
> lport_addresses struct would never be initialized. We would then use
> and attempt to free uninitialized memory.
>
> In reality, pb->n_mac will always be 1. This is because the port binding
is a
> representation of a northbound logical router port. Logical router ports
do
> not contain an array of MACs like the southbound port bindings do.
Instead,
> they have a single MAC string that is always guaranteed to be non-NULL.
This
> string is copied into the port binding's MAC array. Therefore, a
southbound
> port binding that comes from a logical router port will always have n_mac
set
> to 1.
>
> How do we know this is a logical router port? The ports iterated over in
this
> function must have IPv6 prefix delegation configured on them. Only
northbound
> logical router ports have this option available.
>
> To silence the static analyzer, this change directly retrieves pb->mac[0]
> instead of iterating over the pb->mac array. It also adds an assertion
that
> pb->n_mac == 1. This arguably makes the code's intent more clear as
> well.
>
> Reported-at: https://issues.redhat.com/browse/FDP-224
>
> Signed-off-by: Mark Michelson <[email protected]>
> ---
> controller/pinctrl.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index 12055a675..d5957ad69 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -1286,11 +1286,15 @@ fill_ipv6_prefix_state(struct ovsdb_idl_txn
*ovnsb_idl_txn,
> continue;
> }
>
> + /* To reach this point, the port binding must be a logical router
> + * port. LRPs are configured with a single MAC that is always
non-NULL.
> + * Therefore, we can always safely extract pb->mac[0] since it
will be
> + * non-NULL
> + */
> + ovs_assert(pb->n_mac == 1);
Thanks Mark for the fix. I think we shouldn't use assert here, since the DB
status is not controlled by ovn-controller and it is not good to crash the
ovn-controller when the mac column is cleared by an external tool for
whatever reason. Assertion should be used when we are 100% sure about the
assumption from developer's point of view except when there is a bug in the
current component. In this case I think it is better to print an error and
skip, if not doing more complex error handling.
Regards,
Han
> struct lport_addresses c_addrs;
> - for (size_t j = 0; j < pb->n_mac; j++) {
> - if (extract_lsp_addresses(pb->mac[j], &c_addrs)) {
> - break;
> - }
> + if (!extract_lsp_addresses(pb->mac[0], &c_addrs)) {
> + continue;
> }
>
> pfd = shash_find_data(&ipv6_prefixd, pb->logical_port);
> --
> 2.40.1
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
