Thread: arrayfuncs: fix read of uninitialized mem

This patch fixes a read of uninitialized memory in array_out(). The code
was previously doing a strlen() on a stack-allocated char array that,
under some code paths, had never been assigned to. The problem doesn't
appear in REL7_4_STABLE, so there's no need for a backport.

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. If people
prefer I can just apply the bugfix to HEAD, and save the cleanup till we
branch for 8.1. Comments?

Barring any objections, I'll apply the patch within 24 hours.

-Neil

Neil Conway <neilc@samurai.com> 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

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

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

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

Neil Conway <neilc@samurai.com> 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

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

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