Set closed file descriptor values to -1 to prevent subsequent re-close

[jhfrontz]

We have a rare problem in our proxychains-ng-wrapped multithreaded application: sometimes a freshly opened socket to a local (non-proxied) connection is found to be closed. EBADF -- Bad file descriptor -- is returned on an otherwise successful connect(2) or write(2) call early in the lifetime of the victim connection. These EBADF incidents seem to be coincident with the instability of the proxy connection (in use by other threads in the application).

There are a few spots in proxychains-ng where error legs may call close(2) twice on the same fd value (which could essentially close a re-used fd out from under another thread). Setting the fd value to -1 immediately after calling close(2) will prevent this.

(I went back and forth on whether to set all instances of a closed fd to -1 -- even those that are local variables -- in the name of ease-of-review, but ultimately decided only to change those that I think can result in the race condition).

[rofl0r]

good find. i assume the problem you encountered is fixed with your patch ? the current patch seems a bit noisy, if you can i'd like you to test only one hunk at a time as i suspect the issue with possible double-close happens only in a single spot, and after you found it restrict the patch to that location.

[jhfrontz]

Unfortunately, the problem is not an easy thing to reproduce (it happens about once a week or so across hundreds of connections between over a dozen semi-autonomous systems -- and seemingly never in any of our extensive test environments). We're contemplating putting a patched version of proxychains-ng on one member of the network, but it might be a year (or more) before the condition arises on the test system (if it happens at all).

I suspect the "noisiness" you mention arises because chain_step() modifies the file descriptor (via close(2)) without having had write access to the ns variable (containing the sometimes-to-be-invalidated file descriptor number). I'm not sure how the extent of the associated changes can be remedied/reduced. But I may also not be understanding your observation.

(in re-review, I noticed that I hadn't incorporated a final local fixup before I pushed the PR -- apologies for the update)

[jhfrontz]

@rofl0r updated to simplify the fix. A way to test this would be to add a delay on the return from close(2) (e.g., with a wrapper that perhaps also keeps an accounting of open/closed file descriptors, alerting if close(2) is called on an already-closed file descriptor).

[rofl0r]

the last version of your patch didn't look quite right, so i went ahead and analyzed the problem in depth by following all code paths and committed a patch that should fix the problem for good. thanks for your effort and report. in the unlikely case that the patch i commited still doesnt fix the issue in your application, please open another issue.