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

I had to limit the buffer length somehow, so I choose an OS-specific characteristic.
Is there a better way?

But isn't the whole whitelisting approach very os-specific(in this case linux)? I mean we base the whole whitelisting on OS-specific characteristics like interface-indeces and interface-label-names.
So maybe the whole whitelisting approach is a bug in this manner!? :)

But to be honest, I have no other idea than to treat the whitelisting separately for every OS. You?