Thread: BUG #8139: initdb: Misleading error message when current user not in /etc/passwd

The following bug has been logged on the website:

Bug reference:      8139
Logged by:          Nicolas Marchildon
Email address:      nicolas@marchildon.net
PostgreSQL version: 9.2.4
Operating system:   RHEL 6
Description:        =


Running initdb while logged in as a user that has no entry in /etc/passwd,
which happens when authenticating with Kerberos, and missing sssd-client
prints a misleading error message:

"initdb: could not obtain information about current user: Success"

The misleading part is the "Success". This comes from errno:

        pw =3D getpwuid(geteuid());
        if (!pw)
        {
                fprintf(stderr,
                          _("%s: could not obtain information about current
user: %s\n"),
                                progname, strerror(errno));
                exit(1);
        }

The man page says:

RETURN VALUE
       The  getpwnam()  and  getpwuid() functions return a pointer to a
passwd
       structure, or NULL if the matching entry  is  not  found  or  an =

error
       occurs.   If an error occurs, errno is set appropriately.  If one
wants
       to check errno after the call, it should be  set  to  zero  before =

the
       call.

First, initdb's get_id function does not set errno to zero, which is a bug.
Second, when the return value is NULL, it should only print strerror(errno)
when errno !=3D 0.

nicolas@marchildon.net writes:
> "initdb: could not obtain information about current user: Success"

> The misleading part is the "Success". This comes from errno:

>         pw = getpwuid(geteuid());
>         if (!pw)
>         {
>                 fprintf(stderr,
>                           _("%s: could not obtain information about current
> user: %s\n"),
>                                 progname, strerror(errno));
>                 exit(1);
>         }

> The man page says:

> RETURN VALUE
>        The  getpwnam()  and  getpwuid() functions return a pointer to a
> passwd
>        structure, or NULL if the matching entry  is  not  found  or  an
> error
>        occurs.   If an error occurs, errno is set appropriately.  If one
> wants
>        to check errno after the call, it should be  set  to  zero  before
> the
>        call.

AFAICS, getpwuid is not honoring its specification here: it failed to
set errno.  I don't see that suppressing the strerror result would add
anything much.

            regards, tom lane

Tom Lane wrote:
> nicolas@marchildon.net writes:

> > The man page says:
>=20
> > RETURN VALUE
> >        The  getpwnam()  and  getpwuid() functions return a pointer to=
 a
> > passwd
> >        structure, or NULL if the matching entry  is  not  found  or  =
an=20
> > error
> >        occurs.   If an error occurs, errno is set appropriately.  If =
one
> > wants
> >        to check errno after the call, it should be  set  to  zero  be=
fore=20
> > the
> >        call.
>=20
> AFAICS, getpwuid is not honoring its specification here: it failed to
> set errno.  I don't see that suppressing the strerror result would add
> anything much.

Well, in this case no error occured, but no matching entry was found.
The wording in the manpage is explicit that there not being an entry is
not an error.

--=20
=C1lvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, May  7, 2013 at 02:19:05PM -0400, Alvaro Herrera wrote:
> Tom Lane wrote:
> > nicolas@marchildon.net writes:
>
> > > The man page says:
> >
> > > RETURN VALUE
> > >        The  getpwnam()  and  getpwuid() functions return a pointer to a
> > > passwd
> > >        structure, or NULL if the matching entry  is  not  found  or  an
> > > error
> > >        occurs.   If an error occurs, errno is set appropriately.  If one
> > > wants
> > >        to check errno after the call, it should be  set  to  zero  before
> > > the
> > >        call.
> >
> > AFAICS, getpwuid is not honoring its specification here: it failed to
> > set errno.  I don't see that suppressing the strerror result would add
> > anything much.
>
> Well, in this case no error occured, but no matching entry was found.
> The wording in the manpage is explicit that there not being an entry is
> not an error.

I have developed the attached patch to fix this and another instance I
saw.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +

Bruce Momjian <bruce@momjian.us> writes:
> I have developed the attached patch to fix this and another instance I
> saw.

I think you need to re-read the part of the man page you quoted,
and set errno to zero beforehand, if you're going to rely on it
being zero after a non-error return.

            regards, tom lane

Bruce Momjian wrote:
> On Tue, May  7, 2013 at 02:19:05PM -0400, Alvaro Herrera wrote:

> I have developed the attached patch to fix this and another instance I
> saw.

Remember to set errno = 0 before calling the getpw* function; at least
in initdb it would be meaningful.  And in the pg_upgrade case, it seems
better to abort the upgrade if this call doesn't work.  (Also, there are
other uses of getpwuid/getpwnam elsewhere.  Not sure we want to worry
too much about them.)

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Dec  4, 2013 at 04:03:39PM -0300, Alvaro Herrera wrote:
> Bruce Momjian wrote:
> > On Tue, May  7, 2013 at 02:19:05PM -0400, Alvaro Herrera wrote:
>
> > I have developed the attached patch to fix this and another instance I
> > saw.
>
> Remember to set errno = 0 before calling the getpw* function; at least
> in initdb it would be meaningful.  And in the pg_upgrade case, it seems
> better to abort the upgrade if this call doesn't work.  (Also, there are
> other uses of getpwuid/getpwnam elsewhere.  Not sure we want to worry
> too much about them.)

Wow, that is an odd API.  Once I started to look at all the errno
clearing necessary, I realized that the function in bin/scripts/common.c
could be moved to src/port and generalized to reduce code duplication.

Updated patch attached, with centralized checking for errno and more
consistent behavior for username lookups.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +

Bruce Momjian <bruce@momjian.us> writes:
> Updated patch attached, with centralized checking for errno and more
> consistent behavior for username lookups.

This bit is not good:

+     errno = 0;    /* clear errno before call */
+     pw = getpwuid(geteuid());
+     if (!pw)
+     {
+         *errstr = psprintf(_("effective user id %d lookup failure: %s"),
+                 (int) geteuid(), errno ? strerror(errno) :
+                 _("user does not exist"));

The second call of geteuid() could clobber errno before strerror can see it.
You should compute geteuid() just once and keep it in a variable.

Also, I don't think that error message meets our style guidelines.
Maybe "failed to look up user id %d: ..." ?

            regards, tom lane

Bruce Momjian wrote:

>           return STATUS_ERROR;
>       }
>
> !     user_name = get_user_name(&errstr);
> !     if (!user_name)
>       {
> !         ereport(LOG, (errmsg("%s\n", errstr)));
> !         pfree(errstr);
>           return STATUS_ERROR;
>       }

The message is already translated by get_user_name, so I think this
should use errmsg_internal() instead of errmsg().  Also, why do you add
a newline?

Not clear whether the new file should be in src/port or src/common.

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Mon, Dec  9, 2013 at 04:35:56PM -0500, Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > Updated patch attached, with centralized checking for errno and more
> > consistent behavior for username lookups.
>
> This bit is not good:
>
> +     errno = 0;    /* clear errno before call */
> +     pw = getpwuid(geteuid());
> +     if (!pw)
> +     {
> +         *errstr = psprintf(_("effective user id %d lookup failure: %s"),
> +                 (int) geteuid(), errno ? strerror(errno) :
> +                 _("user does not exist"));
>
> The second call of geteuid() could clobber errno before strerror can see it.
> You should compute geteuid() just once and keep it in a variable.

Good point --- fixed.

> Also, I don't think that error message meets our style guidelines.
> Maybe "failed to look up user id %d: ..." ?

OK, updated.

Updated patch attached to Alvaro's email reply.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +

On Mon, Dec  9, 2013 at 06:45:39PM -0300, Alvaro Herrera wrote:
> Bruce Momjian wrote:
>
> >           return STATUS_ERROR;
> >       }
> >
> > !     user_name = get_user_name(&errstr);
> > !     if (!user_name)
> >       {
> > !         ereport(LOG, (errmsg("%s\n", errstr)));
> > !         pfree(errstr);
> >           return STATUS_ERROR;
> >       }
>
> The message is already translated by get_user_name, so I think this
> should use errmsg_internal() instead of errmsg().  Also, why do you add
> a newline?

OK, done.

> Not clear whether the new file should be in src/port or src/common.

Agreed.  It isn't designed to add missing OS functionality, but it is
mostly OS-specific code.

Updated patch attached, with Tom's requested changes.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +

On Mon, Dec  9, 2013 at 07:47:34PM -0500, Bruce Momjian wrote:
> On Mon, Dec  9, 2013 at 06:45:39PM -0300, Alvaro Herrera wrote:
> > Bruce Momjian wrote:
> >
> > >           return STATUS_ERROR;
> > >       }
> > >
> > > !     user_name = get_user_name(&errstr);
> > > !     if (!user_name)
> > >       {
> > > !         ereport(LOG, (errmsg("%s\n", errstr)));
> > > !         pfree(errstr);
> > >           return STATUS_ERROR;
> > >       }
> >
> > The message is already translated by get_user_name, so I think this
> > should use errmsg_internal() instead of errmsg().  Also, why do you add
> > a newline?
>
> OK, done.
>
> > Not clear whether the new file should be in src/port or src/common.
>
> Agreed.  It isn't designed to add missing OS functionality, but it is
> mostly OS-specific code.
>
> Updated patch attached, with Tom's requested changes.

Patch applied.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +

On 12/9/13, 7:47 PM, Bruce Momjian wrote:
>> Not clear whether the new file should be in src/port or src/common.
> Agreed.  It isn't designed to add missing OS functionality, but it is
> mostly OS-specific code.

It's not for portability, though, is it?

On Fri, Dec 20, 2013 at 10:46:43AM -0500, Peter Eisentraut wrote:
> On 12/9/13, 7:47 PM, Bruce Momjian wrote:
> >> Not clear whether the new file should be in src/port or src/common.
> > Agreed.  It isn't designed to add missing OS functionality, but it is
> > mostly OS-specific code.
>
> It's not for portability, though, is it?

Well, neither is sprompt.c, but that has a lot of port-specific code in
it, so I used that as a guide.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +

Bruce Momjian wrote:
> On Fri, Dec 20, 2013 at 10:46:43AM -0500, Peter Eisentraut wrote:
> > On 12/9/13, 7:47 PM, Bruce Momjian wrote:
> > >> Not clear whether the new file should be in src/port or src/common.
> > > Agreed.  It isn't designed to add missing OS functionality, but it is
> > > mostly OS-specific code.
> >
> > It's not for portability, though, is it?
>
> Well, neither is sprompt.c, but that has a lot of port-specific code in
> it, so I used that as a guide.

src/common was created much later than sprompt.c was written.  I would
have thought that the consideration would have been that sprompt.c
should eventually be moved to src/common; not that it would serve as a
precedent for anything.

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Jan  7, 2014 at 06:40:14PM -0300, Alvaro Herrera wrote:
> Bruce Momjian wrote:
> > On Fri, Dec 20, 2013 at 10:46:43AM -0500, Peter Eisentraut wrote:
> > > On 12/9/13, 7:47 PM, Bruce Momjian wrote:
> > > >> Not clear whether the new file should be in src/port or src/common.
> > > > Agreed.  It isn't designed to add missing OS functionality, but it is
> > > > mostly OS-specific code.
> > >
> > > It's not for portability, though, is it?
> >
> > Well, neither is sprompt.c, but that has a lot of port-specific code in
> > it, so I used that as a guide.
>
> src/common was created much later than sprompt.c was written.  I would
> have thought that the consideration would have been that sprompt.c
> should eventually be moved to src/common; not that it would serve as a
> precedent for anything.

Are we not moving items over to common where appropriate?  Are we
worried about bring external applications?

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +

Bruce Momjian wrote:
> On Tue, Jan  7, 2014 at 06:40:14PM -0300, Alvaro Herrera wrote:
> > Bruce Momjian wrote:
> > > On Fri, Dec 20, 2013 at 10:46:43AM -0500, Peter Eisentraut wrote:
> > > > On 12/9/13, 7:47 PM, Bruce Momjian wrote:
> > > > >> Not clear whether the new file should be in src/port or src/common.
> > > > > Agreed.  It isn't designed to add missing OS functionality, but it is
> > > > > mostly OS-specific code.
> > > >
> > > > It's not for portability, though, is it?
> > >
> > > Well, neither is sprompt.c, but that has a lot of port-specific code in
> > > it, so I used that as a guide.
> >
> > src/common was created much later than sprompt.c was written.  I would
> > have thought that the consideration would have been that sprompt.c
> > should eventually be moved to src/common; not that it would serve as a
> > precedent for anything.
>
> Are we not moving items over to common where appropriate?

I don't think we're moving code from src/port to src/common just for the
heck of it; but ISTM if we're adding new code which belongs to
src/common, and there's a natural file for it in src/port which should
arguably also be in src/common, then it makes sense to put both the new
code and the old file together in src/common.

Note that nothing in src/port should depend on stuff in src/common.  As
I see it, src/port is very bare-bones stuff which src/common builds on
top of.  It might also make sense, in some cases, to consider low-level
routines in src/port that are used by higher level routines in
src/common.

>  Are we worried about bring external applications?

Please rephrase.  What are we worried about?

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Fri, Jan 10, 2014 at 05:23:43PM -0300, Alvaro Herrera wrote:
> > Are we not moving items over to common where appropriate?
>
> I don't think we're moving code from src/port to src/common just for the
> heck of it; but ISTM if we're adding new code which belongs to
> src/common, and there's a natural file for it in src/port which should
> arguably also be in src/common, then it makes sense to put both the new
> code and the old file together in src/common.
>
> Note that nothing in src/port should depend on stuff in src/common.  As
> I see it, src/port is very bare-bones stuff which src/common builds on
> top of.  It might also make sense, in some cases, to consider low-level
> routines in src/port that are used by higher level routines in
> src/common.

OK, C file moved from /port to /common.

> >  Are we worried about bring external applications?
>
> Please rephrase.  What are we worried about?

I was asking if we did not move old functions from port to common
because of concern about breaking 3rd party applications.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +

On Fri, 2014-01-10 at 18:04 -0500, Bruce Momjian wrote:
> OK, C file moved from /port to /common.

Please fix the new compiler warnings in pg_upgrade.

On Fri, Jan 10, 2014 at 08:49:18PM -0500, Peter Eisentraut wrote:
> On Fri, 2014-01-10 at 18:04 -0500, Bruce Momjian wrote:
> > OK, C file moved from /port to /common.
>
> Please fix the new compiler warnings in pg_upgrade.

Thanks, fixed.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +

Re: Bruce Momjian 2013-12-18 <20131218171628.GA1690@momjian.us>
> On Mon, Dec  9, 2013 at 07:47:34PM -0500, Bruce Momjian wrote:
> > On Mon, Dec  9, 2013 at 06:45:39PM -0300, Alvaro Herrera wrote:
> > > Bruce Momjian wrote:
> > >
> > > >           return STATUS_ERROR;
> > > >       }
> > > >
> > > > !     user_name = get_user_name(&errstr);
> > > > !     if (!user_name)
> > > >       {
> > > > !         ereport(LOG, (errmsg("%s\n", errstr)));
> > > > !         pfree(errstr);
> > > >           return STATUS_ERROR;
> > > >       }
> > >
> > > The message is already translated by get_user_name, so I think this
> > > should use errmsg_internal() instead of errmsg().  Also, why do you add
> > > a newline?
> >
> > OK, done.
> >
> > > Not clear whether the new file should be in src/port or src/common.
> >
> > Agreed.  It isn't designed to add missing OS functionality, but it is
> > mostly OS-specific code.
> >
> > Updated patch attached, with Tom's requested changes.
>
> Patch applied.

Hi,

the quoted code bit above in src/backend/libpq/auth.c is utterly
broken: for peer authentication, it uses get_user_name(), which yields
the *server* user name, not the client's. For that reason, peer
authentication in 9.4devel is broken - you can't log in with your user
name, but you can just say -U postgres (or what the initdb user was),
and it will let you in.

The attached patch reverts the src/backend/libpq/auth.c portion of
613c6d26bd42dd8c2dd0664315be9551475b8864 and fixes peer auth.

Christoph
--
cb@df7cb.de | http://www.df7cb.de/

Christoph Berg <cb@df7cb.de> writes:
> the quoted code bit above in src/backend/libpq/auth.c is utterly
> broken: for peer authentication, it uses get_user_name(), which yields
> the *server* user name, not the client's. For that reason, peer
> authentication in 9.4devel is broken - you can't log in with your user
> name, but you can just say -U postgres (or what the initdb user was),
> and it will let you in.

> The attached patch reverts the src/backend/libpq/auth.c portion of
> 613c6d26bd42dd8c2dd0664315be9551475b8864 and fixes peer auth.

Applied, thanks!

            regards, tom lane

On Fri, Mar 28, 2014 at 10:31:06AM -0400, Tom Lane wrote:
> Christoph Berg <cb@df7cb.de> writes:
> > the quoted code bit above in src/backend/libpq/auth.c is utterly
> > broken: for peer authentication, it uses get_user_name(), which yields
> > the *server* user name, not the client's. For that reason, peer
> > authentication in 9.4devel is broken - you can't log in with your user
> > name, but you can just say -U postgres (or what the initdb user was),
> > and it will let you in.
>
> > The attached patch reverts the src/backend/libpq/auth.c portion of
> > 613c6d26bd42dd8c2dd0664315be9551475b8864 and fixes peer auth.
>
> Applied, thanks!

I guess I should have said I was working on an updated patch.  I will
merge my changes in.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +

On Fri, Mar 28, 2014 at 10:33:48AM -0400, Bruce Momjian wrote:
> On Fri, Mar 28, 2014 at 10:31:06AM -0400, Tom Lane wrote:
> > Christoph Berg <cb@df7cb.de> writes:
> > > the quoted code bit above in src/backend/libpq/auth.c is utterly
> > > broken: for peer authentication, it uses get_user_name(), which yields
> > > the *server* user name, not the client's. For that reason, peer
> > > authentication in 9.4devel is broken - you can't log in with your user
> > > name, but you can just say -U postgres (or what the initdb user was),
> > > and it will let you in.
> >
> > > The attached patch reverts the src/backend/libpq/auth.c portion of
> > > 613c6d26bd42dd8c2dd0664315be9551475b8864 and fixes peer auth.
> >
> > Applied, thanks!
>
> I guess I should have said I was working on an updated patch.  I will
> merge my changes in.

I have applied the attached patch to make the code more closely match
what is done in src/common/username.c, particularly to display the errno
string on failure.

Christoph, thanks so much for finding this error now, rather than later.

I looked over the original patch that introduced this bug and
auth_peer() is the only place where we were passing in a user id, rather
than passing geteuid() directly to getpwuid, e.g.:

    getpwuid(geteuid())

I think we are good now.  My apologies for introducing this bug.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +