Thread: small fix to possible null pointer dereference in byteaout() varlena.c
small fix to possible null pointer dereference in byteaout() varlena.c
From
Grzegorz Jaśkiewicz
Date:
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
Attachment
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
Re: small fix to possible null pointer dereference in byteaout() varlena.c
From
Grzegorz Jaśkiewicz
Date:
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
Re: small fix to possible null pointer dereference in byteaout() varlena.c
From
Grzegorz Jaśkiewicz
Date:
got it folks. Forgot that elog doesn't return with ERROR