Code review for array_fill patch: fix inadequate check for array size overflow

and bogus documentation (dimension arrays are int[] not anyarray).  Also the
errhint() messages seem to be really errdetail(), since there is nothing
heuristic about them.  Some other trivial cosmetic improvements.

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index fb77ae43ea453f164e515a86ad5b13454d9508d7..15162ed5ed31033d44d1b4201e228c5e4a31c4bc 100644 (file)
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -1,4 +1,4 @@
-
+
 
  
   Functions and Operators
@@ -9186,17 +9186,16 @@ SELECT NULLIF(value, '(none)') ...
   
  
 
-
  
   Array Functions and Operators
 
   
     shows the operators
-   available for array types.
+   available for array types.
   
 
     
-     <span class="marked"><type>array</type></span> Operators
+     <span class="marked">Array</span> Operators
      
       
        
@@ -9326,7 +9325,7 @@ SELECT NULLIF(value, '(none)') ...
   
 
     
-     <span class="marked"><type>array</type></span> Functions
+     <span class="marked">Array</span> Functions
      
       
        
@@ -9374,13 +9373,13 @@ SELECT NULLIF(value, '(none)') ...
        
         
          
-          array_fill(anyelement, anyarray,
-          , anyarray)
+          array_fill(anyelement, int[],
+          , int[])
          
         
         anyarray
-        returns an array initialized with supplied value,
-        dimensions, and lower bounds
+        returns an array initialized with supplied value and
+         dimensions, optionally with lower bounds other than 1
         array_fill(7, ARRAY[3], ARRAY[2])
         [2:4]={7,7,7}
        

diff --git a/src/backend/utils/adt/arrayfuncs.c b/src/backend/utils/adt/arrayfuncs.c
index 6c810025e5ef5e1197224d0c25b7abadc8bd105b..5f3356f5600a6a8b13b83c666c54450aedb9e6d6 100644 (file)
--- a/src/backend/utils/adt/arrayfuncs.c
+++ b/src/backend/utils/adt/arrayfuncs.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *   $PostgreSQL: pgsql/src/backend/utils/adt/arrayfuncs.c,v 1.146 2008/07/16 00:48:53 momjian Exp $
+ *   $PostgreSQL: pgsql/src/backend/utils/adt/arrayfuncs.c,v 1.147 2008/07/21 04:47:00 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -97,9 +97,9 @@ static void array_insert_slice(ArrayType *destArray, ArrayType *origArray,
 static int array_cmp(FunctionCallInfo fcinfo);
 static ArrayType *create_array_envelope(int ndims, int *dimv, int *lbv, int nbytes,
                Oid elmtype, int dataoffset);
-static ArrayType *array_fill_internal(ArrayType *dims, ArrayType *lbs, Datum value, 
-                       Oid elmtype, bool isnull, 
-                       FunctionCallInfo fcinfo);
+static ArrayType *array_fill_internal(ArrayType *dims, ArrayType *lbs,
+                                     Datum value, bool isnull, Oid elmtype,
+                                     FunctionCallInfo fcinfo);
 
 
 /*
@@ -4245,7 +4245,7 @@ typedef struct generate_subscripts_fctx
         bool    reverse;
 } generate_subscripts_fctx;
 
-/* 
+/*
  * generate_subscripts(array anyarray, dim int [, reverse bool])
  *     Returns all subscripts of the array for any dimension
  */
@@ -4335,7 +4335,7 @@ array_fill_with_lower_bounds(PG_FUNCTION_ARGS)
    bool    isnull;
 
    if (PG_ARGISNULL(1) || PG_ARGISNULL(2))
-       ereport(ERROR, 
+       ereport(ERROR,
                (errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
                 errmsg("dimension array or low bound array cannot be NULL")));
 
@@ -4353,11 +4353,11 @@ array_fill_with_lower_bounds(PG_FUNCTION_ARGS)
        isnull = true;
    }
 
-   elmtype = get_fn_expr_argtype(fcinfo->flinfo, 0); 
-   if (!OidIsValid(elmtype)) 
-       elog(ERROR, "could not determine data type of input"); 
+   elmtype = get_fn_expr_argtype(fcinfo->flinfo, 0);
+   if (!OidIsValid(elmtype))
+       elog(ERROR, "could not determine data type of input");
 
-   result = array_fill_internal(dims, lbs, value, elmtype, isnull, fcinfo);
+   result = array_fill_internal(dims, lbs, value, isnull, elmtype, fcinfo);
    PG_RETURN_ARRAYTYPE_P(result);
 }
 
@@ -4375,7 +4375,7 @@ array_fill(PG_FUNCTION_ARGS)
    bool    isnull;
 
    if (PG_ARGISNULL(1))
-       ereport(ERROR, 
+       ereport(ERROR,
                (errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
                 errmsg("dimension array or low bound array cannot be NULL")));
 
@@ -4392,17 +4392,17 @@ array_fill(PG_FUNCTION_ARGS)
        isnull = true;
    }
 
-   elmtype = get_fn_expr_argtype(fcinfo->flinfo, 0); 
-   if (!OidIsValid(elmtype)) 
-       elog(ERROR, "could not determine data type of input"); 
+   elmtype = get_fn_expr_argtype(fcinfo->flinfo, 0);
+   if (!OidIsValid(elmtype))
+       elog(ERROR, "could not determine data type of input");
 
-   result = array_fill_internal(dims, NULL, value, elmtype, isnull, fcinfo);
+   result = array_fill_internal(dims, NULL, value, isnull, elmtype, fcinfo);
    PG_RETURN_ARRAYTYPE_P(result);
 }
 
 static ArrayType *
 create_array_envelope(int ndims, int *dimv, int *lbsv, int nbytes,
-               Oid elmtype, int dataoffset)
+                     Oid elmtype, int dataoffset)
 {
    ArrayType *result;
 
@@ -4418,9 +4418,9 @@ create_array_envelope(int ndims, int *dimv, int *lbsv, int nbytes,
 }
 
 static ArrayType *
-array_fill_internal(ArrayType *dims, ArrayType *lbs, Datum value, 
-                       Oid elmtype, bool isnull,
-                       FunctionCallInfo fcinfo)
+array_fill_internal(ArrayType *dims, ArrayType *lbs,
+                   Datum value, bool isnull, Oid elmtype,
+                   FunctionCallInfo fcinfo)
 {
    ArrayType   *result;
    int *dimv;
@@ -4428,34 +4428,34 @@ array_fill_internal(ArrayType *dims, ArrayType *lbs, Datum value,
    int ndims;
    int nitems;
    int         deflbs[MAXDIM];
-   int16 elmlen; 
-   bool elmbyval; 
+   int16 elmlen;
+   bool elmbyval;
    char elmalign;
    ArrayMetaState      *my_extra;
 
-   /* 
+   /*
     * Params checks
     */
    if (ARR_NDIM(dims) != 1)
        ereport(ERROR,
                (errcode(ERRCODE_ARRAY_SUBSCRIPT_ERROR),
                 errmsg("wrong number of array subscripts"),
-                errhint("Dimension array must be one dimensional.")));
+                errdetail("Dimension array must be one dimensional.")));
 
    if (ARR_LBOUND(dims)[0] != 1)
        ereport(ERROR,
                (errcode(ERRCODE_ARRAY_SUBSCRIPT_ERROR),
                 errmsg("wrong range of array_subscripts"),
-                errhint("Lower bound of dimension array must be one.")));
-   
+                errdetail("Lower bound of dimension array must be one.")));
+
    if (ARR_HASNULL(dims))
-       ereport(ERROR, 
+       ereport(ERROR,
                (errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
                 errmsg("dimension values cannot be null")));
 
    dimv = (int *) ARR_DATA_PTR(dims);
    ndims = ARR_DIMS(dims)[0];
-   
+
    if (ndims < 0)              /* we do allow zero-dimension arrays */
        ereport(ERROR,
                (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
@@ -4465,23 +4465,23 @@ array_fill_internal(ArrayType *dims, ArrayType *lbs, Datum value,
                (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
                 errmsg("number of array dimensions (%d) exceeds the maximum allowed (%d)",
                        ndims, MAXDIM)));
-   
+
    if (lbs != NULL)
    {
        if (ARR_NDIM(lbs) != 1)
            ereport(ERROR,
                    (errcode(ERRCODE_ARRAY_SUBSCRIPT_ERROR),
                         errmsg("wrong number of array subscripts"),
-                        errhint("Dimension array must be one dimensional.")));
+                        errdetail("Dimension array must be one dimensional.")));
 
        if (ARR_LBOUND(lbs)[0] != 1)
            ereport(ERROR,
                (errcode(ERRCODE_ARRAY_SUBSCRIPT_ERROR),
                     errmsg("wrong range of array_subscripts"),
-                    errhint("Lower bound of dimension array must be one.")));
-   
+                    errdetail("Lower bound of dimension array must be one.")));
+
        if (ARR_HASNULL(lbs))
-           ereport(ERROR, 
+           ereport(ERROR,
                (errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
                 errmsg("dimension values cannot be null")));
 
@@ -4489,14 +4489,14 @@ array_fill_internal(ArrayType *dims, ArrayType *lbs, Datum value,
            ereport(ERROR,
                    (errcode(ERRCODE_ARRAY_SUBSCRIPT_ERROR),
                     errmsg("wrong number of array_subscripts"),
-                    errhint("Low bound array has different size than dimensions array.")));
-                    
+                    errdetail("Low bound array has different size than dimensions array.")));
+
        lbsv = (int *) ARR_DATA_PTR(lbs);
    }
-   else    
+   else
    {
        int i;
-   
+
        for (i = 0; i < MAXDIM; i++)
            deflbs[i] = 1;
 
@@ -4506,9 +4506,8 @@ array_fill_internal(ArrayType *dims, ArrayType *lbs, Datum value,
    /* fast track for empty array */
    if (ndims == 0)
        return construct_empty_array(elmtype);
-   
-   nitems = ArrayGetNItems(ndims, dimv);
 
+   nitems = ArrayGetNItems(ndims, dimv);
 
    /*
     * We arrange to look up info about element type only once per series of
@@ -4543,7 +4542,7 @@ array_fill_internal(ArrayType *dims, ArrayType *lbs, Datum value,
        int     i;
        char        *p;
        int         nbytes;
-       Datum   aux_value = value;
+       int         totbytes;
 
        /* make sure data is not toasted */
        if (elmlen == -1)
@@ -4551,40 +4550,44 @@ array_fill_internal(ArrayType *dims, ArrayType *lbs, Datum value,
 
        nbytes = att_addlength_datum(0, elmlen, value);
        nbytes = att_align_nominal(nbytes, elmalign);
+       Assert(nbytes > 0);
 
-       nbytes *= nitems;
-       /* check for overflow of total request */
-       if (!AllocSizeIsValid(nbytes))
+       totbytes = nbytes * nitems;
+
+       /* check for overflow of multiplication or total request */
+       if (totbytes / nbytes != nitems ||
+           !AllocSizeIsValid(totbytes))
            ereport(ERROR,
                    (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
                     errmsg("array size exceeds the maximum allowed (%d)",
                            (int) MaxAllocSize)));
 
-       nbytes += ARR_OVERHEAD_NONULLS(ndims);
-       result = create_array_envelope(ndims, dimv, lbsv, nbytes,
-                           elmtype, 0);
+       /*
+        * This addition can't overflow, but it might cause us to go past
+        * MaxAllocSize.  We leave it to palloc to complain in that case.
+        */
+       totbytes += ARR_OVERHEAD_NONULLS(ndims);
+
+       result = create_array_envelope(ndims, dimv, lbsv, totbytes,
+                                      elmtype, 0);
+
        p = ARR_DATA_PTR(result);
        for (i = 0; i < nitems; i++)
            p += ArrayCastAndSet(value, elmlen, elmbyval, elmalign, p);
-
-       /* cleaning up detoasted copies of datum */
-       if (aux_value != value)
-           pfree((Pointer) value);
    }
    else
    {
        int nbytes;
        int dataoffset;
-       bits8   *bitmap;
 
        dataoffset = ARR_OVERHEAD_WITHNULLS(ndims, nitems);
        nbytes = dataoffset;
 
        result = create_array_envelope(ndims, dimv, lbsv, nbytes,
-                           elmtype, dataoffset);
-       bitmap = ARR_NULLBITMAP(result);
-       MemSet(bitmap, 0, (nitems + 7) / 8);
+                                      elmtype, dataoffset);
+
+       /* create_array_envelope already zeroed the bitmap, so we're done */
    }
-       
+
    return result;
 }

diff --git a/src/test/regress/expected/arrays.out b/src/test/regress/expected/arrays.out
index 7b7a01694acecf91bea64887e59803b7a7a481f7..bcb451e8080a9374b33c8cbccd81e6ea9fc82f42 100644 (file)
--- a/src/test/regress/expected/arrays.out
+++ b/src/test/regress/expected/arrays.out
@@ -988,6 +988,6 @@ select array_fill(1, array[2,2], null);
 ERROR:  dimension array or low bound array cannot be NULL
 select array_fill(1, array[3,3], array[1,1,1]);
 ERROR:  wrong number of array_subscripts
-HINT:  Low bound array has different size than dimensions array.
+DETAIL:  Low bound array has different size than dimensions array.
 select array_fill(1, array[1,2,null]);
 ERROR:  dimension values cannot be null