Re: [HACKERS] small patch to crypt.c
* Joshua D. Drake (j...@commandprompt.com) wrote: Well I was more referring to the default is: check if null, if true return ok check if valuntil today, if true return error else return ok To me we don't need the null check. However, when I tested it, without the null check you can't login. So now I am curious about what is going on. Erm, but what is valuntil set to when it's null? I'd expect it to be zero because it hasn't been changed since: TimestampTz vuntil = 0; Using your pseudo-code, you end up with: check if 0 today, if true return error else return ok Which is why you end up always getting an error when you get rid of the explicit isnull check. Looking at it too quickly, I had assumed that the test was inverted and that your patch worked most of the time but didn't account for GetCurrentTimestamp() going negative. Regardless, setting vuntil to some magic value that really means it's actually NULL, which is what you'd need to do in order to get rid of that explicit check for null, doesn't strike me as a good idea. When a value is null, we shouldn't be looking at the data at all. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] small patch to crypt.c
Stephen Frost sfr...@snowman.net writes: Regardless, setting vuntil to some magic value that really means it's actually NULL, which is what you'd need to do in order to get rid of that explicit check for null, doesn't strike me as a good idea. When a value is null, we shouldn't be looking at the data at all. Even aside from that, the proposed change seems like a bad idea because it introduces an unnecessary call of GetCurrentTimestamp() in the common case where there's no valuntil limit. On some platforms that call is pretty slow. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] small patch to crypt.c
On 06/09/2013 09:28 AM, Tom Lane wrote: Even aside from that, the proposed change seems like a bad idea because it introduces an unnecessary call of GetCurrentTimestamp() in the common case where there's no valuntil limit. On some platforms that call is pretty slow. And that would explain why we don't do something like this: index f01d904..4935c9f 100644 --- a/src/backend/libpq/crypt.c +++ b/src/backend/libpq/crypt.c @@ -145,12 +145,10 @@ md5_crypt_verify(const Port *port, const char *role, char *client_pass) /* * Password OK, now check to be sure we are not past rolvaliduntil */ - if (isnull) + if (isnull || vuntil GetCurrentTimestamp()) retval = STATUS_OK; - else if (vuntil GetCurrentTimestamp()) - retval = STATUS_ERROR; else - retval = STATUS_OK; + retval = STATUS_ERROR; } Right. Ty for the feedback, I know it was just a little bit of code but it just looked off and I appreciate the explanation. JD -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] small patch to crypt.c
Hello, In my quest to understand how all the logging etc works with authentication I came across the area of crypt.c that checks for valid_until but it seems like it has an extraneous check. If I am wrong I apologize for the noise but wouldn't mind an explanation. index f01d904..8d809b2 100644 --- a/src/backend/libpq/crypt.c +++ b/src/backend/libpq/crypt.c @@ -145,9 +145,7 @@ md5_crypt_verify(const Port *port, const char *role, char *client_pass) /* * Password OK, now check to be sure we are not past rolvaliduntil */ - if (isnull) - retval = STATUS_OK; - else if (vuntil GetCurrentTimestamp()) + if (vuntil GetCurrentTimestamp()) retval = STATUS_ERROR; else retval = STATUS_OK; -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] small patch to crypt.c
JD, * Joshua D. Drake (j...@commandprompt.com) wrote: In my quest to understand how all the logging etc works with authentication I came across the area of crypt.c that checks for valid_until but it seems like it has an extraneous check. If I am wrong I apologize for the noise but wouldn't mind an explanation. Alright, there probably aren't too many people out there running with their clock set to pre-2000, but wouldn't this end up giving the wrong result in those cases, as GetCurrentTimestamp() would end up returning a negative value, which would make it less than vuntil's default of zero? Perhaps we could change what vuntil is set to by default, but I think it's probably better to keep things as-is; we should really be checking for null cases explicitly in general. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] small patch to crypt.c
On 06/08/2013 08:47 PM, Stephen Frost wrote: JD, * Joshua D. Drake (j...@commandprompt.com) wrote: In my quest to understand how all the logging etc works with authentication I came across the area of crypt.c that checks for valid_until but it seems like it has an extraneous check. If I am wrong I apologize for the noise but wouldn't mind an explanation. Alright, there probably aren't too many people out there running with their clock set to pre-2000, but wouldn't this end up giving the wrong result in those cases, as GetCurrentTimestamp() would end up returning a negative value, which would make it less than vuntil's default of zero? Perhaps we could change what vuntil is set to by default, but I think it's probably better to keep things as-is; we should really be checking for null cases explicitly in general. Well I was more referring to the default is: check if null, if true return ok check if valuntil today, if true return error else return ok To me we don't need the null check. However, when I tested it, without the null check you can't login. So now I am curious about what is going on. JD Thanks, Stephen -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
