[netcf-devel] [PATCH 1/5] Windows port of netcf api calls

Laine Stump laine at laine.org
Thu Sep 23 12:06:42 UTC 2010


  On 09/22/2010 01:28 PM, Eric Blake wrote:
> On 09/17/2010 10:28 AM, Adam Stokes wrote:
>> +PMIB_IPADDRTABLE _get_ip_addr_table(PMIB_IPADDRTABLE ipAddrTable) {
> Since this function starts with an underscore, should it be marked
> static?  That is, it is seldom wise to export a name with a leading
> underscore.
>
> I'm not sure if netcf has a particular style, but I prefer:
>
> PMIB_IPADDRTABLE
> _get_ip_addr_table(PMIB_IPADDRTABLE ipAddrTable)
> {
>
> That is, separate the lines for the return type and function name, and
> start the open brace on it's own line at the implementation (while
> keeping the return type and name on the same line for forward
> declarations).  Why? Because some tools, including emacs and git diff,
> are hard-coded to recognize this pattern, making it easier to quickly
> locate a function implementation by filtering based on '^_get_ip_addr'
> without having to filter through all the call sites.
>
> But libvirt does not strictly follow this style, and unless netcf has a
> policy, your format does not need to be changed.


Actually netcf's source mostly has the style that Adam has used here. I 
prefer yet a 3rd style (and rely on more intelligent tools to do keyword 
searches ;-) so I tend to just go with whatever is already there :-)


>> +    DWORD r = 0;
>> +    DWORD buf = 0;
>> +
>> +    ipAddrTable = (PMIB_IPADDRTABLE) malloc(sizeof (MIB_IPADDRTABLE));
> I much prefer:
>
> ipAddrTable = malloc(sizeof (*ipAddrTable));
>
> In C, the cast of malloc results is redundant, and using *var instead of
> (type) in the sizeof makes you immune to changing the type of var.
>
> Maybe it would make sense as a separate patch to mimic libvirt's wrapper
> macro VIR_ALLOC, which also sets a compiler attribute to warn if you
> failed to check for ENOMEM condition...


Existing netcf code uses ALLOC, ALLOC_N, REALLOC_N, and FREE from 
gnulib's safe-alloc.h. Since they're already being pulled in anyway, 
it's best to use these for all allocations rather than straight malloc..


>> +	return ipAddrTable;
>> +
>> + error:
>> +    free(ipAddrTable);
>> +    return ipAddrTable;
> Ouch.  This returns a stale pointer, which is a no-no.  Return NULL on
> failure.  Better yet, should netcf have a wrapper mimicking libvirt's
> VIR_FREE(var), which guarantees that var is set to NULL after releasing
> the memory?


Use FREE(var) from gnulib safe-alloc.h (existing netcf code tends to do 
that only where there's a possibility that the pointer may be used later 
on. I tend to like using it always, even when I know for certain that 
the pointer won't be used again, to help prevent future bugs (won't 
gcc's optimization remove the potential dead store anyway?)


>
>> +
>> +/* Create a new netcf if instance for interface NAME */
>> +struct netcf_if *make_netcf_if(struct netcf *ncf, char *name) {
> Should this be const char *name?

The original function used char*, probably because ownership is 
transferred to the netcf object, and it's later freed by somebody else.

>> +    int r;
>> +    struct netcf_if *result = NULL;
>> +
>> +    r = make_ref(result);
>> +    ERR_NOMEM(r<   0, ncf);
>> +    result->ncf = ref(ncf);
>> +    result->name = name;
> Should this be a strdup() of name?


The Linux version of this function expects the caller to strdup name (if 
necessary). the netcf object takes ownership of the string.

Looking at that code, it appears that the Linux version actually leaks 
the name when there is an ENOMEM during make_netcf_if()! (just made a 
note of that and will fix it)

>> +    return result;
>> +
>> + error:
>> +    unref(result, netcf_if);
>> +    return result;
>> +}
>> +
>> +static int list_interface_ids(struct netcf *ncf ATTRIBUTE_UNUSED,
>> +				  int maxnames ATTRIBUTE_UNUSED,
>> +				  char **names, unsigned int flags ATTRIBUTE_UNUSED,
>> +				  const char *id_attr ATTRIBUTE_UNUSED) {
> For extensibility reasons, it's always better to check that flags == 0,
> rather than marking it as ATTRIBUTE_UNUSED - that way, if a future
> extension comes up with a reasonable flag, but then calls older code
> that does not know how to handle the flag, you gracefully exit with
> error instead of proceeding without acting on the flag.


Actually the flags are important here, and should already be used - the 
netcf API specifies NETCF_IFACE_INACTIVE and NETCF_IFACE_ACTIVE flags 
that are used to pick whether to list only interfaces that are up, only 
those that are down, or both. If this isn't handled elsewhere in the 
Windows version, it needs to be handled at this level (and the 
corresponding higher level functions need to pass flags down frmo the 
API function rather than tossing them and sending 0.

For that matter, I don't understand how this even compiles - all the 
args are marked as ATTRIBUTE_UNUSED, yet "names" is actually used.


>> +    unsigned int nint = 0;
>> +
>> +    PIP_ADAPTER_ADDRESSES addrList = NULL;
>> +    PIP_ADAPTER_ADDRESSES adapterp = NULL;
>> +    adapterp = _get_ip_adapter_info(addrList);
>> +    for (nint = 0; adapterp != NULL; nint++) {
> Potential buffer overflow - how do you know that you are not stepping
> beyond maxnames positions in the names array?  maxnames should not be
> ATTRIBUTE_UNUSED.
>
>> +	if (names) {
>> +	    char name[BUFSIZE];
>> +	    WideCharToMultiByte(CP_UTF8, 0, adapterp->FriendlyName,
>> +				-1, name, sizeof(name), NULL, NULL);
> Can this conversion ever fail?
>
>> +	    names[nint] = strdup(name);
> Check for memory allocation failure.


BTW, you may find it useful to use the ERR_NOMEM() macro for this - 
there are numerous examples in the existing code. Likewise, using all 
the other ERR_*() macros would make for nice consistent code. It's a bit 
odd to get used to the lack of conditionals when checking for an error 
(the conditional is implied as the first argument of most of the 
macros), but makes sure that all the right stuff gets set in the netcf 
object when an error occurs. Having that done also makes it easier to 
bail out of higher level functions on return from something that failed 
(just put an error: label in the function, and invoke "ERR_BAIL(ncf)" 
after returning from the lower level function).


>> +	}
>> +	adapterp = adapterp->Next;
>> +    }
>> +    return nint;
>> + error:
>> +    free(addrList);
> Memory leak corner case - if you allocate some, but not all, of
> names[nint], then you need to free the ones you did allocate.
>
>> +    return -1;
>> +}
>> +
>> +int drv_list_interfaces(struct netcf *ncf,
>> +		    int maxnames ATTRIBUTE_UNUSED, char **names,
>> +		    unsigned int flags ATTRIBUTE_UNUSED) {
>> +    return list_interface_ids(ncf, 0, names, 0, NULL);
> Shouldn't this pass maxnames and flags on through?


Yes (again, unless it's handled at a higher level). The caller of the 
API-level ncf_list_interfaces() is expected to have allocated an array 
of char* prior to calling. It should be large enough to hold pointers to 
the names of all of the interfaces (or as many as the caller wants to 
see). (see ncftool.c for an example of calling ncf_list_interfaces)


>> +}
>> +
>> +
>> +int drv_num_of_interfaces(struct netcf *ncf, unsigned int flags ATTRIBUTE_UNUSED) {
>> +    return list_interface_ids(ncf, 0, NULL, 0, NULL);
> Shouldn't this pass flags on through?


Yes (unless Windows has no concept of an interface being up or down :-P )


>> +}
>> +
>> +
>> +struct netcf_if *drv_lookup_by_name(struct netcf *ncf, const char *name) {
>> +    struct netcf_if *nif = NULL;
>> +    char *name_dup;
>> +    unsigned int nint = 0;
>> +    PIP_ADAPTER_ADDRESSES addrList = NULL;
>> +    PIP_ADAPTER_ADDRESSES adapterp = NULL;
>> +    adapterp = _get_ip_adapter_info(addrList);
>> +    for (nint = 0; adapterp != NULL; nint++) {
>> +	if (name) {
>> +	    char wName[BUFSIZE];
>> +	    WideCharToMultiByte(CP_UTF8, 0, adapterp->FriendlyName,
>> +				-1, wName, sizeof(wName), NULL, NULL);
>> +	    if (strcmp(wName, name) == 0) {
>> +		name_dup = strdup(wName);
>> +		nif = make_netcf_if(ncf, name_dup);
> Does this leak name_dup?

Not unless make_netcf_if fails - it expects to take ownership of the 
name, so the caller shouldn't have to free it. Of course, that leaves 
the problem of leaking when make_netcf_if fails, which as I said I just 
discovered is also a problem in the original Linux version.


>
>> +			sprintf(ipBuf,"%d",inet_ntoa(ipAddr));
> Where is the guarantee that ipBuf is big enough for this sprintf, or is
> there a possibility of buffer overflow?


Beyond that, I'm confused about what this sprintf thinks it's printing - 
it looks to me like it's printing out the decimal conversion of the 
*address* of a char* that contains an ASCII representation of an IP 
address. ???

BTW, note that inet_ntoa is not threadsafe (the same static buffer is 
reused by all callers in all threads of the process), so it shouldn't be 
used. Better to use inet_ntop instead.

>> +		     "%d.%d.%d.%d", (ip>>   0)&   255, (ip>>   8)&   255,
>> +		     (ip>>   16)&   255, (ip>>   24)&   255);
> Maybe it's just me, but I prefer 0xff rather than 255 when masking bits.
>    If we could guarantee that gnulib gives us %hhd support, you could
> avoid the mask (and just do the shift).


Why not use inet_ntop() instead?


>> +#ifndef NETCF_WIN_H
>> +#define NETCF_WIN_H
>> +
>> +#ifndef WINVER
>> +#define WINVER 0x0501
> I like using cppi, where this would be:
>
> #ifndef WINVER
> # define WINVER 0x0501
> #endif
>
> but I don't know if netcf plans on copying libvirt's lead in this style
> matter.

I don't think anybody had ever thought about it, but personally wouldn't 
mind - "patches welcome" :-)




More information about the netcf-devel mailing list