Re: [HACKERS] StrNCpy - Mailing list pgsql-hackers

>
> Hi,
>
> While debugging yet another overrun I came across the StrNCpy macro.
> A quick grep of the source tells me that usage of the StrNCpy macro is
> seemingly inconsistent.
>
> Usage 1:
> strptr = palloc(len);    // done is a diffrent context
> ptr = palloc(len + 1);
> StrNCpy(ptr, strptr, len + 1);
>
> Usage 2:
> NameData name;
> StrNCpy(name.data, ptr2name, NAMEDATALEN);
>
> The StrNCpy macro zero terminates the destination buffer.
>
> Usage 1 is gives a read=buffer overrun (which I agree is not the most
> serious of bugs
> if you system doesn't dump core on it).
> Usage 2 makes gives the name a maximum of 31 instead of 32 characters.
>
> Is the maximun name length supposted to be 31 or 32 characters?

Thanks for checking into these things for us Maurice.  I zero-terminate
all name-type fields, so the max is 31, not 32.

The SQL manual page says:

       Names  in  SQL  are  sequences  of  less  than NAMEDATALEN
       alphanumeric characters, starting with an alphabetic char-
       acter.   By  default, NAMEDATALEN is set to 32, but at the
       time the system is built, NAMEDATALEN can  be  changed  by
       changing  the  #ifdef  in  src/backend/include/postgres.h.
       Underscore ("_") is considered an alphabetic character.

I updated this manual page when I decided to be consistent and always
zero-terminate the NAME type.

So the second usage is OK, the first usage:

    > strptr = palloc(len);    // done is a diffrent context
    > ptr = palloc(len + 1);
    > StrNCpy(ptr, strptr, len + 1);

is legally a problem because strNcpy() is doing:

    (strncpy((dst),(src),(len)),(len > 0) ? *((dst)+(len)-1)='\0' :(dummyret)NULL,(void)(dst))

and the call to strncpy() is using len, when that is one too large.
Now, I know I am going to write over that byte anyway if len >0, so it
is cleaned up, but it is wrong.  I will change the macro to do:

    (strncpy((dst),(src),(len-1)),(len > 0) ? *((dst)+(len)-1)='\0' :(dummyret)NULL,(void)(dst))

or something like that so I check for len == 0.

Here is a patch that does this.  I am applying it now.  Uses the new
macro formatting style:

---------------------------------------------------------------------------

*** ./include/c.h.orig    Tue Mar 31 10:42:36 1998
--- ./include/c.h    Tue Mar 31 10:46:23 1998
***************
*** 703,709 ****
   */
  /* we do this so if the macro is used in an if action, it will work */
  #define StrNCpy(dst,src,len)    \
!     (strncpy((dst),(src),(len)),(len > 0) ? *((dst)+(len)-1)='\0' : (dummyret)NULL,(void)(dst))

  /* Get a bit mask of the bits set in non-int32 aligned addresses */
  #define INT_ALIGN_MASK (sizeof(int32) - 1)
--- 703,717 ----
   */
  /* we do this so if the macro is used in an if action, it will work */
  #define StrNCpy(dst,src,len)    \
! ( \
!     ((len) > 0) ? \
!     ( \
!         strncpy((dst),(src),(len)-1), \
!         *((dst)+(len)-1)='\0' \
!     ) \
!     : \
!         (dummyret)NULL,(void)(dst) \
! )

  /* Get a bit mask of the bits set in non-int32 aligned addresses */
  #define INT_ALIGN_MASK (sizeof(int32) - 1)

--
Bruce Momjian                          |  830 Blythe Avenue
maillist@candle.pha.pa.us              |  Drexel Hill, Pennsylvania 19026
  +  If your life is a hard drive,     |  (610) 353-9879(w)
  +  Christ can be your backup.        |  (610) 853-3000(h)