[20/24] If FILENAME_MAX is defined, use it instead of arbitrary value (fix format-truncation errors with GCC >= 7)

Commit Message

Manuel Bouyer Dec. 14, 2020, 4:36 p.m. UTC

---
 tools/xenpmd/xenpmd.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Roger Pau Monne Dec. 29, 2020, 2:51 p.m. UTC | #1

On Mon, Dec 14, 2020 at 05:36:19PM +0100, Manuel Bouyer wrote:
> ---
>  tools/xenpmd/xenpmd.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/tools/xenpmd/xenpmd.c b/tools/xenpmd/xenpmd.c
> index 12b82cf43e..cfd22e64e3 100644
> --- a/tools/xenpmd/xenpmd.c
> +++ b/tools/xenpmd/xenpmd.c
> @@ -101,7 +101,11 @@ FILE *get_next_battery_file(DIR *battery_dir,
>  {
>      FILE *file = 0;
>      struct dirent *dir_entries;
> +#ifdef FILENAME_MAX
> +    char file_name[FILENAME_MAX];
> +#else
>      char file_name[284];
> +#endif
>      int ret;

I think it's dangerous to do this, specially on the stack, GNU libc
manual states:

Usage Note: Don’t use FILENAME_MAX as the size of an array in which to
store a file name! You can’t possibly make an array that big! Use
dynamic allocation (see Memory Allocation) instead.

I think it would be better to replace the snprintf calls with asprintf
and free the buffer afterwards. Setting file_name to 284 should be
fine however, as d_name is 256 max and the paths above are 26 maximum
I think (27 with the nul character).

Thanks, Roger.

Manuel Bouyer Jan. 4, 2021, 11:03 a.m. UTC | #2

On Tue, Dec 29, 2020 at 03:51:55PM +0100, Roger Pau Monné wrote:
> I think it's dangerous to do this, specially on the stack, GNU libc
> manual states:
> 
> Usage Note: Don?t use FILENAME_MAX as the size of an array in which to
> store a file name! You can?t possibly make an array that big! Use
> dynamic allocation (see Memory Allocation) instead.
> 
> I think it would be better to replace the snprintf calls with asprintf
> and free the buffer afterwards.

I went this route, thanks

> Setting file_name to 284 should be
> fine however, as d_name is 256 max and the paths above are 26 maximum
> I think (27 with the nul character).

On NetBSD d_name is 512 ... I guess this is why gcc complains.

Patch

diff --git a/tools/xenpmd/xenpmd.c b/tools/xenpmd/xenpmd.c
index 12b82cf43e..cfd22e64e3 100644
--- a/tools/xenpmd/xenpmd.c
+++ b/tools/xenpmd/xenpmd.c
@@ -101,7 +101,11 @@  FILE *get_next_battery_file(DIR *battery_dir,
 {
     FILE *file = 0;
     struct dirent *dir_entries;
+#ifdef FILENAME_MAX
+    char file_name[FILENAME_MAX];
+#else
     char file_name[284];
+#endif
     int ret;
     
     do