Re: pgcrypto: PGP armor headers

On 10/1/14 1:01 PM, Heikki Linnakangas wrote:
> On 10/01/2014 11:58 AM, Marko Tiikkaja wrote:
>> On 10/1/14, 9:11 AM, Heikki Linnakangas wrote:
>>> We have two options:
>>>
>>> 1. Throw an error if there are any non-ASCII characters in the key/value
>>> arrays.
>>> 2. Don't convert them to UTF-8, but use the current database encoding.
>>>
>>> Both seem sane to me. If we use the current database encoding, then we
>>> have to also decide what to do with the input, in pgp_armor_headers().
>>> If armor() uses the database encoding, but pgp_armor_headers() treats
>>> the input as UTF-8, then a round-trip with pgp_armor_headers(armor(?))
>>> won't work.
>>
>> Yeah.  Both options seem fine to me.  Throwing an error perhaps slightly
>> more so.
>
> I went with 1, throw an error. I also added checks that the key or value
> doesn't contain any embedded newlines, and that the key doesn't contain
> an embedded ": ". Those would cause the armor to be invalid.

Great.

> I think this is now ready for commit, but since I've changed it quite
> significantly from what you originally submitted, please take a moment
> to review this.

   1) I see this compiler warning:

     pgp-pgsql.c: In function ‘pg_armor’:
     pgp-pgsql.c:960:18: warning: ‘values’ may be used uninitialized in
this function [-Wmaybe-uninitialized]
     pgp_armor_encode((uint8 *) VARDATA(data), data_len, &buf,

      It's bogus, but worth silencing anyway.

   2) There's what looks like an extra whitespace change in
pgp_armor_encode(), but maybe that's intentional?

   3) Also, I think the attached two corner cases deserve their own tests.

Other than the above, the patch looks good to me.  Huge thanks for your
work on this one!


.marko