Fix unsafe references to errno within error messaging logic.

Various places were supposing that errno could be expected to hold still
within an ereport() nest or similar contexts.  This isn't true necessarily,
though in some cases it accidentally failed to fail depending on how the
compiler chanced to order the subexpressions.  This class of thinko
explains recent reports of odd failures on clang-built versions, typically
missing or inappropriate HINT fields in messages.

Problem identified by Christian Kruse, who also submitted the patch this
commit is based on.  (I fixed a few issues in his patch and found a couple
of additional places with the same disease.)

Back-patch as appropriate to all supported branches.

diff --git a/src/backend/commands/tablespace.c b/src/backend/commands/tablespace.c
index d73e5e826dce25421d3d7191159947521890535d..60fd939175f550effcf66f9c5e01d970e9517959 100644 (file)
--- a/src/backend/commands/tablespace.c
+++ b/src/backend/commands/tablespace.c
@@ -782,10 +782,14 @@ remove_symlink:
        else
        {
                if (unlink(linkloc) < 0)
-                       ereport(redo ? LOG : (errno == ENOENT ? WARNING : ERROR),
+               {
+                       int                     saved_errno = errno;
+
+                       ereport(redo ? LOG : (saved_errno == ENOENT ? WARNING : ERROR),
                                        (errcode_for_file_access(),
                                         errmsg("could not remove symbolic link \"%s\": %m",
                                                        linkloc)));
+               }
        }
 
        pfree(linkloc_with_version_dir);

diff --git a/src/backend/port/sysv_sema.c b/src/backend/port/sysv_sema.c
index b4825d20fbe8adface4a6d0bacbd7c45ec6f746a..d5d66edcd3bf02731fecac74770044d9d4dbde4d 100644 (file)
--- a/src/backend/port/sysv_sema.c
+++ b/src/backend/port/sysv_sema.c
@@ -91,15 +91,17 @@ InternalIpcSemaphoreCreate(IpcSemaphoreKey semKey, int numSems)
 
        if (semId < 0)
        {
+               int                     saved_errno = errno;
+
                /*
                 * Fail quietly if error indicates a collision with existing set. One
                 * would expect EEXIST, given that we said IPC_EXCL, but perhaps we
                 * could get a permission violation instead?  Also, EIDRM might occur
                 * if an old set is slated for destruction but not gone yet.
                 */
-               if (errno == EEXIST || errno == EACCES
+               if (saved_errno == EEXIST || saved_errno == EACCES
 #ifdef EIDRM
-                       || errno == EIDRM
+                       || saved_errno == EIDRM
 #endif
                        )
                        return -1;
@@ -112,7 +114,7 @@ InternalIpcSemaphoreCreate(IpcSemaphoreKey semKey, int numSems)
                                 errdetail("Failed system call was semget(%lu, %d, 0%o).",
                                                   (unsigned long) semKey, numSems,
                                                   IPC_CREAT | IPC_EXCL | IPCProtection),
-                                (errno == ENOSPC) ?
+                                (saved_errno == ENOSPC) ?
                                 errhint("This error does *not* mean that you have run out of disk space.  "
                  "It occurs when either the system limit for the maximum number of "
                         "semaphore sets (SEMMNI), or the system wide maximum number of "
@@ -136,13 +138,17 @@ IpcSemaphoreInitialize(IpcSemaphoreId semId, int semNum, int value)
 
        semun.val = value;
        if (semctl(semId, semNum, SETVAL, semun) < 0)
+       {
+               int                     saved_errno = errno;
+
                ereport(FATAL,
                                (errmsg_internal("semctl(%d, %d, SETVAL, %d) failed: %m",
                                                                 semId, semNum, value),
-                                (errno == ERANGE) ?
+                                (saved_errno == ERANGE) ?
                                 errhint("You possibly need to raise your kernel's SEMVMX value to be at least "
                                  "%d.  Look into the PostgreSQL documentation for details.",
                                                 value) : 0));
+       }
 }
 
 /*

diff --git a/src/backend/port/sysv_shmem.c b/src/backend/port/sysv_shmem.c
index ac3a9fe3b0b59faaf455a1714939d94ab8ccc1d0..65ad59570cb76ee46a33eeecd31d21e9e5841986 100644 (file)
--- a/src/backend/port/sysv_shmem.c
+++ b/src/backend/port/sysv_shmem.c
@@ -73,15 +73,17 @@ InternalIpcMemoryCreate(IpcMemoryKey memKey, Size size)
 
        if (shmid < 0)
        {
+               int                     shmget_errno = errno;
+
                /*
                 * Fail quietly if error indicates a collision with existing segment.
                 * One would expect EEXIST, given that we said IPC_EXCL, but perhaps
                 * we could get a permission violation instead?  Also, EIDRM might
                 * occur if an old seg is slated for destruction but not gone yet.
                 */
-               if (errno == EEXIST || errno == EACCES
+               if (shmget_errno == EEXIST || shmget_errno == EACCES
 #ifdef EIDRM
-                       || errno == EIDRM
+                       || shmget_errno == EIDRM
 #endif
                        )
                        return NULL;
@@ -95,10 +97,8 @@ InternalIpcMemoryCreate(IpcMemoryKey memKey, Size size)
                 * against SHMMIN in the preexisting-segment case, so we will not get
                 * EINVAL a second time if there is such a segment.
                 */
-               if (errno == EINVAL)
+               if (shmget_errno == EINVAL)
                {
-                       int                     save_errno = errno;
-
                        shmid = shmget(memKey, 0, IPC_CREAT | IPC_EXCL | IPCProtection);
 
                        if (shmid < 0)
@@ -124,8 +124,6 @@ InternalIpcMemoryCreate(IpcMemoryKey memKey, Size size)
                                        elog(LOG, "shmctl(%d, %d, 0) failed: %m",
                                                 (int) shmid, IPC_RMID);
                        }
-
-                       errno = save_errno;
                }
 
                /*
@@ -137,25 +135,26 @@ InternalIpcMemoryCreate(IpcMemoryKey memKey, Size size)
                 * it should be.  SHMMNI violation is ENOSPC, per spec.  Just plain
                 * not-enough-RAM is ENOMEM.
                 */
+               errno = shmget_errno;
                ereport(FATAL,
                                (errmsg("could not create shared memory segment: %m"),
                  errdetail("Failed system call was shmget(key=%lu, size=%zu, 0%o).",
                                        (unsigned long) memKey, size,
                                        IPC_CREAT | IPC_EXCL | IPCProtection),
-                                (errno == EINVAL) ?
+                                (shmget_errno == EINVAL) ?
                                 errhint("This error usually means that PostgreSQL's request for a shared memory "
                 "segment exceeded your kernel's SHMMAX parameter, or possibly that "
                                                 "it is less than "
                                                 "your kernel's SHMMIN parameter.\n"
                "The PostgreSQL documentation contains more information about shared "
                                                 "memory configuration.") : 0,
-                                (errno == ENOMEM) ?
+                                (shmget_errno == ENOMEM) ?
                                 errhint("This error usually means that PostgreSQL's request for a shared "
                                                 "memory segment exceeded your kernel's SHMALL parameter.  You might need "
                                                 "to reconfigure the kernel with larger SHMALL.\n"
                "The PostgreSQL documentation contains more information about shared "
                                                 "memory configuration.") : 0,
-                                (errno == ENOSPC) ?
+                                (shmget_errno == ENOSPC) ?
                                 errhint("This error does *not* mean that you have run out of disk space.  "
                                                 "It occurs either if all available shared memory IDs have been taken, "
                                                 "in which case you need to raise the SHMMNI parameter in your kernel, "
@@ -331,6 +330,7 @@ CreateAnonymousSegment(Size *size)
 {
        Size            allocsize = *size;
        void       *ptr = MAP_FAILED;
+       int                     mmap_errno = 0;
 
 #ifndef MAP_HUGETLB
        if (huge_tlb_pages == HUGE_TLB_ON)
@@ -363,6 +363,7 @@ CreateAnonymousSegment(Size *size)
 
                ptr = mmap(NULL, allocsize, PROT_READ | PROT_WRITE,
                                   PG_MMAP_FLAGS | MAP_HUGETLB, -1, 0);
+               mmap_errno = errno;
                if (huge_tlb_pages == HUGE_TLB_TRY && ptr == MAP_FAILED)
                        elog(DEBUG1, "mmap with MAP_HUGETLB failed, huge pages disabled: %m");
        }
@@ -376,13 +377,17 @@ CreateAnonymousSegment(Size *size)
                 * back to non-huge pages.
                 */
                allocsize = *size;
-               ptr = mmap(NULL, allocsize, PROT_READ | PROT_WRITE, PG_MMAP_FLAGS, -1, 0);
+               ptr = mmap(NULL, allocsize, PROT_READ | PROT_WRITE,
+                                  PG_MMAP_FLAGS, -1, 0);
+               mmap_errno = errno;
        }
 
        if (ptr == MAP_FAILED)
+       {
+               errno = mmap_errno;
                ereport(FATAL,
                                (errmsg("could not map anonymous shared memory: %m"),
-                                (errno == ENOMEM) ?
+                                (mmap_errno == ENOMEM) ?
                                 errhint("This error usually means that PostgreSQL's request "
                                        "for a shared memory segment exceeded available memory, "
                                          "swap space or huge pages. To reduce the request size "
@@ -390,6 +395,7 @@ CreateAnonymousSegment(Size *size)
                                           "memory usage, perhaps by reducing shared_buffers or "
                                                 "max_connections.",
                                                 *size) : 0));
+       }
 
        *size = allocsize;
        return ptr;

diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index b26e28006ec4f04e541a562526912d98c532b638..764534a3ae08395984beba500111b9c8726396aa 100644 (file)
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -264,14 +264,15 @@ exec_command(const char *cmd,
                {
 #ifndef WIN32
                        struct passwd *pw;
+                       uid_t           user_id = geteuid();
 
-                       errno = 0;      /* clear errno before call */
-                       pw = getpwuid(geteuid());
+                       errno = 0;                      /* clear errno before call */
+                       pw = getpwuid(user_id);
                        if (!pw)
                        {
-                               psql_error("could not get home directory for user id %d: %s\n",
-                                                  (int) geteuid(), errno ?
-                                                  strerror(errno) : "user does not exist");
+                               psql_error("could not get home directory for user id %ld: %s\n",
+                                                  (long) user_id,
+                                                errno ? strerror(errno) : _("user does not exist"));
                                exit(EXIT_FAILURE);
                        }
                        dir = pw->pw_dir;

diff --git a/src/common/username.c b/src/common/username.c
index c6812ddd9d17c3219f4a854fdb32b88395da8e48..e946972a561d2737baebb71f0ddb4dad6affaaae 100644 (file)
--- a/src/common/username.c
+++ b/src/common/username.c
@@ -34,17 +34,17 @@ get_user_name(char **errstr)
 {
 #ifndef WIN32
        struct passwd *pw;
-       uid_t user_id = geteuid();
+       uid_t           user_id = geteuid();
 
        *errstr = NULL;
 
-       errno = 0;      /* clear errno before call */
+       errno = 0;                                      /* clear errno before call */
        pw = getpwuid(user_id);
        if (!pw)
        {
-               *errstr = psprintf(_("failed to look up effective user id %d: %s"),
-                               (int) user_id, errno ? strerror(errno) :
-                               _("user does not exist"));
+               *errstr = psprintf(_("failed to look up effective user id %ld: %s"),
+                                                  (long) user_id,
+                                                errno ? strerror(errno) : _("user does not exist"));
                return NULL;
        }