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

On Tue, Sep 13, 2011 at 05:43:28PM +0000, Christian Röller wrote:
> > Most of the following are just suggestions, though there's one stopper.
> >
> > > -static void hip_netdev_white_list_add_index(int if_index)
> > > +static void hip_netdev_white_list_add_index_and_name(const unsigned int
> > if_index,
> > > + const char *const
> > device_name)
> > > {
> > > if (hip_netdev_white_list_count < HIP_NETDEV_MAX_WHITE_LIST) {
> > > - hip_netdev_white_list[hip_netdev_white_list_count++] = if_index;
> > > + hip_netdev_white_list[hip_netdev_white_list_count].if_index =
> > if_index;
> > > +
> > strncpy(hip_netdev_white_list[hip_netdev_white_list_count].if_label,
> > > + device_name, sizeof(hip_netdev_white_list[0].if_label));
> > > + hip_netdev_white_list_count++;
> >
> > Sorry to bring up this issue again :)
> >
> > I suspect you already know that the strncpy() may leave you with an
> > unterminated if_label here, and you're betting on a sensible length of
> > device_name. See e.g. [1] for an explanation.
> > It's always a good idea to add an assert() when gambling, for the sole
> > purpose of documentation if anything. But better still, I'd terminate
> > the string explicitly.
> >
> > If I'm mistaken here, please comment the code (or the device_name
> > parameter) to clarify. Audit all the other instances of strncpy() you
> > touched, too.
>
> Actually you are right, but in this case the null-termination is guaranteed.
> Because the max interface name length in linux is 15 and I am allocating 16 byte(IF_NAMESIZE)
> for my hip_netdev_whitelist_entry struct. And in strncpy I always write IF_NAMESIZE many bytes,
> so even in the case our system has a maximum interface name of 15, I would write at least one byte
> more and terminate the string in this manner, because strncpy would fill up the buffer with nulls.

So you are essentially saying that if HIPL gets ported to something else
than Linux, this will possibly introduce a subtle and hard to find bug? :)

Diego