Hi, Aaron
On Fri, Jun 6, 2025 at 7:31 PM Aaron Conole <acon...@redhat.com> wrote:
>
> Changliang Wu <changliang...@smartx.com> writes:
>
> > 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>
> > ---
>
> Hi Changliang,
>
> Thanks for the test and the fix.
>
> Comments inline.
>
> > 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);
> > -
>
> ^ Why this line change?
>
> > 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]);
> > + }
>
> Wouldn't it make sense instead to increase the length we allocate by
> one? I guess the concern may be we have a buffer that will always be
> one byte larger.
>
> Actually, going one step further, we can eliminate the need to do the
> separate allocation like we're doing. The chassisid_to_string is only
> used when building a dynamic string in the first place. So, we can
> use a dynamic string object that should do the right thing. Something
> like the following may work:
Dynamic string should be a better choice, I will rewrite this part.
>
> void
> chassisid_to_string(uint8_t *array, size_t len, struct ds *str)
> {
> size_t i;
>
> for (i = 0; i < len; i++) {
> ds_put_format(str, "%02x", array[i]);
> if (i != len - 1) {
> ds_put_char(str, ':');
> }
> }
> }
>
> And then adjust the calls accordingly (so we can eliminate 'id' and
> 'system' pointers in `aa_print_element_status_port`).
>
> > }
> > - (*str)[(i * 3) - 1] = '\0';
>
> ^ this is a separate cleanup.
>
> > }
> >
> > /* 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 **);
> >
> > #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;
> > +}
>
> If you adopt the dynamic string approach, this test will need to be
> adjusted slightly.
Sure.
>
> > 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();
> >
> > /* Add more tests here */
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
