Re: [HACKERS] Unnecessary pointer-NULL checks in pgp-pgsql.c
* Robert Haas (robertmh...@gmail.com) wrote: (BTW, why do you not leave a blank line between quoted text and your responses?) +1. That vertical space is really helpful, at least to me. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Unnecessary pointer-NULL checks in pgp-pgsql.c
On Thu, Feb 5, 2015 at 5:31 AM, Stephen Frost sfr...@snowman.net wrote: * Robert Haas (robertmh...@gmail.com) wrote: (BTW, why do you not leave a blank line between quoted text and your responses?) +1. That vertical space is really helpful, at least to me. Will do if people here are better with that. I just did it to keep the messages shorter. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Unnecessary pointer-NULL checks in pgp-pgsql.c
Robert Haas wrote: I wrote: src, dst and ctx are created respectively from mbuf_create_from_data, mbuf_create and pgp_init which never return NULL and they are palloc'd all the time. I think that we could simplify things with the patch attached, note that I added an assertion for correctness but I don't really think that it is much necessary. Yeah, I'd drop the assertion. Also, how about changing things around slightly so that we lose the goto-label construct? There's only one goto, and its only about 6 lines before the label, so we could just flip the sense of the if-test and put the code that gets skipped inside the if-block. Good idea. This gives the patch attached then. Regards, -- Michael *** a/contrib/pgcrypto/pgp-pgsql.c --- b/contrib/pgcrypto/pgp-pgsql.c *** *** 575,609 decrypt_internal(int is_pubenc, int need_text, text *data, err = pgp_set_symkey(ctx, (uint8 *) VARDATA(key), VARSIZE(key) - VARHDRSZ); ! /* ! * decrypt ! */ if (err = 0) err = pgp_decrypt(ctx, src, dst); ! /* ! * failed? ! */ ! if (err 0) ! goto out; ! ! if (ex.expect) ! check_expect(ctx, ex); ! /* remember the setting */ ! got_unicode = pgp_get_unicode_mode(ctx); ! out: ! if (src) ! mbuf_free(src); ! if (ctx) ! pgp_free(ctx); if (err) { px_set_debug_handler(NULL); ! if (dst) ! mbuf_free(dst); ereport(ERROR, (errcode(ERRCODE_EXTERNAL_ROUTINE_INVOCATION_EXCEPTION), errmsg(%s, px_strerror(err; --- 575,599 err = pgp_set_symkey(ctx, (uint8 *) VARDATA(key), VARSIZE(key) - VARHDRSZ); ! /* decrypt */ if (err = 0) + { err = pgp_decrypt(ctx, src, dst); ! if (ex.expect) ! check_expect(ctx, ex); ! /* remember the setting */ ! got_unicode = pgp_get_unicode_mode(ctx); ! } ! mbuf_free(src); ! pgp_free(ctx); if (err) { px_set_debug_handler(NULL); ! mbuf_free(dst); ereport(ERROR, (errcode(ERRCODE_EXTERNAL_ROUTINE_INVOCATION_EXCEPTION), errmsg(%s, px_strerror(err; -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Unnecessary pointer-NULL checks in pgp-pgsql.c
On Wed, Feb 4, 2015 at 7:00 AM, Michael Paquier michael.paqu...@gmail.com wrote: Robert Haas wrote: I wrote: src, dst and ctx are created respectively from mbuf_create_from_data, mbuf_create and pgp_init which never return NULL and they are palloc'd all the time. I think that we could simplify things with the patch attached, note that I added an assertion for correctness but I don't really think that it is much necessary. Yeah, I'd drop the assertion. Also, how about changing things around slightly so that we lose the goto-label construct? There's only one goto, and its only about 6 lines before the label, so we could just flip the sense of the if-test and put the code that gets skipped inside the if-block. Good idea. This gives the patch attached then. Committed. (BTW, why do you not leave a blank line between quoted text and your responses?) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Unnecessary pointer-NULL checks in pgp-pgsql.c
On Sun, Feb 1, 2015 at 9:14 PM, Michael Paquier michael.paqu...@gmail.com wrote: Coverity is pointing out that we are doing pointer-NULL checks on things that cannot be NULL in decrypt_internal(): out: - if (src) - mbuf_free(src); - if (ctx) - pgp_free(ctx); + Assert(ctx != NULL src != NULL dst != NULL); + mbuf_free(src); + pgp_free(ctx); if (err) { px_set_debug_handler(NULL); - if (dst) - mbuf_free(dst); + mbuf_free(dst); src, dst and ctx are created respectively from mbuf_create_from_data, mbuf_create and pgp_init which never return NULL and they are palloc'd all the time. I think that we could simplify things with the patch attached, note that I added an assertion for correctness but I don't really think that it is much necessary. Yeah, I'd drop the assertion. Also, how about changing things around slightly so that we lose the goto-label construct? There's only one goto, and its only about 6 lines before the label, so we could just flip the sense of the if-test and put the code that gets skipped inside the if-block. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Unnecessary pointer-NULL checks in pgp-pgsql.c
Hi all, Coverity is pointing out that we are doing pointer-NULL checks on things that cannot be NULL in decrypt_internal(): out: - if (src) - mbuf_free(src); - if (ctx) - pgp_free(ctx); + Assert(ctx != NULL src != NULL dst != NULL); + mbuf_free(src); + pgp_free(ctx); if (err) { px_set_debug_handler(NULL); - if (dst) - mbuf_free(dst); + mbuf_free(dst); src, dst and ctx are created respectively from mbuf_create_from_data, mbuf_create and pgp_init which never return NULL and they are palloc'd all the time. I think that we could simplify things with the patch attached, note that I added an assertion for correctness but I don't really think that it is much necessary. Regards, -- Michael diff --git a/contrib/pgcrypto/pgp-pgsql.c b/contrib/pgcrypto/pgp-pgsql.c index 1a0e710..af1a990 100644 --- a/contrib/pgcrypto/pgp-pgsql.c +++ b/contrib/pgcrypto/pgp-pgsql.c @@ -594,16 +594,14 @@ decrypt_internal(int is_pubenc, int need_text, text *data, got_unicode = pgp_get_unicode_mode(ctx); out: - if (src) - mbuf_free(src); - if (ctx) - pgp_free(ctx); + Assert(ctx != NULL src != NULL dst != NULL); + mbuf_free(src); + pgp_free(ctx); if (err) { px_set_debug_handler(NULL); - if (dst) - mbuf_free(dst); + mbuf_free(dst); ereport(ERROR, (errcode(ERRCODE_EXTERNAL_ROUTINE_INVOCATION_EXCEPTION), errmsg(%s, px_strerror(err; -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers