[L] Change in openvpn[master]: compat: close all unrelated fds in forked helpers

Attention is currently required from: cron2, srkyn.

razvanc has posted comments on this change by srkyn. ( http://gerrit.openvpn.net/c/openvpn/+/1881?usp=email )

Change subject: compat: close all unrelated fds in forked helpers
......................................................................


Patch Set 4:

(15 comments)

File CMakeLists.txt:

http://gerrit.openvpn.net/c/openvpn/+/1881/comment/539ea40c_e5e98995?usp=email :
PS2, Line 699:             )
> This makes it look as if test_close_fds is somehow related to test_cryptoapi. […]
Done


http://gerrit.openvpn.net/c/openvpn/+/1881/comment/7462133d_279dc489?usp=email :
PS2, Line 852:             tests/unit_tests/openvpn/mock_get_random.c
> Do you need `mock_get_random.c` here? The Automake version doesn't add it and it seems fine.
Done


File src/compat/close-fds.h:

http://gerrit.openvpn.net/c/openvpn/+/1881/comment/6dbe1f11_567fd97a?usp=email :
PS1, Line 13:  */
> The rest of the files have a more comprehensive block of text about the license, we probably want to […]
Done


http://gerrit.openvpn.net/c/openvpn/+/1881/comment/4023b0a5_04fb73fc?usp=email :
PS1, Line 19: void close_fds_except(int keep);
> The new function lost all previous (Doxygen) documentation explaining the need for it and what it do […]
Done


File src/compat/close-fds.c:

http://gerrit.openvpn.net/c/openvpn/+/1881/comment/0f8f44d9_be569440?usp=email :
PS1, Line 12:  *  as published by the Free Software Foundation.
> Truncated license text compared to existing files here as well.
Done


File src/compat/close-fds.c:

http://gerrit.openvpn.net/c/openvpn/+/1881/comment/1501e466_54f5b66c?usp=email :
PS2, Line 39: #ifdef HAVE_CLOSE_RANGE
> I could be wrong (maybe I missed something subtle), but AFAICT this path can never be exercised unde […]
Done


http://gerrit.openvpn.net/c/openvpn/+/1881/comment/0c59f966_7025f8c8?usp=email :
PS2, Line 42:         const int lower_closed = keep == 3 || close_range(3, (unsigned int)keep - 1, 0) == 0;
> ... and please avoid complex conditional operator constructs with side effects in assignments. […]
Done


File src/compat/close-fds.c:

http://gerrit.openvpn.net/c/openvpn/+/1881/comment/1056f83b_1857d0a4?usp=email :
PS3, Line 1: /*
> It looks like all the other files in src/compat have a naming convention of compat-*. […]
Done


http://gerrit.openvpn.net/c/openvpn/+/1881/comment/d89b0a25_26c6a7bb?usp=email :
PS3, Line 64:     }
> This can probably be done in a synonymous but shorter form, i.e. […]
Done


File tests/unit_tests/openvpn/Makefile.am:

http://gerrit.openvpn.net/c/openvpn/+/1881/comment/05fbb928_b6b56f7e?usp=email :
PS2, Line 52: close_fds_testdriver_CFLAGS = -I$(top_srcdir)/src/compat @TEST_CFLAGS@
> This is the only testdriver whose CFLAGS omit `-I$(top_srcdir)/src/openvpn`. […]
Done


File tests/unit_tests/openvpn/test_close_fds.c:

http://gerrit.openvpn.net/c/openvpn/+/1881/comment/fd8640c3_ab378458?usp=email :
PS1, Line 12:  *  as published by the Free Software Foundation.
> License header doesn't match the rest of the code in length.
Done


http://gerrit.openvpn.net/c/openvpn/+/1881/comment/1bf78c4e_5f1d8fce?usp=email :
PS1, Line 36: run_case(int keep_target)
> The current convention AFAICT is to use cmocka for tests, which this does not.
Done


http://gerrit.openvpn.net/c/openvpn/+/1881/comment/6706f88e_1549c3e0?usp=email :
PS1, Line 109:                : EXIT_FAILURE;
> Might we also want to test that fds 0, 1 and 2 survive?
Done


File tests/unit_tests/openvpn/test_misc.c:

http://gerrit.openvpn.net/c/openvpn/+/1881/comment/95ace02a_fef92240?usp=email :
PS3, Line 544:         if (dup2(source, 100) < 0 || dup2(source, 200) < 0)
> The new tests fail when `RLIMIT_NOFILE` ≤ 200 (e.g. `ulimit -n  64` causes them to fail. […]
Done


File tests/unit_tests/openvpn/test_misc.c:

http://gerrit.openvpn.net/c/openvpn/+/1881/comment/c80f52f3_43159fc8?usp=email :
PS4, Line 567:                                 : '0';
This has the tests pass even without the patch, since `dup(source)` will return the lowest free descriptor, so these should always be <= 6 here.

Ideally a unit test should fail without the fix and pass once the fix lands.



-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1881?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: comment
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Iacd56d245e9ab30cfb25b2b364ca661fa289dc55
Gerrit-Change-Number: 1881
Gerrit-PatchSet: 4
Gerrit-Owner: srkyn <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-Reviewer: razvanc <[email protected]>
Gerrit-CC: cron2 <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: cron2 <[email protected]>
Gerrit-Attention: srkyn <[email protected]>
Gerrit-Comment-Date: Mon, 07 Sep 2026 09:56:18 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: cron2 <[email protected]>
Comment-In-Reply-To: razvanc <[email protected]>

_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel