On 6/6/25 8:39 AM, Changliang Wu wrote:
> snprintf will automatically write \0 at the end of the string,
> and the last one byte will be out of bound.
>
> Found in sanitize test.
>
> Signed-off-by: Changliang Wu <changliang...@smartx.com>
> ---
> lib/ovs-lldp.c | 10 ++++++----
> lib/ovs-lldp.h | 1 +
> tests/test-aa.c | 14 ++++++++++++++
> 3 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/lib/ovs-lldp.c b/lib/ovs-lldp.c
> index 2d13e971e..a0b7990b4 100644
> --- a/lib/ovs-lldp.c
> +++ b/lib/ovs-lldp.c
> @@ -103,17 +103,19 @@ static struct lldp_aa_element_system_id system_id_null;
>
> /* Convert an LLDP chassis ID to a string.
> */
> -static void
> +void
> chassisid_to_string(uint8_t *array, size_t len, char **str)
> {
> unsigned int i;
>
> *str = xmalloc(len * 3);
I think, we should just allocate one more byte and not change the rest
of the function.
Alternative is to use dynamic string instead as format_hex_arg() does.
In this case, we should create a new util function that accepts a
delimiter and call it from both chassisid_to_string() and the
format_hex_arg().
> -
> for (i = 0; i < len; i++) {
> - snprintf(&(*str)[i * 3], 4, "%02x:", array[i]);
> + if (i < len - 1) {
> + snprintf(&(*str)[i * 3], 4, "%02x:", array[i]);
> + } else {
> + snprintf(&(*str)[i * 3], 3, "%02x", array[i]);
> + }
> }
> - (*str)[(i * 3) - 1] = '\0';
> }
>
> /* Find an Auto Attach mapping keyed by I-SID.
> diff --git a/lib/ovs-lldp.h b/lib/ovs-lldp.h
> index 661ac4e18..a3160c58d 100644
> --- a/lib/ovs-lldp.h
> +++ b/lib/ovs-lldp.h
> @@ -105,5 +105,6 @@ int aa_mapping_unregister(void *aux);
> /* Used by unit tests */
> struct lldp * lldp_create_dummy(void);
> void lldp_destroy_dummy(struct lldp *);
> +void chassisid_to_string(uint8_t *, size_t, char **);
If you're exporting this function, it should have an lldp_ prefix.
>
> #endif /* OVS_LLDP_H */
> diff --git a/tests/test-aa.c b/tests/test-aa.c
> index 1c0fb2926..0c2970ec2 100644
> --- a/tests/test-aa.c
> +++ b/tests/test-aa.c
> @@ -270,6 +270,19 @@ test_aa_send(void)
> return 0;
> }
>
> +static int
> +test_aa_chassisid_to_string(void)
> +{
> + char *id = NULL;
> + char id_str[] = "01:02:03:04:05:06";
> + uint8_t array[6] = { 0x01, 0x02, 0x03, 0x04, 0x05, 0x06 };
> + chassisid_to_string(array, 6, &id);
> +
> + int ret = strcmp(id, id_str);
> + free(id);
> +
> + return ret;
> +}
>
> static void
> test_aa_main(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
> @@ -279,6 +292,7 @@ test_aa_main(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
> /* Make sure we emit valid auto-attach LLDPPDUs */
> num_tests++;
> num_errors += test_aa_send();
> + num_errors += test_aa_chassisid_to_string();
You missed increasing the num_tests. Also, this test is not about
LLDPPDUs, so it should be separated with at least a blank line from
the previous one.
On the other hand, the change may not even need a unit test. The
larger problem, it seems, that we don't have any test coverage for
the autoattach/status command that would catch the issue as well.
>
> /* Add more tests here */
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
