Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit a2a8acd

Browse files
committed
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
1 parent 3a60c8f commit a2a8acd

File tree

2 files changed

+42
-6
lines changed

2 files changed

+42
-6
lines changed

src/common/exec.c

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -124,8 +124,10 @@ find_my_exec(const char *argv0, char *retpath)
124124

125125
if (!getcwd(cwd, MAXPGPATH))
126126
{
127+
int save_errno = errno;
128+
127129
log_error(_("could not identify current directory: %s"),
128-
strerror(errno));
130+
strerror(save_errno));
129131
return -1;
130132
}
131133

@@ -238,8 +240,10 @@ resolve_symlinks(char *path)
238240
*/
239241
if (!getcwd(orig_wd, MAXPGPATH))
240242
{
243+
int save_errno = errno;
244+
241245
log_error(_("could not identify current directory: %s"),
242-
strerror(errno));
246+
strerror(save_errno));
243247
return -1;
244248
}
245249

@@ -254,7 +258,10 @@ resolve_symlinks(char *path)
254258
*lsep = '\0';
255259
if (chdir(path) == -1)
256260
{
257-
log_error4(_("could not change directory to \"%s\": %s"), path, strerror(errno));
261+
int save_errno = errno;
262+
263+
log_error4(_("could not change directory to \"%s\": %s"),
264+
path, strerror(save_errno));
258265
return -1;
259266
}
260267
fname = lsep + 1;
@@ -281,16 +288,21 @@ resolve_symlinks(char *path)
281288

282289
if (!getcwd(path, MAXPGPATH))
283290
{
291+
int save_errno = errno;
292+
284293
log_error(_("could not identify current directory: %s"),
285-
strerror(errno));
294+
strerror(save_errno));
286295
return -1;
287296
}
288297
join_path_components(path, path, link_buf);
289298
canonicalize_path(path);
290299

291300
if (chdir(orig_wd) == -1)
292301
{
293-
log_error4(_("could not change directory to \"%s\": %s"), orig_wd, strerror(errno));
302+
int save_errno = errno;
303+
304+
log_error4(_("could not change directory to \"%s\": %s"),
305+
orig_wd, strerror(save_errno));
294306
return -1;
295307
}
296308
#endif /* HAVE_READLINK */
@@ -520,7 +532,10 @@ pclose_check(FILE *stream)
520532
if (exitstatus == -1)
521533
{
522534
/* pclose() itself failed, and hopefully set errno */
523-
log_error(_("pclose failed: %s"), strerror(errno));
535+
int save_errno = errno;
536+
537+
log_error(_("pclose failed: %s"),
538+
strerror(save_errno));
524539
}
525540
else
526541
{

src/include/utils/elog.h

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,23 @@
7070
/* SQLSTATE codes for errors are defined in a separate file */
7171
#include "utils/errcodes.h"
7272

73+
/*
74+
* Provide a way to prevent "errno" from being accidentally used inside an
75+
* elog() or ereport() invocation. Since we know that some operating systems
76+
* define errno as something involving a function call, we'll put a local
77+
* variable of the same name as that function in the local scope to force a
78+
* compile error. On platforms that don't define errno in that way, nothing
79+
* happens, so we get no warning ... but we can live with that as long as it
80+
* happens on some popular platforms.
81+
*/
82+
#if defined(errno) && defined(__linux__)
83+
#define pg_prevent_errno_in_scope() int __errno_location pg_attribute_unused()
84+
#elif defined(errno) && (defined(__darwin__) || defined(__freebsd__))
85+
#define pg_prevent_errno_in_scope() int __error pg_attribute_unused()
86+
#else
87+
#define pg_prevent_errno_in_scope()
88+
#endif
89+
7390

7491
/*----------
7592
* New-style error reporting API: to be used in this way:
@@ -103,6 +120,7 @@
103120
#ifdef HAVE__BUILTIN_CONSTANT_P
104121
#define ereport_domain(elevel, domain, rest) \
105122
do { \
123+
pg_prevent_errno_in_scope(); \
106124
if (errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO, domain)) \
107125
errfinish rest; \
108126
if (__builtin_constant_p(elevel) && (elevel) >= ERROR) \
@@ -112,6 +130,7 @@
112130
#define ereport_domain(elevel, domain, rest) \
113131
do { \
114132
const int elevel_ = (elevel); \
133+
pg_prevent_errno_in_scope(); \
115134
if (errstart(elevel_, __FILE__, __LINE__, PG_FUNCNAME_MACRO, domain)) \
116135
errfinish rest; \
117136
if (elevel_ >= ERROR) \
@@ -198,6 +217,7 @@ extern int getinternalerrposition(void);
198217
#ifdef HAVE__BUILTIN_CONSTANT_P
199218
#define elog(elevel, ...) \
200219
do { \
220+
pg_prevent_errno_in_scope(); \
201221
elog_start(__FILE__, __LINE__, PG_FUNCNAME_MACRO); \
202222
elog_finish(elevel, __VA_ARGS__); \
203223
if (__builtin_constant_p(elevel) && (elevel) >= ERROR) \
@@ -206,6 +226,7 @@ extern int getinternalerrposition(void);
206226
#else /* !HAVE__BUILTIN_CONSTANT_P */
207227
#define elog(elevel, ...) \
208228
do { \
229+
pg_prevent_errno_in_scope(); \
209230
elog_start(__FILE__, __LINE__, PG_FUNCNAME_MACRO); \
210231
{ \
211232
const int elevel_ = (elevel); \

0 commit comments

Comments
 (0)