Обсуждение: small fix to possible null pointer dereference in byteaout() varlena.c

It would crash if input is of unrecognized format. Probably than
there's going to be more problems to be concerned with, but just in
case, don't crash in

--
GJ

Grzegorz Jaśkiewicz <gryzman@gmail.com> writes:
> It would crash if input is of unrecognized format. Probably than
> there's going to be more problems to be concerned with, but just in
> case, don't crash in

I'm not sure why you think this is a good change, but it would break
things: in particular, the code would fail to null-terminate the string
in the hex-output case.  Also, the case that you seem to be trying to
defend against can't happen because elog(ERROR) doesn't return.
        regards, tom lane

2010/9/28 Tom Lane <tgl@sss.pgh.pa.us>:
> Grzegorz Jaśkiewicz <gryzman@gmail.com> writes:
>> It would crash if input is of unrecognized format. Probably than
>> there's going to be more problems to be concerned with, but just in
>> case, don't crash in
>
> I'm not sure why you think this is a good change, but it would break
> things: in particular, the code would fail to null-terminate the string
> in the hex-output case.  Also, the case that you seem to be trying to
> defend against can't happen because elog(ERROR) doesn't return.
>

...               rp = result = NULL;             /* keep compiler quiet */       }       *rp = '\0';
....

this strikes me as a clear case of possible null pointer dereference,
wouldn't you agree ?
I know the case is very corner-ish, but still valid imo.




--
GJ

Grzegorz Jaśkiewicz <gryzman@gmail.com> writes:
> ...
>                 rp = result = NULL;             /* keep compiler quiet */
>         }
>         *rp = '\0';
> ....

> this strikes me as a clear case of possible null pointer dereference,
> wouldn't you agree ?

No, I wouldn't.  You need to enlarge your peephole by one line:
   else   {       elog(ERROR, "unrecognized bytea_output setting: %d",            bytea_output);       rp = result =
NULL;       /* keep compiler quiet */   }   *rp = '\0';
 

The "keep compiler quiet" line is unreachable code (and that comment is
supposed to remind you of that).
        regards, tom lane

2010/9/28 Grzegorz Jaśkiewicz <gryzman@gmail.com>:
> 2010/9/28 Tom Lane <tgl@sss.pgh.pa.us>:
>> Grzegorz Jaśkiewicz <gryzman@gmail.com> writes:
>>> It would crash if input is of unrecognized format. Probably than
>>> there's going to be more problems to be concerned with, but just in
>>> case, don't crash in
>>
>> I'm not sure why you think this is a good change, but it would break
>> things: in particular, the code would fail to null-terminate the string
>> in the hex-output case.  Also, the case that you seem to be trying to
>> defend against can't happen because elog(ERROR) doesn't return.
>>
>
> ...
>                rp = result = NULL;             /* keep compiler quiet */
>        }
>        *rp = '\0';
> ....
>
> this strikes me as a clear case of possible null pointer dereference,
> wouldn't you agree ?
> I know the case is very corner-ish, but still valid imo.

That comment that says "keep compiler quiet" is there because we
expect that line of code never to get executed.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

got it folks. Forgot that elog doesn't return with ERROR