Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
MarcinOrlowski opened this issue Mar 9, 2020 · 0 comments

Comments

@MarcinOrlowski
Copy link

MarcinOrlowski commented Mar 9, 2020

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);
	}
	...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant