jpirko / libteam

team netdevice library
GNU Lesser General Public License v2.1
225 stars 57 forks source link

Under some circumstances team_close() can close STDIN it did not open. #49

Open MarcinOrlowski opened 4 years ago

MarcinOrlowski commented 4 years ago

Imagine the following, quite common flow scenario (pseudo code):

void DoSomething(std::string iface) { 
  th = team_alloc();
  if (!th) goto quit;

  uinit32_t ifindex = team_ifname2ifindex(th, ifname.c_str());
  if (!ifindex) goto quit;

  int err = team_init(th, ifindex);
  if (err) goto quit;

  [... do something]

quit:
  if (th) team_free(th);
}

Now imagine you call DoSomething() for the interface that is NOT part of any bonding. In such case , the flow will be like this: team_allow() will pass, but team_init() would fail, so team_free() would be called next to cleanup before leaving. But the problem is that team_free() would try to free too much. Current implementation:

TEAM_EXPORT
void team_free(struct team_handle *th)
{
    close(th->event_fd);
   ...

But the problem is that th->even_fd is NOT initialized by team_allow() but team_init() (or precisely by team_init_event_fd() invoked via team_init(). But as interface we passed to DoSomething() wasn't "legit", it quits not having a chance to initialize it. So now team_free() kicks in trying to clean up the stuff. It grabs th->event_fd and closes as-is, but as it wasn't initialized it closes random file descriptor that is there. As the team_handle struct is allocated with calloc(), default value is 0 (zero) and fd = 0 simply means STDIN fd. It may slip unnoticed with in most cases but if you have a process that reads from stdin (i.e. on a separate thread) and the other is continuosuly calling above example method, i.e. for all the interfaces found on the device, then the bomb is cooked. The fix is obviously simple one liner:

TEAM_EXPORT
void team_free(struct team_handle *th)
{
    if (th->event_fd) {
      close(th->event_fd);
    }
    ...