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

[Dan Čermák]

John Darrington <address@hidden> writes:

> 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).

We don't read the \0, but libreadline does. You can check that by
building poke with -fsanitize=address, start it, press tab and watch it
die (or just build it regularly, start it with valgrind, and press
tab). libreadline then causes an invalid overread by one byte.

>
>      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
>      
>

signature.asc
Description: PGP signature