[PATCH] libxl: make sure string is null-terminated in libxl__prepare_sockaddr_un

[PATCH] libxl: make sure string is null-terminated in libxl__prepare_sockaddr_un

[Wei Liu]

Coverity-ID: 1438472
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/libxl/libxl_utils.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index 5854717b11..e06f765699 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -1238,6 +1238,8 @@ int libxl__prepare_sockaddr_un(libxl__gc *gc,
                                struct sockaddr_un *un, const char *path,
                                const char *what)
 {
+    size_t sz = sizeof(un->sun_path);
+
     if (sizeof(un->sun_path) <= strlen(path)) {
         LOG(ERROR, "UNIX socket path '%s' is too long for %s", path, what);
         LOG(DEBUG, "Path must be less than %zu bytes", sizeof(un->sun_path));
@@ -1245,7 +1247,8 @@ int libxl__prepare_sockaddr_un(libxl__gc *gc,
     }
     memset(un, 0, sizeof(struct sockaddr_un));
     un->sun_family = AF_UNIX;
-    strncpy(un->sun_path, path, sizeof(un->sun_path));
+    strncpy(un->sun_path, path, sz);
+    un->sun_path[sz - 1] = '\0';
     return 0;
 }
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] libxl: make sure string is null-terminated in libxl__prepare_sockaddr_un

[Ian Jackson]

Wei Liu writes ("[PATCH] libxl: make sure string is null-terminated in libxl__prepare_sockaddr_un"):
> Coverity-ID: 1438472
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

But...

> diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
> index 5854717b11..e06f765699 100644
> --- a/tools/libxl/libxl_utils.c
> +++ b/tools/libxl/libxl_utils.c
> @@ -1238,6 +1238,8 @@ int libxl__prepare_sockaddr_un(libxl__gc *gc,
>                                 struct sockaddr_un *un, const char *path,
>                                 const char *what)
>  {
> +    size_t sz = sizeof(un->sun_path);
> +
>      if (sizeof(un->sun_path) <= strlen(path)) {
>          LOG(ERROR, "UNIX socket path '%s' is too long for %s", path, what);
>          LOG(DEBUG, "Path must be less than %zu bytes", sizeof(un->sun_path));
           return ERROR_INVAL;
> @@ -1245,7 +1247,8 @@ int libxl__prepare_sockaddr_un(libxl__gc *gc,
>      }
>      memset(un, 0, sizeof(struct sockaddr_un));
>      un->sun_family = AF_UNIX;
> -    strncpy(un->sun_path, path, sizeof(un->sun_path));
> +    strncpy(un->sun_path, path, sz);
> +    un->sun_path[sz - 1] = '\0';

If we have reached this point, sizeof(un->sun_path) > strlen(path).
So in fact, strcpy would do.  strncpy will always add a nul.

We just memset the whole struct to 0.

If this new code has any effect at all, it will corrupt the string by
truncating it.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] libxl: make sure string is null-terminated in libxl__prepare_sockaddr_un

[Wei Liu]

On Wed, Aug 22, 2018 at 12:39:33PM +0100, Ian Jackson wrote:
> Wei Liu writes ("[PATCH] libxl: make sure string is null-terminated in libxl__prepare_sockaddr_un"):
> > Coverity-ID: 1438472
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> But...
> 
> > diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
> > index 5854717b11..e06f765699 100644
> > --- a/tools/libxl/libxl_utils.c
> > +++ b/tools/libxl/libxl_utils.c
> > @@ -1238,6 +1238,8 @@ int libxl__prepare_sockaddr_un(libxl__gc *gc,
> >                                 struct sockaddr_un *un, const char *path,
> >                                 const char *what)
> >  {
> > +    size_t sz = sizeof(un->sun_path);
> > +
> >      if (sizeof(un->sun_path) <= strlen(path)) {
> >          LOG(ERROR, "UNIX socket path '%s' is too long for %s", path, what);
> >          LOG(DEBUG, "Path must be less than %zu bytes", sizeof(un->sun_path));
>            return ERROR_INVAL;
> > @@ -1245,7 +1247,8 @@ int libxl__prepare_sockaddr_un(libxl__gc *gc,
> >      }
> >      memset(un, 0, sizeof(struct sockaddr_un));
> >      un->sun_family = AF_UNIX;
> > -    strncpy(un->sun_path, path, sizeof(un->sun_path));
> > +    strncpy(un->sun_path, path, sz);
> > +    un->sun_path[sz - 1] = '\0';
> 
> If we have reached this point, sizeof(un->sun_path) > strlen(path).
> So in fact, strcpy would do.  strncpy will always add a nul.
> 
> We just memset the whole struct to 0.
> 
> If this new code has any effect at all, it will corrupt the string by
> truncating it.

Corrupt? Per your analysis above, isn't the last byte always going to be
nul? This change is to placate coverity more than anything else.

Isn't setting the last byte to 0 a common pattern to ensure a
string is null-terminated?

Wei.

> 
> Ian.
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] libxl: make sure string is null-terminated in libxl__prepare_sockaddr_un

[Ian Jackson]

Wei Liu writes ("Re: [PATCH] libxl: make sure string is null-terminated in libxl__prepare_sockaddr_un"):
> On Wed, Aug 22, 2018 at 12:39:33PM +0100, Ian Jackson wrote:
> > If this new code has any effect at all, it will corrupt the string by
> > truncating it.
> 
> Corrupt? Per your analysis above, isn't the last byte always going to be
> nul? This change is to placate coverity more than anything else.

As I said, if this new code has any effect at all.  In that case the
last byte must be non-nul.  In which case setting it to nul is
truncating it.

If it is necessary to placate Coverity, how about an assert ?

> Isn't setting the last byte to 0 a common pattern to ensure a
> string is null-terminated?

Only if you don't mind truncated it.  Which we do, here.  Which is why
the previous code checks to make sure that it fits, including the
trailing nul.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel