Produce compiler errors if errno is referenced inside elog/ereport calls.

It's often unsafe to reference errno within an elog/ereport call, because
there are a lot of sub-functions involved and they might not all preserve
errno.  (This is why we support the %m format spec: it works off a value
of errno captured before we execute any potentially-unsafe functions in
the arguments.)  Therefore, we have a project policy not to use errno
there.

This patch adds a hack to cause an (admittedly obscure) compiler error
for such unsafe usages.  With the current code, the error will only be seen
on Linux, macOS, and FreeBSD, but that should certainly be enough to catch
mistakes in the buildfarm if they somehow get missed earlier.

In addition, fix some places in src/common/exec.c that trip the error.
I think these places are actually all safe, but it's simple enough to
avoid the error by capturing errno manually, and doing so is good
future-proofing in case these call sites get any more complicated.

Thomas Munro (exec.c fixes by me)

Discussion: https://postgr.es/m/2975.1526862605@sss.pgh.pa.us

diff --git a/src/common/exec.c b/src/common/exec.c
index 4df16cd64bd770c73644f3eb6a0d254049741e91..c207c0289fd0f423111867419c328d4daecc3a62 100644 (file)
--- a/src/common/exec.c
+++ b/src/common/exec.c
@@ -124,8 +124,10 @@ find_my_exec(const char *argv0, char *retpath)
 
    if (!getcwd(cwd, MAXPGPATH))
    {
+       int         save_errno = errno;
+
        log_error(_("could not identify current directory: %s"),
-                 strerror(errno));
+                 strerror(save_errno));
        return -1;
    }
 
@@ -238,8 +240,10 @@ resolve_symlinks(char *path)
     */
    if (!getcwd(orig_wd, MAXPGPATH))
    {
+       int         save_errno = errno;
+
        log_error(_("could not identify current directory: %s"),
-                 strerror(errno));
+                 strerror(save_errno));
        return -1;
    }
 
@@ -254,7 +258,10 @@ resolve_symlinks(char *path)
            *lsep = '\0';
            if (chdir(path) == -1)
            {
-               log_error4(_("could not change directory to \"%s\": %s"), path, strerror(errno));
+               int         save_errno = errno;
+
+               log_error4(_("could not change directory to \"%s\": %s"),
+                          path, strerror(save_errno));
                return -1;
            }
            fname = lsep + 1;
@@ -281,8 +288,10 @@ resolve_symlinks(char *path)
 
    if (!getcwd(path, MAXPGPATH))
    {
+       int         save_errno = errno;
+
        log_error(_("could not identify current directory: %s"),
-                 strerror(errno));
+                 strerror(save_errno));
        return -1;
    }
    join_path_components(path, path, link_buf);
@@ -290,7 +299,10 @@ resolve_symlinks(char *path)
 
    if (chdir(orig_wd) == -1)
    {
-       log_error4(_("could not change directory to \"%s\": %s"), orig_wd, strerror(errno));
+       int         save_errno = errno;
+
+       log_error4(_("could not change directory to \"%s\": %s"),
+                  orig_wd, strerror(save_errno));
        return -1;
    }
 #endif                         /* HAVE_READLINK */
@@ -520,7 +532,10 @@ pclose_check(FILE *stream)
    if (exitstatus == -1)
    {
        /* pclose() itself failed, and hopefully set errno */
-       log_error(_("pclose failed: %s"), strerror(errno));
+       int         save_errno = errno;
+
+       log_error(_("pclose failed: %s"),
+                 strerror(save_errno));
    }
    else
    {

diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 4f4091d8cc8eefd525e80e7b657bc0a6f940db7f..6b618a52d0a6adcec31c31eb2159d84029fa3636 100644 (file)
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -70,6 +70,23 @@
 /* SQLSTATE codes for errors are defined in a separate file */
 #include "utils/errcodes.h"
 
+/*
+ * Provide a way to prevent "errno" from being accidentally used inside an
+ * elog() or ereport() invocation.  Since we know that some operating systems
+ * define errno as something involving a function call, we'll put a local
+ * variable of the same name as that function in the local scope to force a
+ * compile error.  On platforms that don't define errno in that way, nothing
+ * happens, so we get no warning ... but we can live with that as long as it
+ * happens on some popular platforms.
+ */
+#if defined(errno) && defined(__linux__)
+#define pg_prevent_errno_in_scope() int __errno_location pg_attribute_unused()
+#elif defined(errno) && (defined(__darwin__) || defined(__freebsd__))
+#define pg_prevent_errno_in_scope() int __error pg_attribute_unused()
+#else
+#define pg_prevent_errno_in_scope()
+#endif
+
 
 /*----------
  * New-style error reporting API: to be used in this way:
@@ -103,6 +120,7 @@
 #ifdef HAVE__BUILTIN_CONSTANT_P
 #define ereport_domain(elevel, domain, rest)   \
    do { \
+       pg_prevent_errno_in_scope(); \
        if (errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO, domain)) \
            errfinish rest; \
        if (__builtin_constant_p(elevel) && (elevel) >= ERROR) \
@@ -112,6 +130,7 @@
 #define ereport_domain(elevel, domain, rest)   \
    do { \
        const int elevel_ = (elevel); \
+       pg_prevent_errno_in_scope(); \
        if (errstart(elevel_, __FILE__, __LINE__, PG_FUNCNAME_MACRO, domain)) \
            errfinish rest; \
        if (elevel_ >= ERROR) \
@@ -198,6 +217,7 @@ extern int  getinternalerrposition(void);
 #ifdef HAVE__BUILTIN_CONSTANT_P
 #define elog(elevel, ...)  \
    do { \
+       pg_prevent_errno_in_scope(); \
        elog_start(__FILE__, __LINE__, PG_FUNCNAME_MACRO); \
        elog_finish(elevel, __VA_ARGS__); \
        if (__builtin_constant_p(elevel) && (elevel) >= ERROR) \
@@ -206,6 +226,7 @@ extern int  getinternalerrposition(void);
 #else                          /* !HAVE__BUILTIN_CONSTANT_P */
 #define elog(elevel, ...)  \
    do { \
+       pg_prevent_errno_in_scope(); \
        elog_start(__FILE__, __LINE__, PG_FUNCNAME_MACRO); \
        { \
            const int elevel_ = (elevel); \