Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Rework refactoring of hex and encoding routines

  1. Moving other hex functions to /common

    Bruce Momjian <bruce@momjian.us> — 2020-12-31T00:35:57Z

    I now understand the wisdom of moving the remaining hex functions to
/common.  I know someone already suggested that, and the attached patch
does this.

I will add the attached patch to the commitfest so I can get cfbot
testing.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EnterpriseDB                             https://enterprisedb.com

  The usefulness of a cup is in its emptiness, Bruce Lee

  2. Re: Moving other hex functions to /common

    Bruce Momjian <bruce@momjian.us> — 2020-12-31T01:22:07Z

    On Wed, Dec 30, 2020 at 07:35:57PM -0500, Bruce Momjian wrote:
> I now understand the wisdom of moving the remaining hex functions to
> /common.  I know someone already suggested that, and the attached patch
> does this.
> 
> I will add the attached patch to the commitfest so I can get cfbot
> testing.

So, I am learning this cfbot thing.  Seems I need -M100% to disable
rename detection for diffs to work with cfbot --- makes sense.
New patch attached.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EnterpriseDB                             https://enterprisedb.com

  The usefulness of a cup is in its emptiness, Bruce Lee

  3. Re: Moving other hex functions to /common

    Michael Paquier <michael@paquier.xyz> — 2020-12-31T06:10:29Z

    On Wed, Dec 30, 2020 at 08:22:07PM -0500, Bruce Momjian wrote:
> So, I am learning this cfbot thing.  Seems I need -M100% to disable
> rename detection for diffs to work with cfbot --- makes sense.

A good way to make sure that a patch format is correct for the CF bot
would be to use "git format-patch -1" to generate a patch from a
single commit.

> New patch attached.

I think that this patch would have more value if we remove completely
the hex routines from ECPG and have ECPG use what's moved in
src/common/, meaning the following changes:
- Remove the exit(), pg_log_fatal() and ereport() calls from
src/common/hex.c, replace the error code paths with -1, and return a
signed result.
- The ECPG copies make no use of ecpg_raise(), so things map easily.
- This means keeping small wrappers in encode.c able to generate those
ereport(FATAL) in the backend, but that's just necessary for the
decode routine that's the only thing using get_hex().
- Let's prefix the routines in src/common/ with "pg_", to be
consistent with base64.
- It would be good to document the top each routine in hex.c (see
base64.c for a similar reference).
--
Michael

  4. Re: Moving other hex functions to /common

    Bruce Momjian <bruce@momjian.us> — 2021-01-01T20:06:13Z

    On Thu, Dec 31, 2020 at 03:10:29PM +0900, Michael Paquier wrote:
> On Wed, Dec 30, 2020 at 08:22:07PM -0500, Bruce Momjian wrote:
> > So, I am learning this cfbot thing.  Seems I need -M100% to disable
> > rename detection for diffs to work with cfbot --- makes sense.
> 
> A good way to make sure that a patch format is correct for the CF bot
> would be to use "git format-patch -1" to generate a patch from a
> single commit.

Thanks.  I had to learn how to squash my commits into a new branch and
then generate a format-patch on that:

	https://bugsdb.com/_en/debug/8b648ec395b86be32efa9629cb006d74

I wanted to see how the cfbot liked my original patch with the renames,
and it didn't, so now I know I have to use this method for the
commitfest.  Patch attached.

> > New patch attached.
> 
> I think that this patch would have more value if we remove completely
> the hex routines from ECPG and have ECPG use what's moved in
> src/common/, meaning the following changes:
> - Remove the exit(), pg_log_fatal() and ereport() calls from
> src/common/hex.c, replace the error code paths with -1, and return a
> signed result.
> - The ECPG copies make no use of ecpg_raise(), so things map easily.
> - This means keeping small wrappers in encode.c able to generate those
> ereport(FATAL) in the backend, but that's just necessary for the
> decode routine that's the only thing using get_hex().
> - Let's prefix the routines in src/common/ with "pg_", to be
> consistent with base64.
> - It would be good to document the top each routine in hex.c (see
> base64.c for a similar reference).

Let me get my patch building on the cfbot and then I will address each
of these.  I am trying to do one stage at a time since I am still
learning the process.  Thanks.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EnterpriseDB                             https://enterprisedb.com

  The usefulness of a cup is in its emptiness, Bruce Lee

  5. Re: Moving other hex functions to /common

    Michael Paquier <michael@paquier.xyz> — 2021-01-02T05:25:33Z

    On Fri, Jan 01, 2021 at 03:06:13PM -0500, Bruce Momjian wrote:
> Thanks.  I had to learn how to squash my commits into a new branch and
> then generate a format-patch on that:
> 
> 	https://bugsdb.com/_en/debug/8b648ec395b86be32efa9629cb006d74
> 
> I wanted to see how the cfbot liked my original patch with the renames,
> and it didn't, so now I know I have to use this method for the
> commitfest.  Patch attached.

There are many ways to do that, indeed.  On my end, I use a local
branch. and then apply a set of git reset --soft before recreating a
single commit.

> Let me get my patch building on the cfbot and then I will address each
> of these.  I am trying to do one stage at a time since I am still
> learning the process.  Thanks.

No problem.  On my end, this stuff has been itching me for a couple of
days and I could not recall why..  Until I remembered that the design
of the hex APIs in your patch is weak with overflow handling because
we don't pass down to the function the size of the destination buffer.
We have finished with a similar set of issues on the past with SCRAM
and base64, with has led to CVE-2019-10164 and the improvements done
in cfc40d3.  So I think that we need to improve things in a safer way.
Mapping with the design for base64, I have finished with the attached
patch, and the following set:
+extern int64 pg_hex_decode(const char *src, int64 len, char *dst, int64 dstlen);
+extern int64 pg_hex_encode(const char *src, int64 len, char *dst, int64 dstlen);
+extern int64 pg_hex_enc_len(int64 srclen);
+extern int64 pg_hex_dec_len(int64 srclen);

This ensures that the result never overflows, which explains the
introduction of an error code for the encoding part, and does not 
use elog()/pg_log() so as external libraries can use them.  ECPG uses
long variables in a couple of places, explaining why it feels safer to
use int64.  int should give enough room to any caller of those APIs,
but there is no drawback in using a 64-bit API either, and I don't
think it is worth the risk to break ECPG either for long handling,
even if I don't believe either that folks are going to work on strings
larger than 2GB.

Things get trickier for the bytea input/output because we want more
generic error messages depending for invalid values or an incorrect
number of digits, which is why I have left the copies in encode.c.
This design could be easily extended with more types of error codes,
though I am not sure if that's worth bothering.

Even with that, this leads to much more sanity for hex buffer
manipulation in backup manifests (I don't think that using  
PG_SHAXXX_DIGEST_STRING_LENGTH is a good idea either, I'd like to get
rid of it in the long-term) and ECPG, so that's clearly a gain.

I don't have a Windows host at hand, though I think that it should
work there correctly.  What do you think about the ideas in the
attached patch?
--
Michael

  6. Re: Moving other hex functions to /common

    Bruce Momjian <bruce@momjian.us> — 2021-01-05T03:47:39Z

    On Sat, Jan  2, 2021 at 02:25:33PM +0900, Michael Paquier wrote:
> > Let me get my patch building on the cfbot and then I will address each
> > of these.  I am trying to do one stage at a time since I am still
> > learning the process.  Thanks.
> 
> No problem.  On my end, this stuff has been itching me for a couple of
> days and I could not recall why..  Until I remembered that the design
> of the hex APIs in your patch is weak with overflow handling because
> we don't pass down to the function the size of the destination buffer.
> We have finished with a similar set of issues on the past with SCRAM
> and base64, with has led to CVE-2019-10164 and the improvements done
> in cfc40d3.  So I think that we need to improve things in a safer way.
> Mapping with the design for base64, I have finished with the attached
> patch, and the following set:
> +extern int64 pg_hex_decode(const char *src, int64 len, char *dst, int64 dstlen);
> +extern int64 pg_hex_encode(const char *src, int64 len, char *dst, int64 dstlen);
> +extern int64 pg_hex_enc_len(int64 srclen);
> +extern int64 pg_hex_dec_len(int64 srclen);

I can see the value of passing the destination length to the hex
functions, and I think you have to pass the src length to pg_hex_encode
since the input can be binary.  I assume the pg_hex_decode doesn't need
the source length because it is a null-terminated C string, right? 
However, pg_base64_decode(const char *src, size_t len, char *dst) does
pass in the src length, so I can see we should just make it the same.  I
also agree it is unclear if 'len' is the src or dst len, so your patch
fixes that with the names.  Also, is there a reason you use int64
instead of the size_t used by bytea?

> This ensures that the result never overflows, which explains the
> introduction of an error code for the encoding part, and does not 
> use elog()/pg_log() so as external libraries can use them.  ECPG uses
> long variables in a couple of places, explaining why it feels safer to
> use int64.  int should give enough room to any caller of those APIs,
> but there is no drawback in using a 64-bit API either, and I don't
> think it is worth the risk to break ECPG either for long handling,
> even if I don't believe either that folks are going to work on strings
> larger than 2GB.

Might as well just have hex match what bytea and esc does.

> Things get trickier for the bytea input/output because we want more
> generic error messages depending for invalid values or an incorrect
> number of digits, which is why I have left the copies in encode.c.
> This design could be easily extended with more types of error codes,
> though I am not sure if that's worth bothering.

I don't think removing the two error type reporting from src/common, and
then having to add it back into adt/encode.c makes sense.  There are
two, soon three, that want those two error reports, so I am thinking we
are best to just leave ecpg alone since it is just a library that
doesn't want more than one error reporting.  I don't think we will have
many more library call cases.

> Even with that, this leads to much more sanity for hex buffer
> manipulation in backup manifests (I don't think that using  
> PG_SHAXXX_DIGEST_STRING_LENGTH is a good idea either, I'd like to get
> rid of it in the long-term) and ECPG, so that's clearly a gain.
> 
> I don't have a Windows host at hand, though I think that it should
> work there correctly.  What do you think about the ideas in the
> attached patch?

I think your patch has a few mistakes.  First, I think you removed the
hex_decode files from the file system, rather than removing it via git,
so the diff didn't apply in the cfbot.  Second, I don't think ecpg ever
used libpgcommon before.  For non-Windows, libpgcommon got referenced
anyway in the build, so non-Windows compiles worked, but in Windows,
that was not referenced, meaning the cfbot failed on ecpglib.  I have
modified the attached patch with both of these fixed --- let's see how
it likes this version.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EnterpriseDB                             https://enterprisedb.com

  The usefulness of a cup is in its emptiness, Bruce Lee

  7. Re: Moving other hex functions to /common

    Michael Paquier <michael@paquier.xyz> — 2021-01-05T06:47:59Z

    On Mon, Jan 04, 2021 at 10:47:39PM -0500, Bruce Momjian wrote:
> I can see the value of passing the destination length to the hex
> functions, and I think you have to pass the src length to pg_hex_encode
> since the input can be binary.  I assume the pg_hex_decode doesn't need
> the source length because it is a null-terminated C string, right?

I don't think that we should assume that this is always the case,
actually.  That would be an assumption easy to miss for callers of
those routines. 

> However, pg_base64_decode(const char *src, size_t len, char *dst) does
> pass in the src length, so I can see we should just make it the same.  I
> also agree it is unclear if 'len' is the src or dst len, so your patch
> fixes that with the names.  Also, is there a reason you use int64
> instead of the size_t used by bytea?

For the argument type, sure you could just use size_t, using an int
just looked more consistent to me to match with the style of
base64.c.

> I don't think removing the two error type reporting from src/common, and
> then having to add it back into adt/encode.c makes sense.  There are
> two, soon three, that want those two error reports, so I am thinking we
> are best to just leave ecpg alone since it is just a library that
> doesn't want more than one error reporting.  I don't think we will have
> many more library call cases.

Hmm.  Even for libpq?  I have grown allergic to the addition of more
"ifdef FRONTEND" and elog()/pg_log() calls into src/common/.

> I think your patch has a few mistakes.  First, I think you removed the
> hex_decode files from the file system, rather than removing it via git,
> so the diff didn't apply in the cfbot.

Oh, indeed, thanks.  I missed that the patch should have used
/dev/null for the removed files.

> Second, I don't think ecpg ever used libpgcommon before.

Yep.  That would be the first time.  I don't see anything that would
prevent its use, though I may be missing something.

> For non-Windows, libpgcommon got referenced
> anyway in the build, so non-Windows compiles worked, but in Windows,
> that was not referenced, meaning the cfbot failed on ecpglib.  I have
> modified the attached patch with both of these fixed --- let's see how
> it likes this version.

Indeed.  Likely I am to blame for not having my Windows machine at
hand these days.  I'll have this environment available only next week
:)

FWIW, I think that this refactoring has more value iff we are able to
remove all the duplicate hex implementations we have in the tree,
while being able to cover the case you are looking for frontend tools
able to do logging.  Merging ECPG and the backend requires switching
to a logic where we return more than one error code so we could just
use an enum for the result result à-la-PQping like in libpq.
--
Michael

  8. Re: Moving other hex functions to /common

    Thomas Munro <thomas.munro@gmail.com> — 2021-01-05T07:54:11Z

    On Tue, Jan 5, 2021 at 4:47 PM Bruce Momjian <bruce@momjian.us> wrote:
> ... let's see how it likes this version.

cfbot ideally processes a new patch fairly quickly but I didn't think
of ".diff.gz" when writing the regexp to recognise patch files.  I
just tweaked the relevant regexp and it's building your patch now.
Sorry about that.  :-)

  9. Re: Moving other hex functions to /common

    Bruce Momjian <bruce@momjian.us> — 2021-01-05T17:21:09Z

    On Tue, Jan  5, 2021 at 03:47:59PM +0900, Michael Paquier wrote:
> On Mon, Jan 04, 2021 at 10:47:39PM -0500, Bruce Momjian wrote:
> > I can see the value of passing the destination length to the hex
> > functions, and I think you have to pass the src length to pg_hex_encode
> > since the input can be binary.  I assume the pg_hex_decode doesn't need
> > the source length because it is a null-terminated C string, right?
> 
> I don't think that we should assume that this is always the case,
> actually.  That would be an assumption easy to miss for callers of
> those routines. 

Well, if the backend uses /common for hex like I suggested, and like we
do now, it has to match the function signatures of bytea and esc, see
struct pg_encoding.  I don't see the point in changing those.

> > However, pg_base64_decode(const char *src, size_t len, char *dst) does
> > pass in the src length, so I can see we should just make it the same.  I

Sorry, I was wrong in the above --- len is the destination length.

> > also agree it is unclear if 'len' is the src or dst len, so your patch
> > fixes that with the names.  Also, is there a reason you use int64
> > instead of the size_t used by bytea?
> 
> For the argument type, sure you could just use size_t, using an int
> just looked more consistent to me to match with the style of
> base64.c.

I think we would more likely match what adt/encoding.c does, and we
actually must if we are going to use /common for the backend.

> > I don't think removing the two error type reporting from src/common, and
> > then having to add it back into adt/encode.c makes sense.  There are
> > two, soon three, that want those two error reports, so I am thinking we
> > are best to just leave ecpg alone since it is just a library that
> > doesn't want more than one error reporting.  I don't think we will have
> > many more library call cases.
> 
> Hmm.  Even for libpq?  I have grown allergic to the addition of more
> "ifdef FRONTEND" and elog()/pg_log() calls into src/common/.

I can understand the allergic reaction, but the other options seem
worse, and we have multiple places that need that multiple error string
reporting.

> > I think your patch has a few mistakes.  First, I think you removed the
> > hex_decode files from the file system, rather than removing it via git,
> > so the diff didn't apply in the cfbot.
> 
> Oh, indeed, thanks.  I missed that the patch should have used
> /dev/null for the removed files.
> 
> > Second, I don't think ecpg ever used libpgcommon before.
> 
> Yep.  That would be the first time.  I don't see anything that would
> prevent its use, though I may be missing something.

The cfbot is all green for this patch now, so we are making progress.  ;-)

> > For non-Windows, libpgcommon got referenced
> > anyway in the build, so non-Windows compiles worked, but in Windows,
> > that was not referenced, meaning the cfbot failed on ecpglib.  I have
> > modified the attached patch with both of these fixed --- let's see how
> > it likes this version.
> 
> Indeed.  Likely I am to blame for not having my Windows machine at
> hand these days.  I'll have this environment available only next week
> :)

Well, the cfbot gives us all access to Windows compiles, so we are good.

> FWIW, I think that this refactoring has more value iff we are able to
> remove all the duplicate hex implementations we have in the tree,
> while being able to cover the case you are looking for frontend tools
> able to do logging.  Merging ECPG and the backend requires switching
> to a logic where we return more than one error code so we could just
> use an enum for the result result à-la-PQping like in libpq.

I think we are best leaving ecpglib alone, since it is a library, and
just have one other hex implementation in /common for all other cases.

I found serious confusion in encoding.c:

* function pointer prototype argument names didn't match function
  prototypes
* used dlen for datalen, and data where src was used in real functions
* used len for src and dst len
* used dstlen for srclen

It was very confusing, and this attached patch fixes all of that.  I
also added the pg_ prefix you suggrested.  If we want to add dstlen to
all the functions, we have to do it for all types --- not sure it is
worth it, now that things are much clearer.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EnterpriseDB                             https://enterprisedb.com

  The usefulness of a cup is in its emptiness, Bruce Lee

  10. Re: Moving other hex functions to /common

    Bruce Momjian <bruce@momjian.us> — 2021-01-05T17:22:16Z

    On Tue, Jan  5, 2021 at 08:54:11PM +1300, Thomas Munro wrote:
> On Tue, Jan 5, 2021 at 4:47 PM Bruce Momjian <bruce@momjian.us> wrote:
> > ... let's see how it likes this version.
> 
> cfbot ideally processes a new patch fairly quickly but I didn't think
> of ".diff.gz" when writing the regexp to recognise patch files.  I
> just tweaked the relevant regexp and it's building your patch now.
> Sorry about that.  :-)

Oh, thanks.  I have been using gzip since it makes larger patches less
of a burden.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EnterpriseDB                             https://enterprisedb.com

  The usefulness of a cup is in its emptiness, Bruce Lee

  11. Re: Moving other hex functions to /common

    Michael Paquier <michael@paquier.xyz> — 2021-01-06T04:10:28Z

    On Tue, Jan 05, 2021 at 12:21:09PM -0500, Bruce Momjian wrote:
> Well, if the backend uses /common for hex like I suggested, and like we
> do now, it has to match the function signatures of bytea and esc, see
> struct pg_encoding.  I don't see the point in changing those.

Not necessarily with some thin wrappers to adapt.  And we only care
about the source string for the escape format, not for hex.  It seems
to me that a huge point in redesigning the hex APIs is that we can
make them more security-aware.  We had one CVE in 2019 for SCRAM
because of the base64 ones in src/common/ that got copied from
encode.c.  And I'd rather avoid taking any unnecessary risks here,
particularly as this is going to be used for more security-related
features.

> The cfbot is all green for this patch now, so we are making progress.  ;-)

Ah, cool.

> I think we are best leaving ecpglib alone, since it is a library, and
> just have one other hex implementation in /common for all other cases.
> 
> I found serious confusion in encoding.c:
> 
> * function pointer prototype argument names didn't match function
>   prototypes
> * used dlen for datalen, and data where src was used in real functions
> * used len for src and dst len
> * used dstlen for srclen

It would be as well just a separate cleanup patch?

> It was very confusing, and this attached patch fixes all of that.  I
> also added the pg_ prefix you suggrested.  If we want to add dstlen to
> all the functions, we have to do it for all types --- not sure it is
> worth it, now that things are much clearer.

Overflow protection is very important in my opinion here once we
expose more those functions.  Not touching ECPG at all and not making
this refactoring pluggable into shared libs is fine by me at the end
because we don't have a case for libpq yet, but I object to the lack
of protection against overflows.
--
Michael

  12. Re: Moving other hex functions to /common

    Bruce Momjian <bruce@momjian.us> — 2021-01-06T13:58:23Z

    On Wed, Jan  6, 2021 at 01:10:28PM +0900, Michael Paquier wrote:
> > It was very confusing, and this attached patch fixes all of that.  I
> > also added the pg_ prefix you suggrested.  If we want to add dstlen to
> > all the functions, we have to do it for all types --- not sure it is
> > worth it, now that things are much clearer.
> 
> Overflow protection is very important in my opinion here once we
> expose more those functions.  Not touching ECPG at all and not making
> this refactoring pluggable into shared libs is fine by me at the end
> because we don't have a case for libpq yet, but I object to the lack
> of protection against overflows.

Fine.  Do you want to add the overflow to the patch I posted, for all
encoding types?

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EnterpriseDB                             https://enterprisedb.com

  The usefulness of a cup is in its emptiness, Bruce Lee

  13. Re: Moving other hex functions to /common

    Michael Paquier <michael@paquier.xyz> — 2021-01-11T07:45:14Z

    On Wed, Jan 06, 2021 at 08:58:23AM -0500, Bruce Momjian wrote:
> Fine.  Do you want to add the overflow to the patch I posted, for all
> encoding types?

Yeah.  It looks that it would be good to be consistent as well for
escape case, so as it is possible to add a dstlen argument to struct
pg_encoding for the encoding and decoding routines.  I would also
prefer the option to remove the argument "data" from the encode and
decode length routines for the hex routines part of src/common/, even
if it forces the creation of two small wrappers in encode.c to call
the routines of src/common/.  Would you prefer if I send a patch by
myself?  Please note that anything I'd send would use directly elog()
and pg_log() instead of returning status codes for the src/common/
routines, and of course not touch ECPG, as that's the approach you are
favoring.
--
Michael

  14. Re: Moving other hex functions to /common

    Bruce Momjian <bruce@momjian.us> — 2021-01-11T16:27:30Z

    On Mon, Jan 11, 2021 at 04:45:14PM +0900, Michael Paquier wrote:
> On Wed, Jan 06, 2021 at 08:58:23AM -0500, Bruce Momjian wrote:
> > Fine.  Do you want to add the overflow to the patch I posted, for all
> > encoding types?
> 
> Yeah.  It looks that it would be good to be consistent as well for
> escape case, so as it is possible to add a dstlen argument to struct
> pg_encoding for the encoding and decoding routines.  I would also

Sure.

> prefer the option to remove the argument "data" from the encode and
> decode length routines for the hex routines part of src/common/, even
> if it forces the creation of two small wrappers in encode.c to call
> the routines of src/common/.  Would you prefer if I send a patch by

Agreed.  Having an argument that does nothing is odd.

> myself?  Please note that anything I'd send would use directly elog()
> and pg_log() instead of returning status codes for the src/common/
> routines, and of course not touch ECPG, as that's the approach you are
> favoring.

Sure, I realize the elog/pg_log is odd, but the alternatives seem worse.
You can take ownership of my hex patch and just add to it.  I know you
already did the hex length part, and have other ideas of what you want.

My key management patch needs the hex encoding in /common, so it will
wait for the application of your patch.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EnterpriseDB                             https://enterprisedb.com

  The usefulness of a cup is in its emptiness, Bruce Lee

  15. Re: Moving other hex functions to /common

    Michael Paquier <michael@paquier.xyz> — 2021-01-12T02:26:51Z

    On Mon, Jan 11, 2021 at 11:27:30AM -0500, Bruce Momjian wrote:
> Sure, I realize the elog/pg_log is odd, but the alternatives seem worse.

I guess that it depends on the use cases.  If there is no need to
worry about shared libraries, elog/pg_log would do just fine.

> You can take ownership of my hex patch and just add to it.  I know you
> already did the hex length part, and have other ideas of what you want.

OK, thanks.  I have been looking at it, and tweaked the patch as per
the attached.  That's basically what you did on the following points:
- Use size_t for the arguments, uint64 as return result.
- Leave ECPG out of the equation.
- Use pg_log()/elog() to report failures in src/common/, rather than
error codes.
- Renamed the arguments of encode.c to src, srclen, dst, dstlen.

The two only things that were not present are the set of checks for
overflows, and the adjustments for varlena.c.  The first point makes
the code of encode.c safer, as previously the code would issue a FATAL
*after* writing out-of-bound data.  Now it issues an ERROR before any
overwrite is done, and I have added some assertions as an extra safety
net.  For the second, I think that it makes the allocation pattern
easier to follow, similarly to checksum manifests.

Thoughts?
--
Michael

  16. Re: Moving other hex functions to /common

    Bruce Momjian <bruce@momjian.us> — 2021-01-12T18:13:00Z

    On Tue, Jan 12, 2021 at 11:26:51AM +0900, Michael Paquier wrote:
> The two only things that were not present are the set of checks for
> overflows, and the adjustments for varlena.c.  The first point makes
> the code of encode.c safer, as previously the code would issue a FATAL
> *after* writing out-of-bound data.  Now it issues an ERROR before any
> overwrite is done, and I have added some assertions as an extra safety
> net.  For the second, I think that it makes the allocation pattern
> easier to follow, similarly to checksum manifests.

Thanks for you work on this.  Looks good.  I posted your patch under my
key management patch and the cfbot reports all green:

	http://cfbot.cputube.org/index.html

The key management patch calls the src/common hex functions from
src/backend/crypto, pg_alterckey, and the crypto tests, and these are
all tested by make check-world.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EnterpriseDB                             https://enterprisedb.com

  The usefulness of a cup is in its emptiness, Bruce Lee

  17. Re: Moving other hex functions to /common

    Michael Paquier <michael@paquier.xyz> — 2021-01-13T02:25:02Z

    On Tue, Jan 12, 2021 at 01:13:00PM -0500, Bruce Momjian wrote:
> Thanks for you work on this.  Looks good.

I have been looking again at this patch again for a couple of hours
this morning to double-check if I have not missed anything, and I
think that we should be in good shape.  This still needs a pgindent
run but I'll take care of it.  Let's wait a bit and see if others have
any comments or objections.  If there is nothing, I'll commit what we
have here.

> I posted your patch under my key management patch and the cfbot
> reports all green:
> 
> 	http://cfbot.cputube.org/index.html
> 
> The key management patch calls the src/common hex functions from
> src/backend/crypto, pg_alterckey, and the crypto tests, and these are
> all tested by make check-world.

Ah, OK.  Nice.
--
Michael

  18. Re: Moving other hex functions to /common

    Sehrope Sarkuni <sehrope@jackdb.com> — 2021-01-13T15:00:49Z

    The length functions in src/common/hex.c should cast srclen to uint64 prior
to the shift. The current hex_enc_len(...) in encode.c performs such a
cast.

diff --git a/src/common/hex.c b/src/common/hex.c
index 0123c69697..e87aa1fd7f 100644
--- a/src/common/hex.c
+++ b/src/common/hex.c
@@ -178,7 +178,7 @@ pg_hex_decode(const char *src, size_t srclen, char
*dst, size_t dstlen)
 uint64
 pg_hex_enc_len(size_t srclen)
 {
-       return (srclen << 1);
+       return (uint64) srclen << 1;
 }

 /*
@@ -192,5 +192,5 @@ pg_hex_enc_len(size_t srclen)
 uint64
 pg_hex_dec_len(size_t srclen)
 {
-       return (srclen >> 1);
+       return (uint64) srclen >> 1;
 }

Regards,
-- Sehrope Sarkuni
Founder & CEO | JackDB, Inc. | https://www.jackdb.com/

  19. Re: Moving other hex functions to /common

    Michael Paquier <michael@paquier.xyz> — 2021-01-14T02:17:40Z

    On Wed, Jan 13, 2021 at 10:00:49AM -0500, Sehrope Sarkuni wrote:
> The length functions in src/common/hex.c should cast srclen to uint64 prior
> to the shift. The current hex_enc_len(...) in encode.c performs such a
> cast.

Thanks, Sehrope.  I have reviewed the code this morning and fixed
that, adjusted a couple of elog() strings I found inconsistent after
review and ran pgindent.  And applied it.
--
Michael

  20. Re: Moving other hex functions to /common

    Bruce Momjian <bruce@momjian.us> — 2021-01-14T02:55:34Z

    On Thu, Jan 14, 2021 at 11:17:40AM +0900, Michael Paquier wrote:
> On Wed, Jan 13, 2021 at 10:00:49AM -0500, Sehrope Sarkuni wrote:
> > The length functions in src/common/hex.c should cast srclen to uint64 prior
> > to the shift. The current hex_enc_len(...) in encode.c performs such a
> > cast.
> 
> Thanks, Sehrope.  I have reviewed the code this morning and fixed
> that, adjusted a couple of elog() strings I found inconsistent after
> review and ran pgindent.  And applied it.

Thanks.  All my key management regression tests pass on top of your
applied patch.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EnterpriseDB                             https://enterprisedb.com

  The usefulness of a cup is in its emptiness, Bruce Lee

  21. Re: Moving other hex functions to /common

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-01-14T03:11:54Z

    Bruce Momjian <bruce@momjian.us> writes:
> On Thu, Jan 14, 2021 at 11:17:40AM +0900, Michael Paquier wrote:
>> Thanks, Sehrope.  I have reviewed the code this morning and fixed
>> that, adjusted a couple of elog() strings I found inconsistent after
>> review and ran pgindent.  And applied it.

> Thanks.  All my key management regression tests pass on top of your
> applied patch.

Should the CF entry for this be closed?  The cfbot is still trying to
apply the patch, and unsurprisingly failing.

			regards, tom lane

  22. Re: Moving other hex functions to /common

    Michael Paquier <michael@paquier.xyz> — 2021-01-14T04:05:28Z

    On Wed, Jan 13, 2021 at 10:11:54PM -0500, Tom Lane wrote:
> Should the CF entry for this be closed?  The cfbot is still trying to
> apply the patch, and unsurprisingly failing.

Yes, I was going to close it once I was sure that the buildfarm had
nothing to say.  Done now.
--
Michael