[v2,2/2] Enable the close-on-exec flag for the debug log file descriptor

Message ID 20181011204135.30058-1-karol@babioch.de
State Superseded
Headers show
Series
  • Untitled series #70343
Related show

Commit Message

Karol Babioch Oct. 11, 2018, 8:41 p.m.
On Linux this flag will make sure that no file descriptor is accidentally
leaked into potential child processes. While this is not a problem right now,
it is considered to be good practice these days when dealing with file
descriptors on the Linux.

Signed-off-by: Karol Babioch <karol@babioch.de>
---
 src/utils/wpa_debug.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Johannes Berg Oct. 12, 2018, 5:52 a.m. | #1
On Thu, 2018-10-11 at 22:41 +0200, Karol Babioch wrote:
> On Linux this flag will make sure that no file descriptor is accidentally
> leaked into potential child processes. While this is not a problem right now,
> it is considered to be good practice these days when dealing with file
> descriptors on the Linux.
> 
> Signed-off-by: Karol Babioch <karol@babioch.de>
> ---
>  src/utils/wpa_debug.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/src/utils/wpa_debug.c b/src/utils/wpa_debug.c
> index b412f88e3..9d159632d 100644
> --- a/src/utils/wpa_debug.c
> +++ b/src/utils/wpa_debug.c
> @@ -60,6 +60,9 @@ static int wpa_to_android_level(int level)
>  #ifdef CONFIG_DEBUG_FILE
>  #include <sys/types.h>
>  #include <sys/stat.h>
> +#ifdef __linux__
> +#include <fcntl.h>
> +#endif /* __linux__ */
>  
>  static int out_fd = -1;
>  static FILE *out_file = NULL;
> @@ -567,6 +570,14 @@ int wpa_debug_open_file(const char *path)
>  		return -1;
>  	}
>  
> +#ifdef __linux__
> +	if (fcntl(out_fd, F_SETFD, FD_CLOEXEC) == -1) {
> +		wpa_printf(MSG_ERROR, "wpa_debug_open_file: Failed to set O_CLOEXEC "
> +			   "on output file descriptor, using standard output");

I'd argue you should just fall back to the old behaviour in the unlikely
even this fails, rather than closing the FD?

johannes
Karol Babioch Oct. 12, 2018, 6:47 a.m. | #2
Hi Johannes,

Am 12.10.18 um 07:52 schrieb Johannes Berg:
> I'd argue you should just fall back to the old behaviour in the unlikely
> even this fails, rather than closing the FD?

Of course your suggestion makes a lot of sense and I will send an
updatech patch. Do you have any opinion on what kind of level the
message that we output in this case should be? MSG_ERROR might be
overkill, since we continue to work. Not sure if this is MSG_WARNING or
whether we should go below that.

As you've said it is extremely unlikely that this is going to fail.

Best regards,
Karol Babioch

Patch

diff --git a/src/utils/wpa_debug.c b/src/utils/wpa_debug.c
index b412f88e3..9d159632d 100644
--- a/src/utils/wpa_debug.c
+++ b/src/utils/wpa_debug.c
@@ -60,6 +60,9 @@  static int wpa_to_android_level(int level)
 #ifdef CONFIG_DEBUG_FILE
 #include <sys/types.h>
 #include <sys/stat.h>
+#ifdef __linux__
+#include <fcntl.h>
+#endif /* __linux__ */
 
 static int out_fd = -1;
 static FILE *out_file = NULL;
@@ -567,6 +570,14 @@  int wpa_debug_open_file(const char *path)
 		return -1;
 	}
 
+#ifdef __linux__
+	if (fcntl(out_fd, F_SETFD, FD_CLOEXEC) == -1) {
+		wpa_printf(MSG_ERROR, "wpa_debug_open_file: Failed to set O_CLOEXEC "
+			   "on output file descriptor, using standard output");
+		close(out_fd);
+		return -1;
+	}
+#endif /* __linux__ */
 #ifndef _WIN32
 	setvbuf(out_file, NULL, _IOLBF, 0);
 #endif /* _WIN32 */