Code review comment for lp:~christian-roeller/hipl/whitelisting

> Hi Christian!

Hi

>
> Sorry for coming back late with required fixes :-(

no problem...

>
> > /**
> > + * Gives you the interface label for a given IPv4 or IPv6 address.
> > + *
> > + * @param addr address for which you want to know the label
> > + * @param label pointer where the function stores the label
> > + * @return zero on success, one on error and write the label in param label
> > + * if the given address exists in the system
> > + */
> > +int hip_find_label_for_address(const struct sockaddr *const addr,
> > + char *const label)
>
> [M] safety: this function is not very safe in how it handles the length of the
> memory buffer pointed to by 'label'. Please either document clearly how large
> the memory buffer needs to be and what happens if it is not or add code that
> helps to avoid buffer overflows here.

What is exactly the safety problem here? Do you mean I should additionally mention that label
sould be at least as big as IF_NAMESIZE, so that every possible interfacelabel length would fit?
Because right now I check whether the found interfacelabel is not bigger than the associated memory for label. And only if this is the case I will write to label. And I never write more than sizeof(label) bytes to label. Is this not enough to avoid a buffer overflow?
Right now the function will just return 1 if label would be to small. Maybe I should introduce here a special return value to make the debugging easier for the caller. What do you think?

>
> [M] policy: since this is a new function, it needs to be checked by a unit
> test. I know it's a PITA but unit tests, for example, immediately point to
> such issues as the memory buffer handling.
>
> Regards,
> Stefan

Christian

« Back to merge proposal