Re: libpq 9.4 requires /etc/passwd?

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> On Fri, Jan 9, 2015 at 06:57:02PM -0500, Tom Lane wrote:
>> Hmm ... actually, I'll bet it's not $HOME that's at issue, but the name
>> of the user. Commit a4c8f14364c27508233f8a31ac4b10a4c90235a9 turned
>> failure of pg_fe_getauthname() into a hard connection failure, whereas
>> previously it was harmless as long as the caller provided a username.
>>
>> I wonder if we shouldn't just revert that commit in toto. Yeah,
>> identifying an out-of-memory error might be useful, but this cure
>> seems a lot worse than the disease. What's more, this coding reports
>> *any* pg_fe_getauthname failure as "out of memory", which is much worse
>> than useless.
>>
>> Alternatively, maybe don't try to resolve username this way until
>> we've found that the caller isn't providing any username.

> I have developed the attached patch which documents why the user name
> lookup might fail, and sets the failure string to "". It preserves the
> memory allocation failure behavior.

I'm unimpressed with that patch. It does nothing to fix the fundamental
problem, which is that error handling in this area is many bricks shy of
a load. And to the extent that it addresses Christoph's complaint, it
does so only in a roundabout and utterly undocumented way.

I think we need to suck it up and fix pg_fe_getauthname to have proper
error reporting, which in turns means fixing pqGetpwuid (since the API
for that is incapable of distinguishing "no such user" from "error during
lookup"). Accordingly, I propose the attached patch instead. Some
notes:

* According to Microsoft's documentation, Windows' GetUserName() does not
set errno (it would be mildly astonishing if it did...). The existing
error-handling code in src/common/username.c is therefore wrong too.

* port.h seems to think it should declare pqGetpwuid() if __CYGWIN__,
but noplace else agrees with that.

* I made pg_fe_getauthname's Windows username[] array 256 bytes for
consistency with username.c, where some actual research seems to have
been expended on how large it ought to be.

* pqGetpwuid thinks there was once a four-argument spec for getpwuid_r.
That must have been in the late bronze age, because even in 1992's Single
Unix Spec it's five arguments. And the SUS is our standard benchmark
for Unix portability. So I think we could rip that out, along with the
corresponding configure test, at least in HEAD. I've not done so here
though.

In principle, changing the API specs for pg_fe_getauthname and pqGetpwuid
should be safe because we don't officially export those functions. In
practice, on platforms that don't honor the export restriction list it's
theoretically possible that somebody is calling those from third-party
code. So what I propose we do with this is patch HEAD and 9.4 only.
We need to do *something* in 9.4 to address Christoph's complaint, and
that branch is new enough that we can probably get away with changing
officially-unsupported APIs. The lack of other field complaints makes
me okay with not trying to fix these issues further back.

Comments?

regards, tom lane