diff mbox series

[lttng-tools] Replace deprecated readdir_r() with readdir()

Message ID 20180605161120.14061-1-mjeanson@efficios.com
State Accepted, archived
Delegated to: Jérémie Galarneau
Headers show
Series [lttng-tools] Replace deprecated readdir_r() with readdir() | expand

Commit Message

Michael Jeanson June 5, 2018, 4:11 p.m. UTC
readdir_r() has been deprecated since glibc 2.24 [1].

We used readdir_r() in load_session_from_path() to be thread-safe
since this function is part of liblttng-ust-ctl. However, according
to readdir()'s man page, it's thread-safe as long as the directory
stream it operates on is not shared across threads :

  In the current POSIX.1 specification (POSIX.1-2008), readdir(3) is
  not required to be thread-safe. However, in modern
  implementations (including the glibc implementation), concurrent
  calls to readdir(3) that specify different directory streams are
  thread-safe. Therefore, the use of readdir_r() is generally
  unnecessary in multithreaded programs. In cases where multiple
  threads must read from the same directory stream, using readdir(3)
  with external synchronization is still preferable to the use of
  readdir_r(), for the reasons given in the points above.

In our use-case where we open the directory stream in the same function,
we know it won't be shared across threads and thus it's safe to use
readdir(). Here is the relevevant bit from the POSIX.1 [2] spec :

  The returned pointer, and pointers within the structure, might be
  invalidated or the structure or the storage areas might be overwritten
  by a subsequent call to readdir() on the same directory stream. They shall
  not be affected by a call to readdir() on a different directory stream.

 [1] https://sourceware.org/bugzilla/show_bug.cgi?id=19056
 [2] http://pubs.opengroup.org/onlinepubs/9699919799/functions/readdir.html

Signed-off-by: Michael Jeanson <mjeanson at efficios.com>
---
 src/common/config/session-config.c | 54 ++++++++++++++----------------
 1 file changed, 26 insertions(+), 28 deletions(-)

Comments

Jérémie Galarneau June 6, 2018, 12:21 a.m. UTC | #1
Merged in master, thanks!

Jérémie

On Tue, Jun 05, 2018 at 12:11:20PM -0400, Michael Jeanson wrote:
> readdir_r() has been deprecated since glibc 2.24 [1].
> 
> We used readdir_r() in load_session_from_path() to be thread-safe
> since this function is part of liblttng-ust-ctl. However, according
> to readdir()'s man page, it's thread-safe as long as the directory
> stream it operates on is not shared across threads :
> 
>   In the current POSIX.1 specification (POSIX.1-2008), readdir(3) is
>   not required to be thread-safe. However, in modern
>   implementations (including the glibc implementation), concurrent
>   calls to readdir(3) that specify different directory streams are
>   thread-safe. Therefore, the use of readdir_r() is generally
>   unnecessary in multithreaded programs. In cases where multiple
>   threads must read from the same directory stream, using readdir(3)
>   with external synchronization is still preferable to the use of
>   readdir_r(), for the reasons given in the points above.
> 
> In our use-case where we open the directory stream in the same function,
> we know it won't be shared across threads and thus it's safe to use
> readdir(). Here is the relevevant bit from the POSIX.1 [2] spec :
> 
>   The returned pointer, and pointers within the structure, might be
>   invalidated or the structure or the storage areas might be overwritten
>   by a subsequent call to readdir() on the same directory stream. They shall
>   not be affected by a call to readdir() on a different directory stream.
> 
>  [1] https://sourceware.org/bugzilla/show_bug.cgi?id=19056
>  [2] http://pubs.opengroup.org/onlinepubs/9699919799/functions/readdir.html
> 
> Signed-off-by: Michael Jeanson <mjeanson at efficios.com>
> ---
>  src/common/config/session-config.c | 54 ++++++++++++++----------------
>  1 file changed, 26 insertions(+), 28 deletions(-)
> 
> diff --git a/src/common/config/session-config.c b/src/common/config/session-config.c
> index 624064a1..56db1195 100644
> --- a/src/common/config/session-config.c
> +++ b/src/common/config/session-config.c
> @@ -2953,23 +2953,6 @@ end:
>  	return ret;
>  }
>  
> -/* Allocate dirent as recommended by READDIR(3), NOTES on readdir_r */
> -static
> -struct dirent *alloc_dirent(const char *path)
> -{
> -	size_t len;
> -	long name_max;
> -	struct dirent *entry;
> -
> -	name_max = pathconf(path, _PC_NAME_MAX);
> -	if (name_max == -1) {
> -		name_max = PATH_MAX;
> -	}
> -	len = offsetof(struct dirent, d_name) + name_max + 1;
> -	entry = zmalloc(len);
> -	return entry;
> -}
> -
>  static
>  int load_session_from_path(const char *path, const char *session_name,
>  	struct session_config_validation_ctx *validation_ctx, int overwrite,
> @@ -3006,7 +2989,6 @@ int load_session_from_path(const char *path, const char *session_name,
>  		}
>  	}
>  	if (directory) {
> -		struct dirent *entry;
>  		struct dirent *result;
>  		size_t file_path_root_len;
>  
> @@ -3017,16 +2999,9 @@ int load_session_from_path(const char *path, const char *session_name,
>  			goto end;
>  		}
>  
> -		entry = alloc_dirent(path);
> -		if (!entry) {
> -			ret = -LTTNG_ERR_NOMEM;
> -			goto end;
> -		}
> -
>  	        ret = lttng_dynamic_buffer_append(&file_path, path, path_len);
>  		if (ret) {
>  			ret = -LTTNG_ERR_NOMEM;
> -			free(entry);
>  			goto end;
>  		}
>  
> @@ -3040,8 +3015,32 @@ int load_session_from_path(const char *path, const char *session_name,
>  		file_path_root_len = file_path.size;
>  
>  		/* Search for *.lttng files */
> -		while (!readdir_r(directory, entry, &result) && result) {
> -			size_t file_name_len = strlen(result->d_name);
> +		for (;;) {
> +			size_t file_name_len;
> +
> +			/*
> +			 * When the end of the directory stream is reached, NULL is
> +			 * returned and errno is kept unchanged. When an error occurs,
> +			 * NULL is returned and errno is set accordingly. To
> +			 * distinguish between the two, set errno to zero before
> +			 * calling readdir().
> +			 *
> +			 * On success, readdir() returns a pointer to a dirent
> +			 * structure. (This structure may be statically allocated, do
> +			 * not attempt to free(3) it.)
> +			 */
> +			errno = 0;
> +			result = readdir(directory);
> +
> +			/* Reached end of dir stream or error out */
> +			if (!result) {
> +				if (errno) {
> +					PERROR("readdir");
> +				}
> +				break;
> +			}
> +
> +			file_name_len = strlen(result->d_name);
>  
>  			if (file_name_len <=
>  				sizeof(DEFAULT_SESSION_CONFIG_FILE_EXTENSION)) {
> @@ -3089,7 +3088,6 @@ int load_session_from_path(const char *path, const char *session_name,
>  			}
>  		}
>  
> -		free(entry);
>  	} else {
>  		ret = load_session_from_file(path, session_name,
>  			validation_ctx, overwrite, overrides);
> -- 
> 2.17.0
>
diff mbox series

Patch

diff --git a/src/common/config/session-config.c b/src/common/config/session-config.c
index 624064a1..56db1195 100644
--- a/src/common/config/session-config.c
+++ b/src/common/config/session-config.c
@@ -2953,23 +2953,6 @@  end:
 	return ret;
 }
 
-/* Allocate dirent as recommended by READDIR(3), NOTES on readdir_r */
-static
-struct dirent *alloc_dirent(const char *path)
-{
-	size_t len;
-	long name_max;
-	struct dirent *entry;
-
-	name_max = pathconf(path, _PC_NAME_MAX);
-	if (name_max == -1) {
-		name_max = PATH_MAX;
-	}
-	len = offsetof(struct dirent, d_name) + name_max + 1;
-	entry = zmalloc(len);
-	return entry;
-}
-
 static
 int load_session_from_path(const char *path, const char *session_name,
 	struct session_config_validation_ctx *validation_ctx, int overwrite,
@@ -3006,7 +2989,6 @@  int load_session_from_path(const char *path, const char *session_name,
 		}
 	}
 	if (directory) {
-		struct dirent *entry;
 		struct dirent *result;
 		size_t file_path_root_len;
 
@@ -3017,16 +2999,9 @@  int load_session_from_path(const char *path, const char *session_name,
 			goto end;
 		}
 
-		entry = alloc_dirent(path);
-		if (!entry) {
-			ret = -LTTNG_ERR_NOMEM;
-			goto end;
-		}
-
 	        ret = lttng_dynamic_buffer_append(&file_path, path, path_len);
 		if (ret) {
 			ret = -LTTNG_ERR_NOMEM;
-			free(entry);
 			goto end;
 		}
 
@@ -3040,8 +3015,32 @@  int load_session_from_path(const char *path, const char *session_name,
 		file_path_root_len = file_path.size;
 
 		/* Search for *.lttng files */
-		while (!readdir_r(directory, entry, &result) && result) {
-			size_t file_name_len = strlen(result->d_name);
+		for (;;) {
+			size_t file_name_len;
+
+			/*
+			 * When the end of the directory stream is reached, NULL is
+			 * returned and errno is kept unchanged. When an error occurs,
+			 * NULL is returned and errno is set accordingly. To
+			 * distinguish between the two, set errno to zero before
+			 * calling readdir().
+			 *
+			 * On success, readdir() returns a pointer to a dirent
+			 * structure. (This structure may be statically allocated, do
+			 * not attempt to free(3) it.)
+			 */
+			errno = 0;
+			result = readdir(directory);
+
+			/* Reached end of dir stream or error out */
+			if (!result) {
+				if (errno) {
+					PERROR("readdir");
+				}
+				break;
+			}
+
+			file_name_len = strlen(result->d_name);
 
 			if (file_name_len <=
 				sizeof(DEFAULT_SESSION_CONFIG_FILE_EXTENSION)) {
@@ -3089,7 +3088,6 @@  int load_session_from_path(const char *path, const char *session_name,
 			}
 		}
 
-		free(entry);
 	} else {
 		ret = load_session_from_file(path, session_name,
 			validation_ctx, overwrite, overrides);