Re: [HACKERS] Strange interval arithmetic - Mailing list pgsql-patches

I started working on this problem on Tuesday, but was in New York City
yesterday so I was not able to comment on this patch before.  I think
the one applied is great.  (I added C comments on the use of "errno =
0".

Here is the patch I was working on.  It does us a separate libpq
strtol() function, but I question whether it is worth it, or if it is
meaningful when used by FRONTEND applications.  Anyway, I am just
throwing it out if it gives others ideas.

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

Michael Fuhr wrote:
> On Wed, Nov 30, 2005 at 02:01:46PM -0500, Tom Lane wrote:
> > Michael Fuhr <mike@fuhr.org> writes:
> > > Any preferences on an approach?  The simplest and easiest to verify
> > > would be to raise an error for just this particular case; a TODO
> > > item might be to change how the string is parsed to allow values
> > > larger than LONG_MAX.
> >
> > I think the latter would be a feature enhancement and therefore not
> > good material to back-patch.  Just erroring out seems appropriate
> > for now.
>
> Agreed.  I'm thinking about rewriting strtol() calls in datetime.c
> to look like this:
>
>   errno = 0;
>   val = strtol(field[i], &cp, 10);
>   if (errno == ERANGE)
>       return DTERR_FIELD_OVERFLOW;
>
> Does that look okay?  Or would you rather raise an error with ereport()?
>
> > > I see several calls to strtol() that aren't checked for overflow but
> > > that might not be relevant to this problem, so I'm thinking this patch
> > > ought not touch them.  Maybe that's another TODO item.
> >
> > If it's possible for them to be given overflowing input, they probably
> > ought to be checked.
>
> I'm looking at all the strtol() calls in datetime.c right now; I
> haven't looked anywhere else yet.  Should I bother checking values
> that will be range checked later anyway?  Time zone displacements,
> for example?
>
> --
> Michael Fuhr
>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: if posting/reading through Usenet, please send an appropriate
>        subscribe-nomail command to majordomo@postgresql.org so that your
>        message can get through to the mailing list cleanly
>

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073
Index: src/backend/libpq/ip.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/libpq/ip.c,v
retrieving revision 1.33
diff -c -c -r1.33 ip.c
*** src/backend/libpq/ip.c    22 Nov 2005 18:17:11 -0000    1.33
--- src/backend/libpq/ip.c    1 Dec 2005 18:40:02 -0000
***************
*** 342,348 ****
      long        bits;
      char       *endptr;

!     bits = strtol(numbits, &endptr, 10);

      if (*numbits == '\0' || *endptr != '\0')
          return -1;
--- 342,348 ----
      long        bits;
      char       *endptr;

!     bits = pg_strtol_range(numbits, &endptr, 10);

      if (*numbits == '\0' || *endptr != '\0')
          return -1;
Index: src/backend/utils/adt/datetime.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/adt/datetime.c,v
retrieving revision 1.161
diff -c -c -r1.161 datetime.c
*** src/backend/utils/adt/datetime.c    22 Nov 2005 18:17:22 -0000    1.161
--- src/backend/utils/adt/datetime.c    1 Dec 2005 18:40:03 -0000
***************
*** 1013,1019 ****
                      if (tzp == NULL)
                          return DTERR_BAD_FORMAT;

!                     val = strtol(field[i], &cp, 10);

                      j2date(val, &tm->tm_year, &tm->tm_mon, &tm->tm_mday);
                      /* Get the time zone from the end of the string */
--- 1013,1019 ----
                      if (tzp == NULL)
                          return DTERR_BAD_FORMAT;

!                     val = pg_strtol_range(field[i], &cp, 10);

                      j2date(val, &tm->tm_year, &tm->tm_mon, &tm->tm_mday);
                      /* Get the time zone from the end of the string */
***************
*** 1158,1164 ****
                      char       *cp;
                      int            val;

!                     val = strtol(field[i], &cp, 10);

                      /*
                       * only a few kinds are allowed to have an embedded
--- 1158,1164 ----
                      char       *cp;
                      int            val;

!                     val = pg_strtol_range(field[i], &cp, 10);

                      /*
                       * only a few kinds are allowed to have an embedded
***************
*** 1915,1921 ****
                              break;
                      }

!                     val = strtol(field[i], &cp, 10);

                      /*
                       * only a few kinds are allowed to have an embedded
--- 1915,1921 ----
                              break;
                      }

!                     val = pg_strtol_range(field[i], &cp, 10);

                      /*
                       * only a few kinds are allowed to have an embedded
***************
*** 2456,2466 ****

      *tmask = DTK_TIME_M;

!     tm->tm_hour = strtol(str, &cp, 10);
      if (*cp != ':')
          return DTERR_BAD_FORMAT;
      str = cp + 1;
!     tm->tm_min = strtol(str, &cp, 10);
      if (*cp == '\0')
      {
          tm->tm_sec = 0;
--- 2456,2466 ----

      *tmask = DTK_TIME_M;

!     tm->tm_hour = pg_strtol_range(str, &cp, 10);
      if (*cp != ':')
          return DTERR_BAD_FORMAT;
      str = cp + 1;
!     tm->tm_min = pg_strtol_range(str, &cp, 10);
      if (*cp == '\0')
      {
          tm->tm_sec = 0;
***************
*** 2471,2477 ****
      else
      {
          str = cp + 1;
!         tm->tm_sec = strtol(str, &cp, 10);
          if (*cp == '\0')
              *fsec = 0;
          else if (*cp == '.')
--- 2471,2477 ----
      else
      {
          str = cp + 1;
!         tm->tm_sec = pg_strtol_range(str, &cp, 10);
          if (*cp == '\0')
              *fsec = 0;
          else if (*cp == '.')
***************
*** 2522,2528 ****

      *tmask = 0;

!     val = strtol(str, &cp, 10);
      if (cp == str)
          return DTERR_BAD_FORMAT;

--- 2522,2528 ----

      *tmask = 0;

!     val = pg_strtol_range(str, &cp, 10);
      if (cp == str)
          return DTERR_BAD_FORMAT;

***************
*** 2809,2819 ****
      if (*str != '+' && *str != '-')
          return DTERR_BAD_FORMAT;

!     hr = strtol(str + 1, &cp, 10);

      /* explicit delimiter? */
      if (*cp == ':')
!         min = strtol(cp + 1, &cp, 10);
      /* otherwise, might have run things together... */
      else if (*cp == '\0' && strlen(str) > 3)
      {
--- 2809,2819 ----
      if (*str != '+' && *str != '-')
          return DTERR_BAD_FORMAT;

!     hr = pg_strtol_range(str + 1, &cp, 10);

      /* explicit delimiter? */
      if (*cp == ':')
!         min = pg_strtol_range(cp + 1, &cp, 10);
      /* otherwise, might have run things together... */
      else if (*cp == '\0' && strlen(str) > 3)
      {
***************
*** 3056,3062 ****

              case DTK_DATE:
              case DTK_NUMBER:
!                 val = strtol(field[i], &cp, 10);

                  if (type == IGNORE_DTF)
                      type = DTK_SECOND;
--- 3056,3062 ----

              case DTK_DATE:
              case DTK_NUMBER:
!                 val = pg_strtol_range(field[i], &cp, 10);

                  if (type == IGNORE_DTF)
                      type = DTK_SECOND;
Index: src/backend/utils/adt/numeric.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/adt/numeric.c,v
retrieving revision 1.88
diff -c -c -r1.88 numeric.c
*** src/backend/utils/adt/numeric.c    22 Nov 2005 18:17:23 -0000    1.88
--- src/backend/utils/adt/numeric.c    1 Dec 2005 18:40:08 -0000
***************
*** 2812,2818 ****
          char       *endptr;

          cp++;
!         exponent = strtol(cp, &endptr, 10);
          if (endptr == cp)
              ereport(ERROR,
                      (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
--- 2812,2818 ----
          char       *endptr;

          cp++;
!         exponent = pg_strtol_range(cp, &endptr, 10);
          if (endptr == cp)
              ereport(ERROR,
                      (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
Index: src/include/port.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/port.h,v
retrieving revision 1.84
diff -c -c -r1.84 port.h
*** src/include/port.h    15 Oct 2005 02:49:41 -0000    1.84
--- src/include/port.h    1 Dec 2005 18:40:09 -0000
***************
*** 249,254 ****
--- 249,257 ----
  #define closesocket close
  #endif   /* WIN32 */

+ /* do strtol(), but error out on ERANGE */
+ long pg_strtol_range(const char *nptr, char **endptr, int base);
+
  /*
   * Default "extern" declarations or macro substitutes for library routines.
   * When necessary, these routines are provided by files in src/port/.
Index: src/interfaces/ecpg/pgtypeslib/numeric.c
===================================================================
RCS file: /cvsroot/pgsql/src/interfaces/ecpg/pgtypeslib/numeric.c,v
retrieving revision 1.24
diff -c -c -r1.24 numeric.c
*** src/interfaces/ecpg/pgtypeslib/numeric.c    22 Nov 2005 18:17:32 -0000    1.24
--- src/interfaces/ecpg/pgtypeslib/numeric.c    1 Dec 2005 18:40:11 -0000
***************
*** 218,224 ****
          char       *endptr;

          (*ptr)++;
!         exponent = strtol(*ptr, &endptr, 10);
          if (endptr == (*ptr))
          {
              errno = PGTYPES_NUM_BAD_NUMERIC;
--- 218,224 ----
          char       *endptr;

          (*ptr)++;
!         exponent = pg_strtol_range(*ptr, &endptr, 10);
          if (endptr == (*ptr))
          {
              errno = PGTYPES_NUM_BAD_NUMERIC;
Index: src/port/exec.c
===================================================================
RCS file: /cvsroot/pgsql/src/port/exec.c,v
retrieving revision 1.40
diff -c -c -r1.40 exec.c
*** src/port/exec.c    22 Nov 2005 18:17:34 -0000    1.40
--- src/port/exec.c    1 Dec 2005 18:40:11 -0000
***************
*** 592,594 ****
--- 592,618 ----

      return -1;
  }
+
+
+ /*
+  *    This really should be in its own file, but it so small, it hardly
+  *    seems worth it.
+  */
+ long
+ pg_strtol_range(const char *nptr, char **endptr, int base)
+ {
+     long ret = strtol(nptr, endptr, base);
+
+     if (errno == ERANGE)
+     {
+ #ifndef FRONTEND
+         elog(ERROR, _("value not in supported range for an integer: \"%s\""), nptr);
+ #else
+         /* Keep strings identical for ease of translation */
+         fprintf(stderr, _("value not in supported range for an integer: \"%s\""), nptr);
+         fputc('\n', stderr);
+ #endif
+         return LONG_MAX;
+     }
+     return ret;
+ }