Re: Minor issues in .pgpass - Mailing list pgsql-hackers

On Mon, Mar 2, 2020 at 6:07 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2020/02/29 0:46, Hamid Akhtar wrote:
> The following review has been posted through the commitfest application:
> make installcheck-world:  not tested
> Implements feature:       not tested
> Spec compliant:           not tested
> Documentation:            not tested
>
> First of all, this seems like fixing a valid issue, albeit, the probability of somebody messing is low, but it is still better to fix this problem.
>
> I've not tested the patch in any detail, however, there are a couple of comments I have before I proceed on with detailed testing.

Thanks for the review and comments!

> 1. pgindent is showing a few issues with formatting. Please have a look and resolve those.

Yes.

> 2. I think you can potentially use "len" variable instead of introducing "buflen" and "tmplen" variables.

Basically I don't want to use the same variable for several purposes
because which would decrease the code readability.

> Also, I would choose a more appropriate name for "tmp" variable.

Yeah, so what about "rest" as the variable name?

> I believe if you move the following lines before the conditional statement and simply and change the if statement to "if (len >= sizeof(buf) - 1)", it will serve the purpose.

ISTM that this doesn't work correctly when the "buf" contains
trailing carriage returns but not newlines (i.e., this line is too long
so the "buf" doesn't include newline). In this case, pg_strip_crlf()
shorten the "buf" and then its return value "len" should become
less than sizeof(buf). So the following condition always becomes
false unexpectedly in that case even though there is still rest of
the line to eat.

Per code comments for pg_strip_crlf:

"pg_strip_crlf -- Remove any trailing newline and carriage return"

 

If the buf read contains a newline or a carriage return at the end, then clearly the line

is not exceeding the sizeof(buf). If alternatively, it doesn't, then pg_strip_crlf will have 

no effect on string length and for any lines exceeding sizeof(buf), the following conditional

statement becomes true.

> +               if (len >= sizeof(buf) - 1)
> +               {
> +                       char    tmp[LINELEN];

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

--

Highgo Software (Canada/China/Pakistan)
URL : www.highgo.ca
ADDR: 10318 WHALLEY BLVD, Surrey, BC
CELL：+923335449950  EMAIL: mailto:hamid.akhtar@highgo.ca

SKYPE: engineeredvirus