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

On Thu, Mar 24, 2016 at 11:07 AM, Robbie Harwood <rharwood(at)redhat(dot)com> wrote:
> Christian Ullrich <chris(at)chrullrich(dot)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