Re: [BUGS] BUG #13854: SSPI authentication failure: wrong realm name used

On Thu, Mar 24, 2016 at 11:07 AM, Robbie Harwood <rharwood@redhat.com> wrote:
> Christian Ullrich <chris@chrullrich.net> writes:
>
>> Updated patch attached.
>
> I unfortunately don't have windows machines to test this on, but I
> thought it might be helpful to review this anyway since I'm touching
> code in the same general area (GSSAPI).  And as far as I can tell, you
> don't break anything there; master continues to behave as expected.
>
> Some comments inline:
>
>>   pg_SSPI_recvauth(Port *port)
>>   {
>>       int                     mtype;
>> +     int                     status;
>
> The section of this function for include_realm checking already uses an
> int status return code (retval).  I would expect to see them share a
> variable rather than have both "retval" and "status".
>
>> +             status = pg_SSPI_make_upn(accountname, sizeof(accountname),
>> +                                                               domainname, sizeof(domainname),
>> +                                                               port->hba->upn_username);
>
> This is the only place I see that this function is called.  That being
> the case, why bother with the sizes of parameters?  Why doesn't
> pg_SSPI_make_upn() just call sizeof() by itself rather than taking those
> as arguments?

Well, suppose there is another caller to that function in the future
which wants to use arguments of different lengths.  This actually
seems pretty sensible to me - concern about the length of the buffer
is isolated in the caller, without any spooky action at a distance.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company