UpnpGetIfInfo change global interface name (gIF_NAME) to wrong name

[ingo-h]

I have set a default IPv4 interface "eth0". If called UpnpGetIfInfo(eth0) it provided the information as expected. But if I have for example a typo on the interface name "ethO" with an upper case O instead of a zero, it correctly returns with UPNP_E_INVALID_INTERFACE.

But it has modified the global interface name variable gIF_NAME to wrong "ethO" (with upper 'O'). B.t.w. it has nothing to do with 0 vs. 'O'. It always stores the interface name given as parameter. I can't estimate the impact yet but I don't believe that this is a good state when work is going on with the correct initialized library. I think it is worth to have a look at it.

[mrjimenez]

I agree, does not look good. Indeed, I just found a small bug looking at this code. But back to your point:

/*! Static buffer to contain interface name. (extern'ed in upnp.h) */
char gIF_NAME[LINE_SIZE] = {'\0'};

/*! Static buffer to contain interface IPv4 address. (extern'ed in upnp.h) */
char gIF_IPV4[INET_ADDRSTRLEN] = {'\0'};

/*! Static buffer to contain interface IPv4 netmask. (extern'ed in upnp.h) */
char gIF_IPV4_NETMASK[INET_ADDRSTRLEN] = {'\0'};

/*! Static buffer to contain interface IPv6 link-local address (LLA).
 * (extern'ed in upnp.h) */
char gIF_IPV6[INET6_ADDRSTRLEN] = {'\0'};

/*! IPv6 LLA prefix length. (extern'ed in upnp.h) */
unsigned gIF_IPV6_PREFIX_LENGTH = 0;

/*! Static buffer to contain interface IPv6 unique-local or globally-unique
 * address (ULA or GUA). (extern'ed in upnp.h) */
char gIF_IPV6_ULA_GUA[INET6_ADDRSTRLEN] = {'\0'};

/*! IPv6 ULA or GUA prefix length. (extern'ed in upnp.h) */
unsigned gIF_IPV6_ULA_GUA_PREFIX_LENGTH = 0;

/*! Contains interface index. (extern'ed in upnp.h) */
unsigned gIF_INDEX = (unsigned)-1;

/*! local IPv4 port for the mini-server */
unsigned short LOCAL_PORT_V4;

/*! IPv6 LLA port for the mini-server */
unsigned short LOCAL_PORT_V6;

/*! IPv6 ULA or GUA port for the mini-server */
unsigned short LOCAL_PORT_V6_ULA_GUA;

/*! UPnP device and control point handle table  */
static void *HandleTable[NUM_HANDLE];

I really missed these in my head. This kind of global variables is the main reason why I did 1.8.x back then. The library does not need that kind of global context. E.g., gIF_NAME is actually only used in upnpapi.c::UpnpGetIfInfo(). ssdp_server.c::create_ssdp_sock_v6() just prints its value.

What I propose is the following:

1. Put every global variable in a struct.
2. Create an UpnpNew()/UpnpDelete() pair that would return an opaque pointer to the referred struct, just like we do in lots of other places, e.g. UpnpString.
3. Change the API to include that handler as the first parameter in every call.
4. Extend the API to return these ex-global variables.

Pros:

1. Users get access to this state by means of a clean API.
2. The library would easily become reentrant. This is likely currently an issue with dynamic libraries.

Cons:

1. This is a major API break. But I personally have no problem with that, since fixing is as simple as passing the handler to every call.

What do you think?

[whyman]

Sounds great to me, also similar was requested in #241

[mrjimenez]

That would be a good excuse to release a 1.16.x :laughing:

[ingo-h]

Sounds good but I can't say much with my little knowledge of the library so far. But in general I prefer not to use global variables. They only cause problems.

[wooky]

Cons:

1. This is a major API break. But I personally have no problem with that, since fixing is as simple as passing the handler to every call.

Perhaps, for the sake of backwards-compatibility, create new methods which will accept a handle and leave the old methods as-is.

[ingo-h]

@wooky The library is written in C. It has no classes with methods.

[mrjimenez]

Hi @ingo-h ,

The library is formally C, but not only the UPnP protocol is structured as a DOM, which is something hierarchical, but also many parts of the code use a borrowed construction/destruction/methods approach. Also, there is no problem using the word methods with a library, since the library itself can be considered an object, and the functions that it exports can be thought of as its methods.

In fact, I have thought about rewriting the library in C++ many times, just didn't had spare time to do that. I think it would be a very beautiful code. If you read the UPnP protocol specification, you will see that it practically asks you to write the code in an object oriented way.

@wooky ,

Usually what we do in that situation is to maintain the previous release in a bug-fix-only mode, no new features added, so the branch 1.14.x will be maintained for a while. Keeping two similar APIs seems a little confusing. Also the work necessary to upgrade is close to zero, as you will see when I commit that code. In fact I have already completed converting the file upnpapi.c. Just a few more files and the change will be complete.

Best regards, Marcelo.

[ingo-h]

Hi @mrjimenez, I see more and more that we are nearly on the same path. I thought the library is strictly standard C only. I would very appreciate if we could also use classes and objects [#248].

[ingo-h]

This error is still there even with the new user interface without using global variables. That means it doesn't depend on any global variable. If gtests are running there is a test showing it.

[mrjimenez]

Hi Ingo,

I checked the code, and it turns out that gIF_NAME was actually a local variable of the function UpnpGetIfInfo(). It has later been used in ssdp_server.c for debugging purposes, so we could safely remove it from the generator code. Also, we may choose to leave it there for future debug.

To really fix the issue you report, would mean messing a lot with the UpnpGetIfInfo() function, and since it is not written in a way I consider clean, means that this is something very error prone. In particular, I am referring to the several return paths in the middle of the function that might fail, ideally there should be only one return point to make things simpler to clean. Also do note that gIF_NAME is probably not the only variable that suffers the issue you report, I have not done a deep check, but it seems that gIF_IPV4, gIF_IPV4_NETMASK, gIF_IPV6 and gIF_IPV6_ULA_GUA_PREFIX_LENGTH might get improperly initialized too in some conditions. Also the UpnpGetIfInfo() has two different versions that must be checked, one for windows e one for unix like.

As I said before, a proper fix involves the rewrite of this function. Since accessing gIF_NAME without a positive result from UpnpInit2() is incorrect from the client, and since UpnpInit2() will not return UPNP_E_SUCCESS, unless UpnpGetIfInfo() also returns UPNP_E_SUCCESS, there should be no problem here if the client is reasonable.

I am not against a proper fix, but as you can see it is a lot of work and testing, and maybe we don't gain enough in return.

[ingo-h]

I will improve the gtests for UpnpGetIfInfo() so that we catch all (known) conditions and haven't to worry about with modifing its internal code. Improving tests is mainly to provide more than one (mocked) network interface with gtest/tools.