Thread
Commits
-
Fix issues around .pgpass file.
- 2eb3bc58814f 13.0 landed
-
Minor issues in .pgpass
Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-01-21T06:27:50Z
Hi, When I was researching the maximum length of password in PostgreSQL to answer the question from my customer, I found that there are two minor issues in .pgpass file. (1) If the length of a line in .pgpass file is larger than 319B, libpq silently treats each 319B in the line as a separate setting line. (2) The document explains that a line beginning with # is treated as a comment in .pgpass. But as far as I read the code, there is no code doing such special handling. Whether a line begins with # or not, libpq just checks that the first token in the line match with the host. That is, if you try to connect to the host with the hostname beginning with #, it can match to the line beginning with # in .pgpass. Also if the length of that "comment" line is larger than 319B, the latter part of the line can be treated as valid setting. You may think that these unexpected behaviors are not so harmful in practice because "usually" the length of password setting line is less than 319B and the hostname beginning with # is less likely to be used. But the problem exists. And there are people who want to use large password or to write a long comment (e.g., with multibyte characters like Japanese) in .pgass, so these may be more harmful in the near future. For (1), I think that we should make libpq warn if the length of a line is larger than 319B, and throw away the remaining part beginning from 320B position. Whether to enlarge the length of a line should be a separate discussion, I think. For (2), libpq should treat any lines beginning with # as comments. I've not created the patch yet, but will do if we reach to the consensus. Regards, -- Fujii Masao NTT DATA CORPORATION Advanced Platform Technology Group Research and Development Headquarters -
Re: Minor issues in .pgpass
Daniel Gustafsson <daniel@yesql.se> — 2020-01-21T21:59:50Z
> On 21 Jan 2020, at 07:27, Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > For (2), libpq should treat any lines beginning with # as comments. I haven't read the code to confirm that it really isn't, but +1 on making it so. I can't see a reason for allowing a hostname to start with #, but allowing comments does seem useful. cheers ./daniel
-
Re: Minor issues in .pgpass
David Fetter <david@fetter.org> — 2020-01-22T00:06:21Z
On Tue, Jan 21, 2020 at 03:27:50PM +0900, Fujii Masao wrote: > Hi, > > When I was researching the maximum length of password in PostgreSQL > to answer the question from my customer, I found that there are two > minor issues in .pgpass file. > > (1) If the length of a line in .pgpass file is larger than 319B, > libpq silently treats each 319B in the line as a separate > setting line. This seems like a potentially serious bug. For example, a truncated password could get retried enough times to raise intruder alarms, and it wouldn't be easy to track down. > (2) The document explains that a line beginning with # is treated > as a comment in .pgpass. But as far as I read the code, > there is no code doing such special handling. This is a flat-out bug, as it violates a promise the documentation has made. > Also if the length of that "comment" line is larger than 319B, > the latter part of the line can be treated as valid setting. > You may think that these unexpected behaviors are not so harmful > in practice because "usually" the length of password setting line is > less than 319B and the hostname beginning with # is less likely to be > used. But the problem exists. And there are people who want to use > large password or to write a long comment (e.g., with multibyte > characters like Japanese) in .pgass, so these may be more harmful > in the near future. > > For (1), I think that we should make libpq warn if the length of a line > is larger than 319B, and throw away the remaining part beginning from > 320B position. Whether to enlarge the length of a line should be > a separate discussion, I think. Agreed. > For (2), libpq should treat any lines beginning with # as comments. Would it make sense for lines starting with whitespace and then # to be treated as comments, too, e.g.: # Please don't treat this as a parameter ? Best, David. -- David Fetter <david(at)fetter(dot)org> http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
-
Re: Minor issues in .pgpass
Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-02-12T17:01:30Z
On 2020/01/22 9:06, David Fetter wrote: > On Tue, Jan 21, 2020 at 03:27:50PM +0900, Fujii Masao wrote: >> Hi, >> >> When I was researching the maximum length of password in PostgreSQL >> to answer the question from my customer, I found that there are two >> minor issues in .pgpass file. >> >> (1) If the length of a line in .pgpass file is larger than 319B, >> libpq silently treats each 319B in the line as a separate >> setting line. > > This seems like a potentially serious bug. For example, a truncated > password could get retried enough times to raise intruder alarms, and > it wouldn't be easy to track down. > >> (2) The document explains that a line beginning with # is treated >> as a comment in .pgpass. But as far as I read the code, >> there is no code doing such special handling. > > This is a flat-out bug, as it violates a promise the documentation has > made. > >> Also if the length of that "comment" line is larger than 319B, >> the latter part of the line can be treated as valid setting. > >> You may think that these unexpected behaviors are not so harmful >> in practice because "usually" the length of password setting line is >> less than 319B and the hostname beginning with # is less likely to be >> used. But the problem exists. And there are people who want to use >> large password or to write a long comment (e.g., with multibyte >> characters like Japanese) in .pgass, so these may be more harmful >> in the near future. >> >> For (1), I think that we should make libpq warn if the length of a line >> is larger than 319B, and throw away the remaining part beginning from >> 320B position. Whether to enlarge the length of a line should be >> a separate discussion, I think. > > Agreed. > >> For (2), libpq should treat any lines beginning with # as comments. Patch attached. This patch does the above (1) and (2). > Would it make sense for lines starting with whitespace and then # to > be treated as comments, too, e.g.: Could you tell me why you want to treat such a line as comment? Basically I don't want to change the existing rules for parsing .pgpass file more thane necessary. Regards, -- Fujii Masao NTT DATA CORPORATION Advanced Platform Technology Group Research and Development Headquarters
-
Re: Minor issues in .pgpass
Hamid Akhtar <hamid.akhtar@gmail.com> — 2020-02-28T15:46:18Z
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. 1. pgindent is showing a few issues with formatting. Please have a look and resolve those. 2. I think you can potentially use "len" variable instead of introducing "buflen" and "tmplen" variables. Also, I would choose a more appropriate name for "tmp" variable. 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. ======================================== /* strip trailing newline and carriage return */ len = pg_strip_crlf(buf); if (len == 0) continue; ======================================== So, the patch should look like this in my opinion (ignore the formatting issues as this is just to give you an idea of what I mean): diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c index 408000a..6ca262f 100644 --- a/src/interfaces/libpq/fe-connect.c +++ b/src/interfaces/libpq/fe-connect.c @@ -6949,6 +6949,7 @@ passwordFromFile(const char *hostname, const char *port, const char *dbname, { FILE *fp; struct stat stat_buf; + int line_number = 0; #define LINELEN NAMEDATALEN*5 char buf[LINELEN]; @@ -7018,10 +7019,40 @@ passwordFromFile(const char *hostname, const char *port, const char *dbname, if (fgets(buf, sizeof(buf), fp) == NULL) break; - /* strip trailing newline and carriage return */ - len = pg_strip_crlf(buf); + line_number++; - if (len == 0) + /* strip trailing newline and carriage return */ + len = pg_strip_crlf(buf); + + if (len == 0) + continue; + + if (len >= sizeof(buf) - 1) + { + char tmp[LINELEN]; + + /* + * Warn if this password setting line is too long, + * because it's unexpectedly truncated. + */ + if (buf[0] != '#') + fprintf(stderr, + libpq_gettext("WARNING: line %d too long in password file \"%s\"\n"), + line_number, pgpassfile); + + /* eat rest of the line */ + while (!feof(fp) && !ferror(fp)) + { + if (fgets(tmp, sizeof(tmp), fp) == NULL) + break; + len = strlen(tmp); + if (len < sizeof(tmp) -1 || tmp[len - 1] == '\n') + break; + } + } + + /* ignore comments */ + if (buf[0] == '#') --- 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 The new status of this patch is: Waiting on Author -
Re: Minor issues in .pgpass
Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-03-02T13:07:14Z
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. > + if (len >= sizeof(buf) - 1) > + { > + char tmp[LINELEN]; Regards, -- Fujii Masao NTT DATA CORPORATION Advanced Platform Technology Group Research and Development Headquarters -
Re: Minor issues in .pgpass
Hamid Akhtar <hamid.akhtar@gmail.com> — 2020-03-03T12:38:14Z
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 -
Re: Minor issues in .pgpass
Hamid Akhtar <hamid.akhtar@gmail.com> — 2020-03-03T13:07:10Z
On Tue, Mar 3, 2020 at 5:38 PM Hamid Akhtar <hamid.akhtar@gmail.com> wrote: > > > 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. >> > That is fine. > >> > Also, I would choose a more appropriate name for "tmp" variable. >> >> Yeah, so what about "rest" as the variable name? >> > May be something like "excess_buf" or any other one that describes that these bytes are to be discarded. > >> > 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 > -- 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 -
Re: Minor issues in .pgpass
Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-03-03T15:57:16Z
On 2020/03/03 21:38, Hamid Akhtar wrote: > > > On Mon, Mar 2, 2020 at 6:07 PM Fujii Masao <masao.fujii@oss.nttdata.com <mailto: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). 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
-
Re: Minor issues in .pgpass
Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-03-04T10:04:07Z
On 2020/03/03 22:07, Hamid Akhtar wrote: > On Tue, Mar 3, 2020 at 5:38 PM Hamid Akhtar <hamid.akhtar@gmail.com <mailto:hamid.akhtar@gmail.com>> wrote: > > > > On Mon, Mar 2, 2020 at 6:07 PM Fujii Masao <masao.fujii@oss.nttdata.com <mailto: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. Fixed. Attached is the updated version of the patch. I marked this CF entry as "Needs Review" again. > > 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. > > > That is fine. > > > > Also, I would choose a more appropriate name for "tmp" variable. > > Yeah, so what about "rest" as the variable name? > > > May be something like "excess_buf" or any other one that describes that these bytes are to be discarded. Thanks for the comment! But IMO that "rest" is not so bad choice, so for now I used "rest" in the latest patch. Regards, -- Fujii Masao NTT DATA CORPORATION Advanced Platform Technology Group Research and Development Headquarters
-
Re: Minor issues in .pgpass
Hamid Akhtar <hamid.akhtar@gmail.com> — 2020-03-04T11:39:55Z
On Tue, Mar 3, 2020 at 8:57 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > > > On 2020/03/03 21:38, Hamid Akhtar wrote: > > > > > > On Mon, Mar 2, 2020 at 6:07 PM Fujii Masao <masao.fujii@oss.nttdata.com > <mailto: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). > > 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. > I'm not sure if I understand your comment here. From the code of pg_strip_crlf I see that it is handling both carriage return and/or new line at the end of a string: ============= src/common/string.c ============= while (len > 0 && (str[len - 1] == '\n' || str[len - 1] == '\r')) str[--len] = '\0'; ============= > 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 -
Re: Minor issues in .pgpass
Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-03-04T11:54:06Z
On 2020/03/04 20:39, Hamid Akhtar wrote: > > > On Tue, Mar 3, 2020 at 8:57 PM Fujii Masao <masao.fujii@oss.nttdata.com <mailto:masao.fujii@oss.nttdata.com>> wrote: > > > > On 2020/03/03 21:38, Hamid Akhtar wrote: > > > > > > On Mon, Mar 2, 2020 at 6:07 PM Fujii Masao <masao.fujii@oss.nttdata.com <mailto:masao.fujii@oss.nttdata.com> <mailto:masao.fujii@oss.nttdata.com <mailto: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). > > 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. > > > I'm not sure if I understand your comment here. From the code of pg_strip_crlf > I see that it is handling both carriage return and/or new line at the end of a > string: So if "buf" contains a carriage return at the end, it's removed and the "len" that pg_strip_crlf() returns obviously should be smaller than sizeof(buf). This causes the following condition that you proposed as follows to always be false (i.e., len < sizeof(buf) - 1) even when there are still rest of line. So we cannot eat rest of the line even though it exists. I'm missing something? + if (len >= sizeof(buf) - 1) + { + char tmp[LINELEN]; + + /* + * Warn if this password setting line is too long, + * because it's unexpectedly truncated. + */ + if (buf[0] != '#') + fprintf(stderr, + libpq_gettext("WARNING: line %d too long in password file \"%s\"\n"), + line_number, pgpassfile); + + /* eat rest of the line */ + while (!feof(fp) && !ferror(fp)) Regards, -- Fujii Masao NTT DATA CORPORATION Advanced Platform Technology Group Research and Development Headquarters -
Re: Minor issues in .pgpass
Hamid Akhtar <hamid.akhtar@gmail.com> — 2020-03-04T12:45:38Z
On Wed, Mar 4, 2020 at 4:54 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > > > On 2020/03/04 20:39, Hamid Akhtar wrote: > > > > > > On Tue, Mar 3, 2020 at 8:57 PM Fujii Masao <masao.fujii@oss.nttdata.com > <mailto:masao.fujii@oss.nttdata.com>> wrote: > > > > > > > > On 2020/03/03 21:38, Hamid Akhtar wrote: > > > > > > > > > On Mon, Mar 2, 2020 at 6:07 PM Fujii Masao < > masao.fujii@oss.nttdata.com <mailto:masao.fujii@oss.nttdata.com> <mailto: > masao.fujii@oss.nttdata.com <mailto: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). > > > > 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. > > > > > > I'm not sure if I understand your comment here. From the code of > pg_strip_crlf > > I see that it is handling both carriage return and/or new line at the > end of a > > string: > > So if "buf" contains a carriage return at the end, it's removed and > the "len" that pg_strip_crlf() returns obviously should be smaller > than sizeof(buf). This causes the following condition that you > proposed as follows to always be false (i.e., len < sizeof(buf) - 1) > even when there are still rest of line. So we cannot eat rest of > the line even though it exists. I'm missing something? > No, you are perfectly fine. I now understand where you are coming from. So, all good now. > > + if (len >= sizeof(buf) - 1) > + { > + char tmp[LINELEN]; > + > + /* > + * Warn if this password setting line is too long, > + * because it's unexpectedly truncated. > + */ > + if (buf[0] != '#') > + fprintf(stderr, > + libpq_gettext("WARNING: > line %d too long in password file \"%s\"\n"), > + line_number, pgpassfile); > + > + /* eat rest of the line */ > + while (!feof(fp) && !ferror(fp)) > > 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 -
Re: Minor issues in .pgpass
Hamid Akhtar <hamid.akhtar@gmail.com> — 2020-03-04T14:01:16Z
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation: tested, passed Tested and looks fine to me. The new status of this patch is: Ready for Committer
-
Re: Minor issues in .pgpass
Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-03-05T04:07:28Z
On 2020/03/04 23:01, Hamid Akhtar wrote: > The following review has been posted through the commitfest application: > make installcheck-world: tested, passed > Implements feature: tested, passed > Spec compliant: tested, passed > Documentation: tested, passed > > Tested and looks fine to me. > > The new status of this patch is: Ready for Committer Many thanks for testing and reviewing the patch! I pushed it. Regards, -- Fujii Masao NTT DATA CORPORATION Advanced Platform Technology Group Research and Development Headquarters