Skip to content

Commit 1de422a

Browse files
thejcfactorthejcfactor
committed
PYCBC-1814: Return nullptr instead of Py_None on pending exceptions
Changes -------- * Replace Py_RETURN_NONE with return nullptr at 29 sites in transactions.cxx where an exception was already set on that path; returning Py_None with a pending exception makes CPython discard it and raise SystemError instead (Py_FatalError under Py_DEBUG) * Release self and its C++ config via Py_DECREF before returning nullptr in transaction_config__new__ and transaction_options__new__, which previously leaked both on a PyArg_ParseTupleAndKeywords failure Change-Id: Ib6322c4b4a3143c09250be8c8c8fd24a5ce2f65b Reviewed-on: https://review.couchbase.org/c/couchbase-python-client/+/250604 Tested-by: Build Bot <build@couchbase.com> Reviewed-by: Dimitris Christodoulou <dimitris.christodoulou@couchbase.com>
1 parent 541ad43 commit 1de422a

1 file changed

Lines changed: 31 additions & 29 deletions

File tree

‎src/transactions/transactions.cxx‎

Lines changed: 31 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,8 @@ pycbc::txns::transaction_config__new__(PyTypeObject* type, PyObject* args, PyObj
134134
&metadata_collection,
135135
&scan_consistency)) {
136136
PyErr_SetString(PyExc_ValueError, "couldn't parse args");
137-
Py_RETURN_NONE;
137+
Py_DECREF(self);
138+
return nullptr;
138139
}
139140
if (nullptr != durability_level) {
140141
self->cfg->durability_level(
@@ -280,7 +281,8 @@ pycbc::txns::transaction_options__new__(PyTypeObject* type, PyObject* args, PyOb
280281
&metadata_scope,
281282
&metadata_collection)) {
282283
PyErr_SetString(PyExc_ValueError, "couldn't parse args");
283-
Py_RETURN_NONE;
284+
Py_DECREF(self);
285+
return nullptr;
284286
}
285287
if (nullptr != durability_level) {
286288
self->opts->durability_level(
@@ -354,7 +356,7 @@ pycbc::txns::transaction_query_options__new__(PyTypeObject* type, PyObject* args
354356
if (!PyArg_ParseTupleAndKeywords(
355357
args, kwargs, kw_format, const_cast<char**>(kw_list), &pyObj_query_args)) {
356358
PyErr_SetString(PyExc_ValueError, "couldn't parse args");
357-
Py_RETURN_NONE;
359+
return nullptr;
358360
}
359361
auto self = reinterpret_cast<pycbc::txns::transaction_query_options*>(type->tp_alloc(type, 0));
360362
auto opts =
@@ -420,7 +422,7 @@ pycbc::txns::transaction_get_result__get__(pycbc::txns::transaction_get_result*
420422
PyObject* default_value = nullptr;
421423
if (!PyArg_ParseTuple(args, "s|O", &field_name, &default_value)) {
422424
PyErr_SetString(PyExc_ValueError, "couldn't parse args");
423-
Py_RETURN_NONE;
425+
return nullptr;
424426
}
425427
if (ID == field_name) {
426428
return PyUnicode_FromString(result->res->id().key().c_str());
@@ -435,15 +437,15 @@ pycbc::txns::transaction_get_result__get__(pycbc::txns::transaction_get_result*
435437
pyObj_value = pycbc::cbpp_to_py(result->res->content().data);
436438
} catch (const std::exception& e) {
437439
PyErr_SetString(PyExc_TypeError, e.what());
438-
Py_RETURN_NONE;
440+
return nullptr;
439441
}
440442
PyObject* pyObj_result = PyTuple_Pack(2, pyObj_value, pyObj_flags);
441443
Py_DECREF(pyObj_value);
442444
Py_DECREF(pyObj_flags);
443445
return pyObj_result;
444446
}
445447
PyErr_SetString(PyExc_ValueError, fmt::format("unknown field_name {}", field_name).c_str());
446-
Py_RETURN_NONE;
448+
return nullptr;
447449
}
448450

449451
static PyMethodDef transaction_get_result_methods[] = {
@@ -697,17 +699,17 @@ pycbc::txns::destroy_transactions([[maybe_unused]] PyObject* self, PyObject* arg
697699
if (!PyArg_ParseTupleAndKeywords(
698700
args, kwargs, kw_format, const_cast<char**>(kw_list), &PyCapsule_Type, &pyObj_txns)) {
699701
PyErr_SetString(PyExc_ValueError, "couldn't parse args");
700-
Py_RETURN_NONE;
702+
return nullptr;
701703
}
702704
if (nullptr == pyObj_txns) {
703705
PyErr_SetString(PyExc_ValueError, "expected a transactions object");
704-
Py_RETURN_NONE;
706+
return nullptr;
705707
}
706708
auto txns =
707709
reinterpret_cast<pycbc::txns::transactions*>(PyCapsule_GetPointer(pyObj_txns, "txns_"));
708710
if (nullptr == txns) {
709711
PyErr_SetString(PyExc_ValueError, "passed null transactions");
710-
Py_RETURN_NONE;
712+
return nullptr;
711713
}
712714
Py_BEGIN_ALLOW_THREADS txns->txns->close();
713715
Py_END_ALLOW_THREADS Py_RETURN_NONE;
@@ -1201,25 +1203,25 @@ pycbc::txns::transaction_query_op([[maybe_unused]] PyObject* self, PyObject* arg
12011203
&pyObj_callback,
12021204
&pyObj_errback)) {
12031205
PyErr_SetString(PyExc_ValueError, "couldn't parse args");
1204-
Py_RETURN_NONE;
1206+
return nullptr;
12051207
}
12061208
if (nullptr == pyObj_ctx) {
12071209
PyErr_SetString(PyExc_ValueError, "expected transaction_context");
1208-
Py_RETURN_NONE;
1210+
return nullptr;
12091211
}
12101212
auto ctx =
12111213
reinterpret_cast<pycbc::txns::transaction_context*>(PyCapsule_GetPointer(pyObj_ctx, "ctx_"));
12121214
if (nullptr == ctx) {
12131215
PyErr_SetString(PyExc_ValueError, "passed null transaction_context");
1214-
Py_RETURN_NONE;
1216+
return nullptr;
12151217
}
12161218
if (nullptr == statement) {
12171219
PyErr_SetString(PyExc_ValueError, "expected query statement");
1218-
Py_RETURN_NONE;
1220+
return nullptr;
12191221
}
12201222
if (nullptr == pyObj_options) {
12211223
PyErr_SetString(PyExc_ValueError, "expected options");
1222-
Py_RETURN_NONE;
1224+
return nullptr;
12231225
}
12241226
auto opt = reinterpret_cast<pycbc::txns::transaction_query_options*>(pyObj_options);
12251227
if ((nullptr == pyObj_callback) != (nullptr == pyObj_errback)) {
@@ -1282,7 +1284,7 @@ pycbc::txns::transaction_op([[maybe_unused]] PyObject* self, PyObject* args, PyO
12821284
&pyObj_txn_get_result);
12831285
if (!ret) {
12841286
PyErr_SetString(PyExc_ValueError, "couldn't parse args");
1285-
Py_RETURN_NONE;
1287+
return nullptr;
12861288
}
12871289
if (nullptr != pyObj_value) {
12881290
PyObject* pyObj_data = PyTuple_GET_ITEM(pyObj_value, 0);
@@ -1296,13 +1298,13 @@ pycbc::txns::transaction_op([[maybe_unused]] PyObject* self, PyObject* args, PyO
12961298
}
12971299
if (nullptr == pyObj_ctx) {
12981300
PyErr_SetString(PyExc_ValueError, "no transaction_context passed in");
1299-
Py_RETURN_NONE;
1301+
return nullptr;
13001302
}
13011303
auto ctx =
13021304
reinterpret_cast<pycbc::txns::transaction_context*>(PyCapsule_GetPointer(pyObj_ctx, "ctx_"));
13031305
if (nullptr == ctx) {
13041306
PyErr_SetString(PyExc_ValueError, "passed null transaction_context");
1305-
Py_RETURN_NONE;
1307+
return nullptr;
13061308
}
13071309

13081310
if ((nullptr == pyObj_callback) != (nullptr == pyObj_errback)) {
@@ -1322,7 +1324,7 @@ pycbc::txns::transaction_op([[maybe_unused]] PyObject* self, PyObject* args, PyO
13221324
PyErr_SetString(PyExc_ValueError, "couldn't create document id for get");
13231325
Py_XDECREF(pyObj_callback);
13241326
Py_XDECREF(pyObj_errback);
1325-
Py_RETURN_NONE;
1327+
return nullptr;
13261328
}
13271329
couchbase::core::document_id id{ bucket, scope, collection, key };
13281330
Py_BEGIN_ALLOW_THREADS ctx->ctx->get_optional(
@@ -1339,7 +1341,7 @@ pycbc::txns::transaction_op([[maybe_unused]] PyObject* self, PyObject* args, PyO
13391341
"couldn't create document id for get_replica_from_preferred_server_group");
13401342
Py_XDECREF(pyObj_callback);
13411343
Py_XDECREF(pyObj_errback);
1342-
Py_RETURN_NONE;
1344+
return nullptr;
13431345
}
13441346
couchbase::core::document_id id{ bucket, scope, collection, key };
13451347
Py_BEGIN_ALLOW_THREADS ctx->ctx->get_replica_from_preferred_server_group(
@@ -1356,15 +1358,15 @@ pycbc::txns::transaction_op([[maybe_unused]] PyObject* self, PyObject* args, PyO
13561358
PyErr_SetString(PyExc_ValueError, "couldn't create document id for insert");
13571359
Py_XDECREF(pyObj_callback);
13581360
Py_XDECREF(pyObj_errback);
1359-
Py_RETURN_NONE;
1361+
return nullptr;
13601362
}
13611363
couchbase::core::document_id id{ bucket, scope, collection, key };
13621364
if (nullptr == pyObj_value) {
13631365
PyErr_SetString(PyExc_ValueError,
13641366
fmt::format("no value given for an insert of key {}", id.key()).c_str());
13651367
Py_XDECREF(pyObj_callback);
13661368
Py_XDECREF(pyObj_errback);
1367-
Py_RETURN_NONE;
1369+
return nullptr;
13681370
}
13691371
Py_BEGIN_ALLOW_THREADS ctx->ctx->insert(
13701372
id,
@@ -1380,14 +1382,14 @@ pycbc::txns::transaction_op([[maybe_unused]] PyObject* self, PyObject* args, PyO
13801382
PyErr_SetString(PyExc_ValueError, "replace expects a value");
13811383
Py_XDECREF(pyObj_callback);
13821384
Py_XDECREF(pyObj_errback);
1383-
Py_RETURN_NONE;
1385+
return nullptr;
13841386
}
13851387
if (nullptr == pyObj_txn_get_result ||
13861388
0 == PyObject_TypeCheck(pyObj_txn_get_result, &transaction_get_result_type)) {
13871389
PyErr_SetString(PyExc_ValueError, "replace expects to be passed a transaction_get_result");
13881390
Py_XDECREF(pyObj_callback);
13891391
Py_XDECREF(pyObj_errback);
1390-
Py_RETURN_NONE;
1392+
return nullptr;
13911393
}
13921394
auto tx_get_result =
13931395
reinterpret_cast<pycbc::txns::transaction_get_result*>(pyObj_txn_get_result);
@@ -1406,7 +1408,7 @@ pycbc::txns::transaction_op([[maybe_unused]] PyObject* self, PyObject* args, PyO
14061408
PyErr_SetString(PyExc_ValueError, "remove expects to be passed a transaction_get_result");
14071409
Py_XDECREF(pyObj_callback);
14081410
Py_XDECREF(pyObj_errback);
1409-
Py_RETURN_NONE;
1411+
return nullptr;
14101412
}
14111413
auto tx_get_result =
14121414
reinterpret_cast<pycbc::txns::transaction_get_result*>(pyObj_txn_get_result);
@@ -1424,7 +1426,7 @@ pycbc::txns::transaction_op([[maybe_unused]] PyObject* self, PyObject* args, PyO
14241426
PyErr_SetString(PyExc_ValueError, "unknown txn operation");
14251427
Py_XDECREF(pyObj_callback);
14261428
Py_XDECREF(pyObj_errback);
1427-
Py_RETURN_NONE;
1429+
return nullptr;
14281430
}
14291431
if (nullptr == pyObj_callback && nullptr == pyObj_errback) {
14301432
PyObject* ret = nullptr;
@@ -1495,18 +1497,18 @@ pycbc::txns::transaction_get_multi_op([[maybe_unused]] PyObject* self,
14951497
&pyObj_errback);
14961498
if (!ret) {
14971499
PyErr_SetString(PyExc_ValueError, "couldn't parse args");
1498-
Py_RETURN_NONE;
1500+
return nullptr;
14991501
}
15001502

15011503
if (nullptr == pyObj_ctx) {
15021504
PyErr_SetString(PyExc_ValueError, "no transaction_context passed in");
1503-
Py_RETURN_NONE;
1505+
return nullptr;
15041506
}
15051507
auto ctx =
15061508
reinterpret_cast<pycbc::txns::transaction_context*>(PyCapsule_GetPointer(pyObj_ctx, "ctx_"));
15071509
if (nullptr == ctx) {
15081510
PyErr_SetString(PyExc_ValueError, "passed null transaction_context");
1509-
Py_RETURN_NONE;
1511+
return nullptr;
15101512
}
15111513

15121514
if (!PyTuple_Check(pyObj_specs) && !PyList_Check(pyObj_specs)) {
@@ -1597,7 +1599,7 @@ pycbc::txns::transaction_get_multi_op([[maybe_unused]] PyObject* self,
15971599
PyErr_SetString(PyExc_ValueError, "Unknown transaction operation");
15981600
Py_XDECREF(pyObj_callback);
15991601
Py_XDECREF(pyObj_errback);
1600-
Py_RETURN_NONE;
1602+
return nullptr;
16011603
}
16021604

16031605
if (nullptr == pyObj_callback && nullptr == pyObj_errback) {

0 commit comments

Comments
 (0)