Fix volatile vs. pointer confusion
Variables used after a longjmp() need to be declared volatile. In
case of a pointer, it's the pointer itself that needs to be declared
volatile, not the pointed-to value. So we need
PyObject *volatile items;
instead of
volatile PyObject *items; /* wrong */
Attached patch fixes a couple of cases of that. Most instances were
already correct.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
0001-Fix-volatile-vs.-pointer-confusion.patchtext/plain; charset=UTF-8; name=0001-Fix-volatile-vs.-pointer-confusion.patch; x-mac-creator=0; x-mac-type=0Download
From 1bb6acf63cf796d0659283c7ea5220385f7de181 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 11 Mar 2019 08:18:55 +0100
Subject: [PATCH] Fix volatile vs. pointer confusion
Variables used after a longjmp() need to be declared volatile. In
case of a pointer, it's the pointer itself that needs to be declared
volatile, not the pointed-to value. So we need
PyObject *volatile items;
instead of
volatile PyObject *items; /* wrong */
---
contrib/hstore_plpython/hstore_plpython.c | 9 ++++-----
contrib/jsonb_plpython/jsonb_plpython.c | 9 +++------
2 files changed, 7 insertions(+), 11 deletions(-)
diff --git a/contrib/hstore_plpython/hstore_plpython.c b/contrib/hstore_plpython/hstore_plpython.c
index 2f24090ff3..93c39d294d 100644
--- a/contrib/hstore_plpython/hstore_plpython.c
+++ b/contrib/hstore_plpython/hstore_plpython.c
@@ -128,7 +128,7 @@ Datum
plpython_to_hstore(PG_FUNCTION_ARGS)
{
PyObject *dict;
- volatile PyObject *items_v = NULL;
+ PyObject *volatile items = NULL;
int32 pcount;
HStore *out;
@@ -139,14 +139,13 @@ plpython_to_hstore(PG_FUNCTION_ARGS)
errmsg("not a Python mapping")));
pcount = PyMapping_Size(dict);
- items_v = PyMapping_Items(dict);
+ items = PyMapping_Items(dict);
PG_TRY();
{
int32 buflen;
int32 i;
Pairs *pairs;
- PyObject *items = (PyObject *) items_v;
pairs = palloc(pcount * sizeof(*pairs));
@@ -177,14 +176,14 @@ plpython_to_hstore(PG_FUNCTION_ARGS)
pairs[i].isnull = false;
}
}
- Py_DECREF(items_v);
+ Py_DECREF(items);
pcount = hstoreUniquePairs(pairs, pcount, &buflen);
out = hstorePairs(pairs, pcount, buflen);
}
PG_CATCH();
{
- Py_DECREF(items_v);
+ Py_DECREF(items);
PG_RE_THROW();
}
PG_END_TRY();
diff --git a/contrib/jsonb_plpython/jsonb_plpython.c b/contrib/jsonb_plpython/jsonb_plpython.c
index f44d364c97..1bc984d5c4 100644
--- a/contrib/jsonb_plpython/jsonb_plpython.c
+++ b/contrib/jsonb_plpython/jsonb_plpython.c
@@ -237,17 +237,14 @@ PLyMapping_ToJsonbValue(PyObject *obj, JsonbParseState **jsonb_state)
JsonbValue *out = NULL;
/* We need it volatile, since we use it after longjmp */
- volatile PyObject *items_v = NULL;
+ PyObject *volatile items = NULL;
pcount = PyMapping_Size(obj);
- items_v = PyMapping_Items(obj);
+ items = PyMapping_Items(obj);
PG_TRY();
{
Py_ssize_t i;
- PyObject *items;
-
- items = (PyObject *) items_v;
pushJsonbValue(jsonb_state, WJB_BEGIN_OBJECT, NULL);
@@ -279,7 +276,7 @@ PLyMapping_ToJsonbValue(PyObject *obj, JsonbParseState **jsonb_state)
}
PG_CATCH();
{
- Py_DECREF(items_v);
+ Py_DECREF(items);
PG_RE_THROW();
}
PG_END_TRY();
--
2.21.0
On Mon, Mar 11, 2019 at 08:23:39AM +0100, Peter Eisentraut wrote:
Attached patch fixes a couple of cases of that. Most instances were
already correct.
It seems to me that you should look at that:
/messages/by-id/20190308055911.GG4099@paquier.xyz
They treat about the same subject, and a patch has been sent for this
CF.
--
Michael
On 2019-Mar-11, Peter Eisentraut wrote:
Variables used after a longjmp() need to be declared volatile. In
case of a pointer, it's the pointer itself that needs to be declared
volatile, not the pointed-to value. So we needPyObject *volatile items;
instead of
volatile PyObject *items; /* wrong */
Looking at recently committed 2e616dee9e60, we have introduced this:
+ volatile xmlBufferPtr buf = NULL;
+ volatile xmlNodePtr cur_copy = NULL;
where the pointer-ness nature of the object is inside the typedef. I
*suppose* that this is correct as written. There are a few occurrences
of this pattern in eg. contrib/xml2.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019-03-11 09:31, Michael Paquier wrote:
On Mon, Mar 11, 2019 at 08:23:39AM +0100, Peter Eisentraut wrote:
Attached patch fixes a couple of cases of that. Most instances were
already correct.It seems to me that you should look at that:
/messages/by-id/20190308055911.GG4099@paquier.xyz
They treat about the same subject, and a patch has been sent for this
CF.
I'm aware of that patch and have been looking at it. But it's not
directly related to this issue.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019-03-11 12:57, Alvaro Herrera wrote:
Looking at recently committed 2e616dee9e60, we have introduced this:
+ volatile xmlBufferPtr buf = NULL; + volatile xmlNodePtr cur_copy = NULL;where the pointer-ness nature of the object is inside the typedef. I
*suppose* that this is correct as written. There are a few occurrences
of this pattern in eg. contrib/xml2.
I think this is correct, but I don't want to wreck my sanity trying to
understand the syntax-level details of why.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019-03-11 08:23, Peter Eisentraut wrote:
Variables used after a longjmp() need to be declared volatile. In
case of a pointer, it's the pointer itself that needs to be declared
volatile, not the pointed-to value. So we needPyObject *volatile items;
instead of
volatile PyObject *items; /* wrong */
Attached patch fixes a couple of cases of that. Most instances were
already correct.
Committed.
I'll wait for the build farm to see if there are any new compiler
warnings because of this, then backpatch.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services