Subject: Re: [PATCH 5/5] Prevent tmpbuf from overrunning

Re: [PATCH 5/5] Prevent tmpbuf from overrunning

From: David Drysdale <drysdale_at_google.com>
Date: Mon, 22 Sep 2014 10:57:18 +0100

The code looks OK to me, but it might be easier to have an extra local
variable (e.g. char *name) that is set to either sep->s_name or
tmpbuf; that would reduce the code duplication. Maybe something like
the following (untested)?

diff --git a/ares_getnameinfo.c b/ares_getnameinfo.c
index 5b9f6386b29e..b0bc6da868cd 100644
--- a/ares_getnameinfo.c
+++ b/ares_getnameinfo.c
@@ -281,6 +281,8 @@ static char *lookup_service(unsigned short port, int flags,
   struct servent se;
 #endif
   char tmpbuf[4096];
+ char *name;
+ size_t name_len;

   if (port)
     {
@@ -323,14 +325,20 @@ static char *lookup_service(unsigned short port,
int flags,
 #endif
         }
       if (sep && sep->s_name)
- /* get service name */
- strcpy(tmpbuf, sep->s_name);
+ {
+ /* get service name */
+ name = sep->s_name;
+ }
       else
- /* get port as a string */
- sprintf(tmpbuf, "%u", (unsigned int)ntohs(port));
- if (strlen(tmpbuf) < buflen)
+ {
+ /* get port as a string */
+ sprintf(tmpbuf, "%u", (unsigned int)ntohs(port));
+ name = tmpbuf;
+ }
+ name_len = strlen(name);
+ if (name_len < buflen)
         /* return it if buffer big enough */
- strcpy(buf, tmpbuf);
+ memcpy(buf, name, name_len + 1);
       else
         /* avoid reusing previous one */
         buf[0] = '\0';

On Fri, Sep 19, 2014 at 7:51 PM, Gregor Jasny <gjasny_at_googlemail.com> wrote:
> Fix Coverity error CID 56886.
>
> Signed-off-by: Gregor Jasny <gjasny_at_googlemail.com>
> ---
> ares_getnameinfo.c | 30 ++++++++++++++++++++----------
> 1 file changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/ares_getnameinfo.c b/ares_getnameinfo.c
> index 5b9f638..39337e7 100644
> --- a/ares_getnameinfo.c
> +++ b/ares_getnameinfo.c
> @@ -323,17 +323,27 @@ static char *lookup_service(unsigned short port, int flags,
> #endif
> }
> if (sep && sep->s_name)
> - /* get service name */
> - strcpy(tmpbuf, sep->s_name);
> - else
> - /* get port as a string */
> - sprintf(tmpbuf, "%u", (unsigned int)ntohs(port));
> - if (strlen(tmpbuf) < buflen)
> - /* return it if buffer big enough */
> - strcpy(buf, tmpbuf);
> + {
> + /* get service name */
> + size_t name_len = strlen(sep->s_name);
> + if (name_len < buflen)
> + /* return it if buffer big enough */
> + memcpy(buf, sep->s_name, name_len + 1);
> + else
> + /* avoid reusing previous one */
> + buf[0] = '\0';
> + }
> else
> - /* avoid reusing previous one */
> - buf[0] = '\0';
> + {
> + /* get port as a string */
> + sprintf(tmpbuf, "%u", (unsigned int)ntohs(port));
> + if (strlen(tmpbuf) < buflen)
> + /* return it if buffer big enough */
> + strcpy(buf, tmpbuf);
> + else
> + /* avoid reusing previous one */
> + buf[0] = '\0';
> + }
> return buf;
> }
> buf[0] = '\0';
> --
> 1.8.5.2 (Apple Git-48)
>
Received on 2014-09-22