On 2020/03/03 21:38, Hamid Akhtar wrote:
On Mon, Mar 2, 2020 at 6:07 PM Fujii Masao <masao.fu...@oss.nttdata.com <mailto:masao.fu...@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).
No if the length of the setting line exceeds sizeof(buf) and the buf contains only a carriage return at the end and not newline. This case can happen because fgets() stops reading when a newline (not a carriage return) is found. Normal users are very unlikely to add a carriage return into the middle of the pgpass setting line in practice, though. But IMO the code should handle even this case because it *can* happen, if the code is not so complicated. Regards, -- Fujii Masao NTT DATA CORPORATION Advanced Platform Technology Group Research and Development Headquarters
