Libguestfs - Jan 2022 - [PATCH] lib: Better handling for problems creating the socket path

GCC 12 gives a warning about our previous attempt to check the length
of the socket path.  In the ensuing discussion it was pointed out that
it is easier to get snprintf to do the hard work.  snprintf will
return an int >= UNIX_PATH_MAX if the path is too long, or -1 if there
are other errors such as locale/encoding problems.  So we should just
check for those two cases instead.

https://lists.fedoraproject.org/archives/list/devel at
lists.fedoraproject.org/thread/NPKWMTSJ2A2ABNJJEH6WTZIAEFTX6CQY/

Thanks: Martin Sebor and Laszlo Ersek
---
 lib/launch.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/lib/launch.c b/lib/launch.c
index 253189b200..e3f64aec90 100644
--- a/lib/launch.c
+++ b/lib/launch.c
@@ -325,15 +325,20 @@ int
 guestfs_int_create_socketname (guestfs_h *g, const char *filename,
                                char (*sockpath)[UNIX_PATH_MAX])
 {
+  int r;
+
   if (guestfs_int_lazy_make_sockdir (g) == -1)
     return -1;
 
-  if (strlen (g->sockdir) + 1 + strlen (filename) > UNIX_PATH_MAX-1) {
+  r = snprintf (*sockpath, UNIX_PATH_MAX, "%s/%s", g->sockdir,
filename);
+  if (r >= UNIX_PATH_MAX) {
     error (g, _("socket path too long: %s/%s"), g->sockdir,
filename);
     return -1;
   }
-
-  snprintf (*sockpath, UNIX_PATH_MAX, "%s/%s", g->sockdir,
filename);
+  if (r == -1) {
+    perrorf (g, _("%s"), g->sockdir);
+    return -1;
+  }
 
   return 0;
 }
-- 
2.32.0

On 01/17/22 13:40, Richard W.M. Jones wrote:
> GCC 12 gives a warning about our previous attempt to check the length
> of the socket path.  In the ensuing discussion it was pointed out that
> it is easier to get snprintf to do the hard work.  snprintf will
> return an int >= UNIX_PATH_MAX if the path is too long, or -1 if there
> are other errors such as locale/encoding problems.  So we should just
> check for those two cases instead.
> 
> https://lists.fedoraproject.org/archives/list/devel at
lists.fedoraproject.org/thread/NPKWMTSJ2A2ABNJJEH6WTZIAEFTX6CQY/
> 
> Thanks: Martin Sebor and Laszlo Ersek
> ---
>  lib/launch.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/launch.c b/lib/launch.c
> index 253189b200..e3f64aec90 100644
> --- a/lib/launch.c
> +++ b/lib/launch.c
> @@ -325,15 +325,20 @@ int
>  guestfs_int_create_socketname (guestfs_h *g, const char *filename,
>                                 char (*sockpath)[UNIX_PATH_MAX])
>  {
> +  int r;
> +
>    if (guestfs_int_lazy_make_sockdir (g) == -1)
>      return -1;
>  
> -  if (strlen (g->sockdir) + 1 + strlen (filename) > UNIX_PATH_MAX-1)
{
> +  r = snprintf (*sockpath, UNIX_PATH_MAX, "%s/%s",
g->sockdir, filename);
> +  if (r >= UNIX_PATH_MAX) {
>      error (g, _("socket path too long: %s/%s"), g->sockdir,
filename);
>      return -1;
>    }
> -
> -  snprintf (*sockpath, UNIX_PATH_MAX, "%s/%s", g->sockdir,
filename);
> +  if (r == -1) {
> +    perrorf (g, _("%s"), g->sockdir);
> +    return -1;
> +  }
>  
>    return 0;
>  }
> 
The specs don't say (-1), they say "a negative value". I suggest
updating both the commit message and the last error check.

With that:

Reviewed-by: Laszlo Ersek <lersek at redhat.com>

Thanks,
Laszlo