On 9/9/14 11:01 AM, I wrote:
On 9/9/14 10:54 AM, Heikki Linnakangas wrote:
So I think this (and the corresponding dearmor code too) should be
refactored to use a StringInfo that gets enlarged as needed, instead of
hoping to guess the size correctly beforehand. To ease review, might
make sense to do that as a separate patch over current sources, and the
main patch on top of that.


Yeah, I thought the same thing while writing that code.  Thanks, I'll do
it that way.

Attached is what I have right now. I started working on the decoding part, but it has this piece of code:

   /* decode crc */
   if (b64_decode(p + 1, 4, buf) != 3)
       goto out;

which makes the approach a bit uglier. If I did this the same way, I would have to create and destroy a StringInfo just for this operation, which seems ugly. So I wonder if I shouldn't try and instead keep the code closer to what it is in HEAD right now; I could call enlargeStringInfo() first, then hand out a pointer to b64_encode (or b64_decode()) and finally increment StringInfoData.len by how much was actually written. That would keep the code changes a lot smaller, too.

Is either of these approaches anywhere near what you had in mind?

I'm also not sure why we need to keep a copy of the base64 encoding/decoding logic instead of exporting it in utils/adt/encode.c.


.marko
*** a/contrib/pgcrypto/pgp-armor.c
--- b/contrib/pgcrypto/pgp-armor.c
***************
*** 31,36 ****
--- 31,38 ----
  
  #include "postgres.h"
  
+ #include "lib/stringinfo.h"
+ 
  #include "px.h"
  #include "pgp.h"
  
***************
*** 38,58 ****
   * BASE64 - duplicated :(
   */
  
! static const unsigned char _base64[] =
  "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/";
  
! static int
! b64_encode(const uint8 *src, unsigned len, uint8 *dst)
  {
-       uint8      *p,
-                          *lend = dst + 76;
        const uint8 *s,
                           *end = src + len;
        int                     pos = 2;
        unsigned long buf = 0;
  
        s = src;
-       p = dst;
  
        while (s < end)
        {
--- 40,58 ----
   * BASE64 - duplicated :(
   */
  
! static const char _base64[] =
  "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/";
  
! static void
! b64_encode(const uint8 *src, unsigned len, StringInfo dst)
  {
        const uint8 *s,
                           *end = src + len;
        int                     pos = 2;
        unsigned long buf = 0;
+       int                     linepos = 0;
  
        s = src;
  
        while (s < end)
        {
***************
*** 65,93 **** b64_encode(const uint8 *src, unsigned len, uint8 *dst)
                 */
                if (pos < 0)
                {
!                       *p++ = _base64[(buf >> 18) & 0x3f];
!                       *p++ = _base64[(buf >> 12) & 0x3f];
!                       *p++ = _base64[(buf >> 6) & 0x3f];
!                       *p++ = _base64[buf & 0x3f];
  
                        pos = 2;
                        buf = 0;
                }
!               if (p >= lend)
                {
!                       *p++ = '\n';
!                       lend = p + 76;
                }
        }
        if (pos != 2)
        {
!               *p++ = _base64[(buf >> 18) & 0x3f];
!               *p++ = _base64[(buf >> 12) & 0x3f];
!               *p++ = (pos == 0) ? _base64[(buf >> 6) & 0x3f] : '=';
!               *p++ = '=';
        }
- 
-       return p - dst;
  }
  
  /* probably should use lookup table */
--- 65,92 ----
                 */
                if (pos < 0)
                {
!                       appendStringInfoCharMacro(dst, _base64[(buf >> 18) & 
0x3f]);
!                       appendStringInfoCharMacro(dst, _base64[(buf >> 12) & 
0x3f]);
!                       appendStringInfoCharMacro(dst, _base64[(buf >> 6) & 
0x3f]);
!                       appendStringInfoCharMacro(dst, _base64[buf & 0x3f]);
  
+                       linepos += 4;
                        pos = 2;
                        buf = 0;
                }
!               if (linepos >= 76)
                {
!                       appendStringInfoCharMacro(dst, '\n');
!                       linepos = 0;
                }
        }
        if (pos != 2)
        {
!               appendStringInfoCharMacro(dst, _base64[(buf >> 18) & 0x3f]);
!               appendStringInfoCharMacro(dst, _base64[(buf >> 12) & 0x3f]);
!               appendStringInfoCharMacro(dst, (pos == 0) ? _base64[(buf >> 6) 
& 0x3f] : '=');
!               appendStringInfoCharMacro(dst, '=');
        }
  }
  
  /* probably should use lookup table */
***************
*** 160,174 **** b64_decode(const uint8 *src, unsigned len, uint8 *dst)
  }
  
  static unsigned
- b64_enc_len(unsigned srclen)
- {
-       /*
-        * 3 bytes will be converted to 4, linefeed after 76 chars
-        */
-       return (srclen + 2) * 4 / 3 + srclen / (76 * 3 / 4);
- }
- 
- static unsigned
  b64_dec_len(unsigned srclen)
  {
        return (srclen * 3) >> 2;
--- 159,164 ----
***************
*** 203,240 **** crc24(const uint8 *data, unsigned len)
        return crc & 0xffffffL;
  }
  
! int
! pgp_armor_encode(const uint8 *src, unsigned len, uint8 *dst)
  {
-       int                     n;
-       uint8      *pos = dst;
        unsigned        crc = crc24(src, len);
  
!       n = strlen(armor_header);
!       memcpy(pos, armor_header, n);
!       pos += n;
  
!       n = b64_encode(src, len, pos);
!       pos += n;
  
!       if (*(pos - 1) != '\n')
!               *pos++ = '\n';
  
!       *pos++ = '=';
!       pos[3] = _base64[crc & 0x3f];
!       crc >>= 6;
!       pos[2] = _base64[crc & 0x3f];
!       crc >>= 6;
!       pos[1] = _base64[crc & 0x3f];
!       crc >>= 6;
!       pos[0] = _base64[crc & 0x3f];
!       pos += 4;
  
!       n = strlen(armor_footer);
!       memcpy(pos, armor_footer, n);
!       pos += n;
! 
!       return pos - dst;
  }
  
  static const uint8 *
--- 193,217 ----
        return crc & 0xffffffL;
  }
  
! void
! pgp_armor_encode(const uint8 *src, unsigned len, StringInfo dst)
  {
        unsigned        crc = crc24(src, len);
  
!       appendStringInfoString(dst, armor_header);
  
!       b64_encode(src, len, dst);
  
!       if (*(dst->data + dst->len - 1) != '\n')
!               appendStringInfoChar(dst, '\n');
  
!       appendStringInfoChar(dst, '=');
!       appendStringInfoChar(dst, _base64[(crc >> 18) & 0x3f]);
!       appendStringInfoChar(dst, _base64[(crc >> 12) & 0x3f]);
!       appendStringInfoChar(dst, _base64[(crc >> 6) & 0x3f]);
!       appendStringInfoChar(dst, _base64[crc & 0x3f]);
  
!       appendStringInfoString(dst, armor_footer);
  }
  
  static const uint8 *
***************
*** 370,381 **** out:
  }
  
  unsigned
- pgp_armor_enc_len(unsigned len)
- {
-       return b64_enc_len(len) + strlen(armor_header) + strlen(armor_footer) + 
16;
- }
- 
- unsigned
  pgp_armor_dec_len(unsigned len)
  {
        return b64_dec_len(len);
--- 347,352 ----
*** a/contrib/pgcrypto/pgp-pgsql.c
--- b/contrib/pgcrypto/pgp-pgsql.c
***************
*** 820,842 **** pg_armor(PG_FUNCTION_ARGS)
  {
        bytea      *data;
        text       *res;
!       int                     data_len,
!                               res_len,
!                               guess_len;
  
        data = PG_GETARG_BYTEA_P(0);
        data_len = VARSIZE(data) - VARHDRSZ;
  
!       guess_len = pgp_armor_enc_len(data_len);
!       res = palloc(VARHDRSZ + guess_len);
  
!       res_len = pgp_armor_encode((uint8 *) VARDATA(data), data_len,
!                                                          (uint8 *) 
VARDATA(res));
!       if (res_len > guess_len)
!               ereport(ERROR,
!                               
(errcode(ERRCODE_EXTERNAL_ROUTINE_INVOCATION_EXCEPTION),
!                                errmsg("Overflow - encode estimate too 
small")));
!       SET_VARSIZE(res, VARHDRSZ + res_len);
  
        PG_FREE_IF_COPY(data, 0);
        PG_RETURN_TEXT_P(res);
--- 820,839 ----
  {
        bytea      *data;
        text       *res;
!       int                     data_len;
!       StringInfoData buf;
  
        data = PG_GETARG_BYTEA_P(0);
        data_len = VARSIZE(data) - VARHDRSZ;
  
!       initStringInfo(&buf);
  
!       pgp_armor_encode((uint8 *) VARDATA(data), data_len, &buf);
! 
!       res = palloc(VARHDRSZ + buf.len);
!       SET_VARSIZE(res, VARHDRSZ + buf.len);
!       memcpy(VARDATA(res), buf.data, buf.len);
!       pfree(buf.data);
  
        PG_FREE_IF_COPY(data, 0);
        PG_RETURN_TEXT_P(res);
*** a/contrib/pgcrypto/pgp.h
--- b/contrib/pgcrypto/pgp.h
***************
*** 29,34 ****
--- 29,35 ----
   * contrib/pgcrypto/pgp.h
   */
  
+ #include "lib/stringinfo.h"
  #include "mbuf.h"
  #include "px.h"
  
***************
*** 274,280 **** void           pgp_cfb_free(PGP_CFB *ctx);
  int                   pgp_cfb_encrypt(PGP_CFB *ctx, const uint8 *data, int 
len, uint8 *dst);
  int                   pgp_cfb_decrypt(PGP_CFB *ctx, const uint8 *data, int 
len, uint8 *dst);
  
! int                   pgp_armor_encode(const uint8 *src, unsigned len, uint8 
*dst);
  int                   pgp_armor_decode(const uint8 *src, unsigned len, uint8 
*dst);
  unsigned      pgp_armor_enc_len(unsigned len);
  unsigned      pgp_armor_dec_len(unsigned len);
--- 275,281 ----
  int                   pgp_cfb_encrypt(PGP_CFB *ctx, const uint8 *data, int 
len, uint8 *dst);
  int                   pgp_cfb_decrypt(PGP_CFB *ctx, const uint8 *data, int 
len, uint8 *dst);
  
! void          pgp_armor_encode(const uint8 *src, unsigned len, StringInfo 
dst);
  int                   pgp_armor_decode(const uint8 *src, unsigned len, uint8 
*dst);
  unsigned      pgp_armor_enc_len(unsigned len);
  unsigned      pgp_armor_dec_len(unsigned len);
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to