On 1/9/24 19:16, Han Zhou wrote:
On Tue, Jan 9, 2024 at 10:32 AM Mark Michelson <[email protected]
<mailto:[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
<https://issues.redhat.com/browse/FDP-224>
>
> Signed-off-by: Mark Michelson <[email protected]
<mailto:[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
Thanks Han, that makes total sense. I will make this change in v2.
> 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] <mailto:[email protected]>
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
<https://mail.openvswitch.org/mailman/listinfo/ovs-dev>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
