Re: Moving other hex functions to /common

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