Thread: Unportable(?) use of setenv() in secure_open_gssapi()

I noticed while investigating [1] that we have one single solitary
use of setenv(3) in our code base, in secure_open_gssapi().

It's been project policy since 2001 to avoid setenv(), and I notice
that src/port/win32env.c lacks support for setenv(), making it
pretty doubtful that the call has the semantics one would wish
on Windows.

Now, versions of the POSIX spec released in this century do have setenv(),
and even seem to regard it as "more standard" than putenv().  So maybe
there's a case for moving our goalposts and deciding to allow use of
setenv().  But then it seems like we'd better twiddle win32env.c to
support it; and I'm not sure back-patching such a change would be wise.

Alternatively, we could change secure_open_gssapi() to use putenv(),
at the cost of a couple more lines of code.

Thoughts?

            regards, tom lane

[1]
https://www.postgresql.org/message-id/SN2PR05MB264066382E2CC75E734492C8E3510%40SN2PR05MB2640.namprd05.prod.outlook.com

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> I noticed while investigating [1] that we have one single solitary
> use of setenv(3) in our code base, in secure_open_gssapi().
>
> It's been project policy since 2001 to avoid setenv(), and I notice
> that src/port/win32env.c lacks support for setenv(), making it
> pretty doubtful that the call has the semantics one would wish
> on Windows.

Yeah, that doesn't seem good, though you'd have to be building with MIT
Kerberos for Windows to end up with GSSAPI on a Windows build in the
first place (much more common on Windows is to build with Microsoft SSPI
support instead).  Still, it looks like someone went to the trouble of
setting that up on a buildfarm animal- looks like hamerkop has it.

> Now, versions of the POSIX spec released in this century do have setenv(),
> and even seem to regard it as "more standard" than putenv().  So maybe
> there's a case for moving our goalposts and deciding to allow use of
> setenv().  But then it seems like we'd better twiddle win32env.c to
> support it; and I'm not sure back-patching such a change would be wise.
>
> Alternatively, we could change secure_open_gssapi() to use putenv(),
> at the cost of a couple more lines of code.
>
> Thoughts?

So, auth.c already does the song-and-dance for putenv for this exact
variable, but it happens too late if you want to use GSSAPI for an
encrypted connection.  Looking at this now, it seems like we should
really just move up where that's happening instead of having it done
once in be-secure-gssapi.c and then again in auth.c.  Maybe we could do
it in BackendInitialize..?

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> It's been project policy since 2001 to avoid setenv(), and I notice
>> that src/port/win32env.c lacks support for setenv(), making it
>> pretty doubtful that the call has the semantics one would wish
>> on Windows.

> Yeah, that doesn't seem good, though you'd have to be building with MIT
> Kerberos for Windows to end up with GSSAPI on a Windows build in the
> first place (much more common on Windows is to build with Microsoft SSPI
> support instead).  Still, it looks like someone went to the trouble of
> setting that up on a buildfarm animal- looks like hamerkop has it.

It looks like it'd only matter if Kerberos were using a different CRT
version than PG proper, which is probably even less likely.  Still,
that could happen.

> So, auth.c already does the song-and-dance for putenv for this exact
> variable, but it happens too late if you want to use GSSAPI for an
> encrypted connection.  Looking at this now, it seems like we should
> really just move up where that's happening instead of having it done
> once in be-secure-gssapi.c and then again in auth.c.  Maybe we could do
> it in BackendInitialize..?

Hm, yeah, and it's also pretty darn inconsistent that one of them does
overwrite = 1 while the other emulates overwrite = 0.  I'd be for
unifying that code.  It'd also lead to a more safely back-patchable
fix than the other solution.

Going forward, adding support for setenv() wouldn't be an unreasonable
thing to do, I think.  It's certainly something that people find
attractive to use, and the portability issues we had with it back at
the turn of the century should be pretty much gone.  I do note that my
old dinosaur gaur, which is the last surviving buildfarm member without
unsetenv(), lacks setenv() as well --- but I'd be willing to add support
for that as a src/port module.  We'd also have to fix win32env.c, but
that's not much new code either.

            regards, tom lane