Re: [PATCH unionfs 3/3] Don't use strncat() with length derived from source string

[Sergey Bugaev]

On Mon, Apr 26, 2021 at 9:02 PM Jessica Clarke <jrtc27@jrtc27.com> wrote:
> > -  strncpy (filepath, path, length);
> > -  strncat (filepath, filename, strlen (filename));
> > +  strcpy (filepath, path);
> > +  strcat (filepath, filename);
>
> This is dubious. We should be using safe interfaces where possible.

To expand a bit on my (an GCC's) reasoning, it makes no sense to use
strncpy/strncat with length that is derived from the *source* string
length, such as

strncat (filepath, filename, strlen (filename));

because str[n]cat would never copy more than strlen (filename) bytes
from filename anyway. The intended/safe way to use strncpy is by
specifying the *destination* buffer size, e.g.

strcpy (buffer, source, sizeof buffer);

I could have done just that; but then it makes little sense when the
buffer size is, too, directly derived from the source string length,
as in

int dir_len;
dir_len = strlen(dir);
char *tmp;
tmp = (char *) malloc (dir_len + 1);
strncpy (tmp, dir, dir_len);

But anyway, I don't care enough about this rather harmless warning to
argue further.

The actually important part here was the following fix (which I'm
going to use as an excuse to try out the "scissors" feature):

-- >8 --
Subject: [PATCH unionfs v2 3/3] Properly null-terminate a string

---
 stow.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/stow.c b/stow.c
index 812d33b..ddbcf20 100644
--- a/stow.c
+++ b/stow.c
@@ -281,16 +281,17 @@ stow_diradd (char *dir, int flags, struct
patternlist *patternlist,
   if (dir[dir_len - 1 ] != '/')
     {
       char *tmp;

-      tmp = (char *) malloc (dir_len + 1);
+      tmp = (char *) malloc (dir_len + 2);

       if (tmp == NULL)
        return ENOMEM;

       strncpy (tmp, dir, dir_len);

       tmp[dir_len] = '/';
+      tmp[dir_len + 1] = 0;

       dir = tmp;
     }
-- 

Sergey