[education/rkward] rkward: Avoid overflow of request id; includes some cleanups
Thomas Friedrichsmeier
null at kde.org
Sun Aug 31 13:51:37 BST 2025
Git commit 08206ae281e4add896d5e890d7ff3edcab5f59db by Thomas Friedrichsmeier.
Committed on 30/08/2025 at 21:03.
Pushed by tfry into branch 'master'.
Avoid overflow of request id; includes some cleanups
M +14 -6 rkward/autotests/core_test.cpp
M +2 -2 rkward/rbackend/rcommand.cpp
M +4 -6 rkward/rbackend/rkbackendtransmitter.cpp
M +6 -2 rkward/rbackend/rkrbackend.cpp
M +4 -1 rkward/rbackend/rkrbackendprotocol_shared.cpp
M +22 -37 rkward/rbackend/rktransmitter.cpp
https://invent.kde.org/education/rkward/-/commit/08206ae281e4add896d5e890d7ff3edcab5f59db
diff --git a/rkward/autotests/core_test.cpp b/rkward/autotests/core_test.cpp
index 7d42e20da..8dc04d38d 100644
--- a/rkward/autotests/core_test.cpp
+++ b/rkward/autotests/core_test.cpp
@@ -96,9 +96,15 @@ class RKWardCoreTest : public QObject {
RInterface::issueCommand(command, chain);
}
- void waitForAllFinished(int timeoutms = 2000) {
+ // returns false, if timeout was hit
+ bool waitForAllFinished(int timeoutms = 2000) {
+ bool timedout = true;
runCommandWithTimeout(
- new RCommand(QStringLiteral("# waitForAllFinished"), RCommand::App | RCommand::EmptyCommand | RCommand::Sync), nullptr, [](RCommand *) {}, timeoutms);
+ new RCommand(QStringLiteral("# waitForAllFinished"), RCommand::App | RCommand::EmptyCommand | RCommand::Sync), nullptr, [&timedout](RCommand *) {
+ timedout = false;
+ }, timeoutms);
+ // NOTE: failure message was already generated
+ return !timedout;
}
void cleanGlobalenv() {
@@ -414,11 +420,11 @@ class RKWardCoreTest : public QObject {
void priorityCommandTest() {
// NOTE: Hopefully, this test case is fixed for good, but when it is not (it wasn't quite in 08/2025), 10 iterations are not near enough to trigger the
- // failure, reliably. On my test system, I rather needed on the order to 10000 iterations for that. Of course that makes testing a pain, and cannot
+ // failure, somewhat reliably. On my test system, I rather needed on the order to 10000 iterations for that. Of course that makes testing a pain, and cannot
// reasonably be done on the CI...
for (int i = 0; i < 10; ++i) {
bool priority_command_done = false;
- runCommandAsync(new RCommand(QStringLiteral("%1cat(\"sleeping\\n\");%1Sys.sleep(5)").arg(QString().fill(u'\n', i % 5)), RCommand::User), nullptr, [&priority_command_done](RCommand *command) {
+ runCommandAsync(new RCommand(QStringLiteral("%1cat(\"sleeping\\n\");%1Sys.sleep(5);cat(\"BUG\")").arg(QString().fill(u'\n', i % 5)), RCommand::User), nullptr, [&priority_command_done](RCommand *command) {
QVERIFY(priority_command_done);
QVERIFY(command->failed());
QVERIFY(command->wasCanceled());
@@ -431,8 +437,10 @@ class RKWardCoreTest : public QObject {
// NOTE: The above two commands may or may not start executing in that order. Conceivably, the priority command gets handled, before the initial sleep
// command even started. This interesting corner case, in particular, has been causing trouble in the past, so we try to tigger it, deliberately.
// The inserted newline(s) in the fist command make(s) that a tiny bit more likely to happen (because parsing needs more iterations).
- waitForAllFinished(); // first wait with a short timeout: sleep should have been cancelled
- waitForAllFinished(6000); // fallbacck: priority_command_done must remain in scope until done (even if interrupting fails for some reason)
+ if (!waitForAllFinished()) { // first, wait with a short timeout: sleep should have been cancelled
+ waitForAllFinished(6000); // but if that fails, keep priority_command_done in scope to avoid crash)
+ QVERIFY(false); // still a bug, of course
+ }
}
}
diff --git a/rkward/rbackend/rcommand.cpp b/rkward/rbackend/rcommand.cpp
index 734ad7f8e..e6a0c437d 100644
--- a/rkward/rbackend/rcommand.cpp
+++ b/rkward/rbackend/rcommand.cpp
@@ -30,8 +30,8 @@ int RCommand::next_id = 0;
RCommand::RCommand(const QString &command, int type, const QString &rk_equiv) : RData(), RCommandChain(false) {
RK_TRACE(RBACKEND);
_id = next_id++;
- // if we ever submit enough commands to get a buffer overflow, use only positive numbers.
- if (next_id < 0) {
+ // not likely to trigger in real use, but make sure not to exceed transmittable (32 bit signed) range
+ if (next_id > (1 << 30)) {
next_id = 0;
}
_type = type;
diff --git a/rkward/rbackend/rkbackendtransmitter.cpp b/rkward/rbackend/rkbackendtransmitter.cpp
index ce0bd2081..b1ed68d3c 100644
--- a/rkward/rbackend/rkbackendtransmitter.cpp
+++ b/rkward/rbackend/rkbackendtransmitter.cpp
@@ -111,11 +111,6 @@ void RKRBackendTransmitter::requestReceived(RBackendRequest *request) {
} else if (request->type == RBackendRequest::PriorityCommand) {
RKRBackend::this_pointer->setPriorityCommand(request->takeCommand());
} else { // requests which originated in the backend below this line
- if (current_sync_requests.isEmpty()) {
- RK_ASSERT(false);
- return;
- }
-
// "Synchronous" requests are not necessarily answered in the order they have been queued
int id = request->id;
RBackendRequest *current_sync_request = nullptr;
@@ -126,7 +121,10 @@ void RKRBackendTransmitter::requestReceived(RBackendRequest *request) {
break;
}
}
- RK_ASSERT(current_sync_request);
+ if (!current_sync_request) {
+ RK_DEBUG(RBACKEND, DL_ERROR, "Got reply for unknown sync request id %d, type %d", id, request->type);
+ return;
+ }
if (current_sync_request->type == RBackendRequest::Output) {
delete current_sync_request; // this was just our internal request
} else {
diff --git a/rkward/rbackend/rkrbackend.cpp b/rkward/rbackend/rkrbackend.cpp
index 07bdfd948..1c3e7045c 100644
--- a/rkward/rbackend/rkrbackend.cpp
+++ b/rkward/rbackend/rkrbackend.cpp
@@ -98,22 +98,26 @@ void RKRBackend::interruptCommand(int command_id) {
RK_DEBUG(RBACKEND, DL_DEBUG, "Received interrupt request for command id %d", command_id);
QMutexLocker lock(&all_current_commands_mutex);
- if (all_current_commands.isEmpty()) return;
- if ((command_id == -1) || (all_current_commands.last()->id == command_id)) {
+ if ((command_id == -1) || (!all_current_commands.isEmpty() && (all_current_commands.last()->id == command_id))) {
if (!too_late_to_interrupt) {
RK_DEBUG(RBACKEND, DL_DEBUG, "scheduling interrupt for command id %d", command_id);
scheduleInterrupt();
}
} else {
+ bool any_found = false;
// if the command to cancel is *not* the topmost command, then do not interrupt, yet.
for (RCommandProxy *candidate : std::as_const(all_current_commands)) {
if (candidate->id == command_id) {
if (!current_commands_to_cancel.contains(candidate)) {
RK_DEBUG(RBACKEND, DL_DEBUG, "scheduling delayed interrupt for command id %d", command_id);
current_commands_to_cancel.append(candidate);
+ any_found = true;
}
}
}
+ if (any_found) {
+ RK_DEBUG(RBACKEND, DL_ERROR, "interrupt scheduled for command id %d, but it is not current", command_id);
+ }
}
}
diff --git a/rkward/rbackend/rkrbackendprotocol_shared.cpp b/rkward/rbackend/rkrbackendprotocol_shared.cpp
index 009370682..9a41fad19 100644
--- a/rkward/rbackend/rkrbackendprotocol_shared.cpp
+++ b/rkward/rbackend/rkrbackendprotocol_shared.cpp
@@ -28,7 +28,10 @@ RBackendRequest::RBackendRequest(bool synchronous, RCallbackType type) {
RBackendRequest::synchronous = synchronous;
RBackendRequest::type = type;
- id = ++_id;
+ if (++_id >= (1 << 30)) { // Not likely to happen, but never overflow the 32 bit int used for transmission
+ _id = 1;
+ }
+ id = _id;
done = false;
command = nullptr;
output = nullptr;
diff --git a/rkward/rbackend/rktransmitter.cpp b/rkward/rbackend/rktransmitter.cpp
index 43c0ae81a..23c2d77d0 100644
--- a/rkward/rbackend/rktransmitter.cpp
+++ b/rkward/rbackend/rktransmitter.cpp
@@ -12,7 +12,7 @@ SPDX-License-Identifier: GPL-2.0-or-later
void RKRBackendSerializer::serialize(const RBackendRequest &request, QDataStream &stream) {
RK_TRACE(RBACKEND);
- stream << (qint16)request.id;
+ stream << (qint32)request.id;
stream << (qint8)request.type;
stream << request.synchronous;
stream << request.done; // well, not really needed, but...
@@ -38,29 +38,26 @@ void RKRBackendSerializer::serialize(const RBackendRequest &request, QDataStream
}
}
+template<typename T> static T readS(QDataStream &stream) {
+ T ret;
+ stream >> ret;
+ return ret;
+}
+
RBackendRequest *RKRBackendSerializer::unserialize(QDataStream &stream) {
RK_TRACE(RBACKEND);
RBackendRequest *request = new RBackendRequest(false, RBackendRequest::OtherRequest); // will be overwritten
RBackendRequest::_id--;
- bool dummyb;
- qint8 dummy8;
- qint16 dummy16;
- stream >> dummy16;
- request->id = dummy16;
- stream >> dummy8;
- request->type = (RBackendRequest::RCallbackType)dummy8;
+ request->id = readS<qint32>(stream);
+ request->type = (RBackendRequest::RCallbackType)readS<qint8>(stream);
stream >> request->synchronous;
- stream >> dummyb;
- request->done = dummyb;
- stream >> dummyb;
- if (dummyb) request->command = unserializeProxy(stream);
- stream >> dummyb;
- if (dummyb) request->output = unserializeOutput(stream);
+ request->done = readS<bool>(stream);
+ if (readS<bool>(stream)) request->command = unserializeProxy(stream);
+ if (readS<bool>(stream)) request->output = unserializeOutput(stream);
stream >> request->params;
- stream >> dummyb;
- if (dummyb) request->subcommandrequest = unserialize(stream);
+ if (readS<bool>(stream)) request->subcommandrequest = unserialize(stream);
return request;
}
@@ -79,15 +76,12 @@ ROutputList *RKRBackendSerializer::unserializeOutput(QDataStream &stream) {
RK_TRACE(RBACKEND);
ROutputList *ret = new ROutputList();
- qint32 len;
- stream >> len;
+ auto len = readS<qint32>(stream);
ret->reserve(len);
for (qint32 i = 0; i < len; ++i) {
ROutput out;
- qint8 dummy8;
- stream >> dummy8;
- out.type = (ROutput::ROutputType)dummy8;
+ out.type = (ROutput::ROutputType)readS<qint8>(stream);
stream >> out.output;
ret->append(out);
}
@@ -120,9 +114,7 @@ RData *RKRBackendSerializer::unserializeData(QDataStream &stream) {
RData *ret = new RData;
RData::RDataType type;
- qint8 dummy8;
- stream >> dummy8;
- type = (RData::RDataType)dummy8;
+ type = (RData::RDataType)readS<qint8>(stream);
if (type == RData::IntVector) {
RData::IntStorage data;
stream >> data;
@@ -137,8 +129,7 @@ RData *RKRBackendSerializer::unserializeData(QDataStream &stream) {
ret->setData(data);
} else if (type == RData::StructureVector) {
RData::RDataStorage data;
- qint32 len;
- stream >> len;
+ auto len = readS<qint32>(stream);
data.reserve(len);
for (qint32 i = 0; i < len; ++i) {
data.append(unserializeData(stream));
@@ -167,18 +158,12 @@ void RKRBackendSerializer::serializeProxy(const RCommandProxy &proxy, QDataStrea
RCommandProxy *RKRBackendSerializer::unserializeProxy(QDataStream &stream) {
RK_TRACE(RBACKEND);
- QString command;
- stream >> command;
- qint32 type;
- stream >> type;
+ auto command = readS<QString>(stream);
+ auto type = readS<qint32>(stream);
RCommandProxy *ret = new RCommandProxy(command, type);
- qint32 dummy32;
- stream >> dummy32;
- ret->id = dummy32;
- stream >> dummy32;
- ret->status = dummy32;
- stream >> dummy32;
- ret->has_been_run_up_to = dummy32;
+ ret->id = readS<qint32>(stream);
+ ret->status = readS<qint32>(stream);
+ ret->has_been_run_up_to = readS<qint32>(stream);
stream >> (ret->updates_object);
RData *data = unserializeData(stream);
More information about the rkward-tracker
mailing list