Re: [PATCHES] arrayfuncs: fix read of uninitialized mem
On Thu, 2004-09-16 at 09:18, Neil Conway wrote:
> I'll update the patch and apply it later today.
Applied to HEAD. The version of the patch that I applied is attached.
-Neil
Index: src/backend/utils/adt/arrayfuncs.c
===
RCS file: /home/neilc/private-cvsroot/pgsql-server/src/backend/utils/adt/arrayfuncs.c,v
retrieving revision 1.111
diff -c -r1.111 arrayfuncs.c
*** src/backend/utils/adt/arrayfuncs.c 2 Sep 2004 20:05:40 - 1.111
--- src/backend/utils/adt/arrayfuncs.c 16 Sep 2004 03:09:33 -
***
*** 896,902
k,
indx[MAXDIM];
int ndim,
! *dim,
*lb;
ArrayMetaState *my_extra;
--- 896,902
k,
indx[MAXDIM];
int ndim,
! *dims,
*lb;
ArrayMetaState *my_extra;
***
*** 937,945
typioparam = my_extra->typioparam;
ndim = ARR_NDIM(v);
! dim = ARR_DIMS(v);
lb = ARR_LBOUND(v);
! nitems = ArrayGetNItems(ndim, dim);
if (nitems == 0)
{
--- 937,945
typioparam = my_extra->typioparam;
ndim = ARR_NDIM(v);
! dims = ARR_DIMS(v);
lb = ARR_LBOUND(v);
! nitems = ArrayGetNItems(ndim, dims);
if (nitems == 0)
{
***
*** 968,978
values = (char **) palloc(nitems * sizeof(char *));
needquotes = (bool *) palloc(nitems * sizeof(bool));
p = ARR_DATA_PTR(v);
! overall_length = 1; /* [TRH] don't forget to count \0 at end. */
for (i = 0; i < nitems; i++)
{
Datum itemvalue;
! bool nq;
itemvalue = fetch_att(p, typbyval, typlen);
values[i] = DatumGetCString(FunctionCall3(&my_extra->proc,
--- 968,979
values = (char **) palloc(nitems * sizeof(char *));
needquotes = (bool *) palloc(nitems * sizeof(bool));
p = ARR_DATA_PTR(v);
! overall_length = 1; /* don't forget to count \0 at end. */
!
for (i = 0; i < nitems; i++)
{
Datum itemvalue;
! bool needquote;
itemvalue = fetch_att(p, typbyval, typlen);
values[i] = DatumGetCString(FunctionCall3(&my_extra->proc,
***
*** 983,1010
p = (char *) att_align(p, typalign);
/* count data plus backslashes; detect chars needing quotes */
! nq = (values[i][0] == '\0'); /* force quotes for empty string */
! for (tmp = values[i]; *tmp; tmp++)
{
char ch = *tmp;
overall_length += 1;
if (ch == '"' || ch == '\\')
{
! nq = true;
#ifndef TCL_ARRAYS
overall_length += 1;
#endif
}
else if (ch == '{' || ch == '}' || ch == typdelim ||
isspace((unsigned char) ch))
! nq = true;
}
! needquotes[i] = nq;
/* Count the pair of double quotes, if needed */
! if (nq)
overall_length += 2;
/* and the comma */
--- 984,1015
p = (char *) att_align(p, typalign);
/* count data plus backslashes; detect chars needing quotes */
! if (values[i][0] == '\0')
! needquote = true; /* force quotes for empty string */
! else
! needquote = false;
!
! for (tmp = values[i]; *tmp != '\0'; tmp++)
{
char ch = *tmp;
overall_length += 1;
if (ch == '"' || ch == '\\')
{
! needquote = true;
#ifndef TCL_ARRAYS
overall_length += 1;
#endif
}
else if (ch == '{' || ch == '}' || ch == typdelim ||
isspace((unsigned char) ch))
! needquote = true;
}
! needquotes[i] = needquote;
/* Count the pair of double quotes, if needed */
! if (needquote)
overall_length += 2;
/* and the comma */
***
*** 1014,1020
/*
* count total number of curly braces in output string
*/
! for (i = j = 0, k = 1; i < ndim; k *= dim[i++], j += k);
/* add explicit dimensions if required */
if (needdims)
--- 1019,1028
/*
* count total number of curly braces in output string
*/
! for (i = j = 0, k = 1; i < ndim; i++)
! k *= dims[i], j += k;
!
! dims_str[0] = '\0';
/* add explicit dimensions if required */
if (needdims)
***
*** 1023,1029
for (i = 0; i < ndim; i++)
{
! sprintf(ptr, "[%d:%d]", lb[i], lb[i] + dim[i] - 1);
ptr += strlen(ptr);
}
*ptr++ = *ASSGN;
--- 1031,1037
for (i = 0; i < ndim; i++)
{
! sprintf(ptr, "[%d:%d]", lb[i], lb[i] + dims[i] - 1);
ptr += strlen(ptr);
}
*ptr++ = *ASSGN;
***
*** 1039,1045
if (needdims)
APPENDSTR(dims_str);
APPENDCHAR('{');
! for (i = 0; i < ndim; indx[i++] = 0);
j = 0;
k = 0;
do
--- 1047,1054
if (needdims)
APPENDSTR(dims_str);
APPENDCHAR('{');
! for (i = 0; i < ndim; i++)
! indx[i] = 0;
j = 0;
k = 0;
do
***
*** 1071,1077
for (i = ndim - 1; i >= 0; i--)
{
! indx[i] = (indx[i] + 1) % dim[i];
if (indx[i])
{
APPENDCHAR(typdelim);
--- 1080,1086
for (i = ndim - 1; i >= 0; i--)
{
! indx[i] = (indx[i] + 1) % dims[i];
if (indx[i])
{
APPE
Re: [PATCHES] arrayfuncs: fix read of uninitialized mem
On Thu, 2004-09-16 at 12:19, Tom Lane wrote: > I prefer > > ptr = (foo *) malloc(sizeof(foo)); > or > ptr = (foo *) malloc(n * sizeof(foo)); > since here you will get a warning if ptr is typed as something other > than foo *. I was leaving out the cast above because IMHO that's somewhat orthogonal. If you like the cast (personally I'm in two minds about it), then I'd argue for: ptr = (foo *) malloc(sizeof(*ptr)); IMHO this is preferable because there is nothing that you need to change when the type of "foo" changes that the compiler won't warn you about. In your formulation you need to manually keep the two instances of "foo" in sync: admittedly, not a huge deal because you need to change the "foo" in the cast anyway, but IMHO it's worth enough to prefer the sizeof(lvalue) style. > No doubt you'll say that "ptr" is duplicated close together in your > preferred version, but I don't think it scales nicely to > slightly-more-complex cases. Consider for instance > > ptrs[i++] = malloc(sizeof(*ptrs[i++])); > > This is going to confuse people no matter what. I try to avoid writing "clever" code like that in any case -- using sizeof(type) would definitely make the above clearer, but IMHO that kind of complex lvalue is best avoided in the first place. > I guess at bottom it's the funny semantics of sizeof-applied-to-an- > lvalue-expression that I don't like. I think sizeof(type decl) is much > more obviously a compile-time constant. Granted, sizeof(lvalue) is a little unusual, but personally I think it's just one of those C features that might be weird coming from other languages, but is idiomatic C. Anyway, I'm not religious about this -- since other people seem to prefer the former style I'll revert the change. -Neil ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] arrayfuncs: fix read of uninitialized mem
Neil Conway <[EMAIL PROTECTED]> writes: > Personally I like the style I used, because it makes one class of > mistake more difficult -- getting the sizeof() wrong. Suppose the type > of the variable you're allocating changes -- if you use > ptr = malloc(sizeof(*ptr)); > then the code is still right, but with > ptr = malloc(sizeof(some_type)); > you need to remember to update the sizeof(); worse, the compiler won't > warn you about this. IMHO both of the above are poor style, precisely because the compiler can't warn you about it if the sizeof calculation doesn't match the pointer variable's type. I prefer ptr = (foo *) malloc(sizeof(foo)); or ptr = (foo *) malloc(n * sizeof(foo)); since here you will get a warning if ptr is typed as something other than foo *. Now admittedly you are still relying on two bits of code to match each other, namely the two occurrences of "foo" in this command --- but they are at least adjacent in the source code whereas the declaration of ptr might be some distance away. No doubt you'll say that "ptr" is duplicated close together in your preferred version, but I don't think it scales nicely to slightly-more-complex cases. Consider for instance ptrs[i++] = malloc(sizeof(*ptrs[i++])); This is going to confuse people no matter what. Either you don't write the exact same thing on both sides, or you are relying on the reader to realize the funny semantics of sizeof() and know that i isn't really bumped twice by the above (not to mention relying on the compiler to implement that correctly...). I guess at bottom it's the funny semantics of sizeof-applied-to-an- lvalue-expression that I don't like. I think sizeof(type decl) is much more obviously a compile-time constant. Possibly this is just a hangover from programming in other languages that had the one but not the other, but it's what I find understandable. regards, tom lane ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faqs/FAQ.html
Re: [PATCHES] arrayfuncs: fix read of uninitialized mem
On Thu, 2004-09-16 at 00:18, Tom Lane wrote: > I dislike what you did at lines 983-1012 (replace a local variable by > possibly-many stores into an array). Ok, I'll revert it. > Also, as long as we're fixing unreadable code, how about (line 1019) > [...] Sounds good to me. I'll update the patch and apply it later today. -Neil ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] arrayfuncs: fix read of uninitialized mem
On Thu, 2004-09-16 at 03:32, Joe Conway wrote: > Personally I prefer the original style here. Personally I like the style I used, because it makes one class of mistake more difficult -- getting the sizeof() wrong. Suppose the type of the variable you're allocating changes -- if you use ptr = malloc(sizeof(*ptr)); then the code is still right, but with ptr = malloc(sizeof(some_type)); you need to remember to update the sizeof(); worse, the compiler won't warn you about this. -Neil ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] arrayfuncs: fix read of uninitialized mem
Neil Conway wrote: Barring any objections, I'll apply the patch within 24 hours. *** *** 965,978 * (including any overhead such as escaping backslashes), and detect * whether each item needs double quotes. */ ! values = (char **) palloc(nitems * sizeof(char *)); ! needquotes = (bool *) palloc(nitems * sizeof(bool)); --- 965,978 * (including any overhead such as escaping backslashes), and detect * whether each item needs double quotes. */ ! values = (char **) palloc(nitems * sizeof(*values)); ! needquotes = (bool *) palloc(nitems * sizeof(*needquotes)); Personally I prefer the original style here. And I agree with Tom's nearby comments. But otherwise looks good to me. Joe ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send "unregister YourEmailAddressHere" to [EMAIL PROTECTED])
Re: [PATCHES] arrayfuncs: fix read of uninitialized mem
Neil Conway <[EMAIL PROTECTED]> writes: > I fixed it by initializing dims_str[0] to '\0' circa line 1018 in > current sources. While doing so I couldn't resist the temptation to fix > a few of arrayfunc.c's crimes against good programming practise, so the > attached patch includes some additional cosmetic improvements. I dislike what you did at lines 983-1012 (replace a local variable by possibly-many stores into an array). Also, as long as we're fixing unreadable code, how about (line 1019) for (i = j = 0, k = 1; i < ndim; k *= dims[i++], j += k); becomes for (i = j = 0, k = 1; i < ndim; i++) k *= dims[i], j += k; or some such. The empty loop body is a mistake waiting to happen. Looks fine to me otherwise. regards, tom lane ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faqs/FAQ.html
