Re: [PATCH 1/2] Fix overread by 1 byte in pk_cmd_get_next_match()

[John Darrington]

I think this patch is counter-productive.

On Fri, Dec 13, 2019 at 12:08:31AM +0100, Dan ??erm??k wrote:
     From: Dan ??erm??k <address@hidden>
     
     The name string was generated with the wrong length: strlen() returns the 
length
     of (*c)->name _excluding_ \0, but we must allocated enough bytes for
     '.' + strlen((*c)->name) + '\0' => strlen((*c)->name) + 2.

Since we will never be writing the \0 we do not need to allocate space
for it (although it couldn't hurt)   (we also never read to a
terminating \0 - we use strncmp which doesn't look for one).

     Furthermore, since we only need to copy a single character into name, we 
just
     write the char directly instead of invoking strcpy and then use strncpy 
instead
     of the notoriously unsafe strcat.

I don't believe that strcat is "notoriously unsafe".  In fact, most
people regard strncpy as unsafe (which is why "make syntax-check" fails
if you use it).

J'

     ---
      src/pk-cmd.c | 8 +++++---
      1 file changed, 5 insertions(+), 3 deletions(-)
     
     diff --git a/src/pk-cmd.c b/src/pk-cmd.c
     index 7d703b1..abc2115 100644
     --- a/src/pk-cmd.c
     +++ b/src/pk-cmd.c
     @@ -787,9 +787,11 @@ pk_cmd_get_next_match (int *idx, const char *x, 
size_t len)
            if (*c == &null_cmd)
        break;
      
     -      char *name = xmalloc (strlen ((*c)->name) + 1);
     -      strcpy (name, ".");
     -      strcat (name, (*c)->name);
     +      /* don't forget the null terminator of name */
     +      const size_t name_len = strlen ((*c)->name);
     +      char *name = xmalloc (name_len + 2);
     +      name[0] = '.';
     +      strncpy (name+1, (*c)->name, name_len + 1);
            if (0 !=  strncmp (name, x, len))
        {
          free (name);
     -- 
     2.23.0