service/polkit: address review comments

Author: Cu3PO42

src/services/polkit/agentimpl.cpp

@@ -2,14 +2,14 @@
 #include <algorithm>
 #include <utility>
 
-#include <glib-object.h>
 #include <qlist.h>
 #include <qloggingcategory.h>
 #include <qobject.h>
 #include <qtmetamacros.h>
 
 #include "../../core/generation.hpp"
 #include "../../core/logcat.hpp"
+#include "gobjectref.hpp"
 #include "listener.hpp"
 #include "qml.hpp"
 
@@ -22,10 +22,10 @@ PolkitAgentImpl* PolkitAgentImpl::instance = nullptr;
 
 PolkitAgentImpl::PolkitAgentImpl(PolkitAgent* agent)
     : QObject(nullptr)
-    , listener(qs_polkit_agent_new(this))
+    , listener(qs_polkit_agent_new(this), G_OBJECT_NO_REF)
     , qmlAgent(agent) {
 	auto path = this->qmlAgent->path().toUtf8();
-	qs_polkit_agent_register(this->listener, path.constData());
+	qs_polkit_agent_register(this->listener.get(), path.constData());
 }
 
 PolkitAgentImpl::~PolkitAgentImpl() {
@@ -41,8 +41,7 @@ PolkitAgentImpl::~PolkitAgentImpl() {
 		this->activeFlow->deleteLater();
 	}
 
-	if (this->isRegistered) qs_polkit_agent_unregister(this->listener);
-	g_object_unref(this->listener);
+	if (this->isRegistered) qs_polkit_agent_unregister(this->listener.get());
 }
 
 PolkitAgentImpl* PolkitAgentImpl::tryGetOrCreate(PolkitAgent* agent) {
@@ -58,7 +57,7 @@ PolkitAgentImpl* PolkitAgentImpl::tryGet(const PolkitAgent* agent) {
 }
 
 PolkitAgentImpl* PolkitAgentImpl::tryTakeover(PolkitAgent* agent) {
-	if (auto* impl = tryGet(agent); impl != nullptr) return impl;
+	if (auto* impl = tryGetOrCreate(agent); impl != nullptr) return impl;
 
 	auto* prevGen = EngineGeneration::findObjectGeneration(instance->qmlAgent);
 	auto* myGen = EngineGeneration::findObjectGeneration(agent);
@@ -128,7 +127,7 @@ void PolkitAgentImpl::activateAuthenticationRequest() {
 	                   << ", cookie: " << req->cookie;
 
 	QList<Identity*> identities;
-	for (auto* identity: req->identities) {
+	for (auto& identity: req->identities) {
 		auto* obj = Identity::fromPolkitIdentity(identity);
 		if (obj) identities.append(obj);
 	}

src/services/polkit/agentimpl.hpp

@@ -5,6 +5,7 @@
 #include <qobject.h>
 
 #include "flow.hpp"
+#include "gobjectref.hpp"
 #include "listener.hpp"
 
 namespace qs::service::polkit {
@@ -14,8 +15,11 @@ class PolkitAgentImpl
     : public QObject
     , public ListenerCb {
 	Q_OBJECT;
+	Q_DISABLE_COPY_MOVE(PolkitAgentImpl);
 
 public:
+	~PolkitAgentImpl() override;
+
 	static PolkitAgentImpl* tryGetOrCreate(PolkitAgent* agent);
 	static PolkitAgentImpl* tryGet(const PolkitAgent* agent);
 	static PolkitAgentImpl* tryTakeover(PolkitAgent* agent);
@@ -27,7 +31,6 @@ class PolkitAgentImpl
 
 private:
 	PolkitAgentImpl(PolkitAgent* agent);
-	~PolkitAgentImpl() override;
 
 	static PolkitAgentImpl* instance;
 
@@ -36,7 +39,7 @@ class PolkitAgentImpl
 	/// Finalize and remove the current authentication request.
 	void finishAuthenticationRequest();
 
-	QsPolkitAgent* listener = nullptr;
+	GObjectRef<QsPolkitAgent> listener;
 	bool isRegistered = false;
 
 	PolkitAgent* qmlAgent = nullptr;

src/services/polkit/flow.cpp

@@ -4,6 +4,7 @@
 #include <qlist.h>
 #include <qloggingcategory.h>
 #include <qobject.h>
+#include <qqmlinfo.h>
 #include <qtmetamacros.h>
 
 #include "../../core/logcat.hpp"
@@ -20,9 +21,15 @@ AuthFlow::AuthFlow(AuthRequest* request, QList<Identity*>&& identities, QObject*
     : QObject(parent)
     , mRequest(request)
     , mIdentities(std::move(identities))
-    , mSelectedIdentity(mIdentities.isEmpty() ? nullptr : mIdentities.first()) {
+    , mSelectedIdentity(this->mIdentities.isEmpty() ? nullptr : this->mIdentities.first()) {
+	// We reject auth requests with no identities before a flow is created.
+	// This should never happen.
 	if (!this->mSelectedIdentity)
-		qCCritical(logPolkitState) << "AuthFlow created with no valid identities!";
+		qCFatal(logPolkitState) << "AuthFlow created with no valid identities!";
+
+	for (auto* identity: this->mIdentities) {
+		identity->setParent(this);
+	}
 
 	this->setupSession();
 }
@@ -39,7 +46,10 @@ Identity* AuthFlow::selectedIdentity() const { return this->mSelectedIdentity; }
 
 void AuthFlow::setSelectedIdentity(Identity* identity) {
 	if (this->mSelectedIdentity == identity) return;
-	if (!identity) return; // ignore null changes
+	if (!identity) {
+		qmlWarning(this) << "Cannot set selected identity to null.";
+		return;
+	}
 	for (auto* id: this->mIdentities) {
 		if (id == identity) {
 			this->mSelectedIdentity = id;
@@ -101,7 +111,7 @@ void AuthFlow::setupSession() {
 	qCDebug(logPolkitState) << "setting up session for identity" << this->mSelectedIdentity->name();
 
 	this->currentSession =
-	    new Session(this->mSelectedIdentity->polkitIdentity, this->mRequest->cookie, this);
+	    new Session(this->mSelectedIdentity->polkitIdentity.get(), this->mRequest->cookie, this);
 	QObject::connect(this->currentSession, &Session::request, this, &AuthFlow::request);
 	QObject::connect(this->currentSession, &Session::completed, this, &AuthFlow::completed);
 	QObject::connect(this->currentSession, &Session::showError, this, &AuthFlow::showError);

src/services/polkit/flow.hpp

@@ -15,6 +15,7 @@ class AuthFlow
     , public Retainable {
 	Q_OBJECT;
 	QML_ELEMENT;
+	Q_DISABLE_COPY_MOVE(AuthFlow);
 	QML_UNCREATABLE("AuthFlow can only be obtained from PolkitAgent.");
 
 	// clang-format off
@@ -24,6 +25,8 @@ class AuthFlow
 	/// The icon to present to the user in association with the message.
 	///
 	/// The icon name follows the [FreeDesktop icon naming specification](https://specifications.freedesktop.org/icon-naming-spec/icon-naming-spec-latest.html).
+	/// Use @@Quickshell.Quickshell.iconPath() to resolve the icon name to an
+	/// actual file path for display.
 	Q_PROPERTY(QString iconName READ iconName CONSTANT);
 
 	/// The action ID represents the action that is being authorized.
@@ -44,10 +47,10 @@ class AuthFlow
 	Q_PROPERTY(QList<Identity*> identities READ identities CONSTANT);
 
 	/// The identity that will be used to authenticate.
-	/// 
-	/// Setting this will abort any ongoing authentication conversations and start a new one.
+	///
+	/// Changing this will abort any ongoing authentication conversations and start a new one.
 	Q_PROPERTY(Identity* selectedIdentity READ selectedIdentity WRITE setSelectedIdentity NOTIFY selectedIdentityChanged);
-	
+
 	/// Indicates that a response from the user is required from the user,
 	/// typically a password.
 	Q_PROPERTY(bool isResponseRequired READ isResponseRequired NOTIFY responseRequestChanged);
@@ -61,6 +64,7 @@ class AuthFlow
 	/// An additional message to present to the user.
 	///
 	/// This may be used to show errors or supplementary information.
+	/// See @@supplementaryIsError to determine if this is an error message.
 	Q_PROPERTY(QString supplementaryMessage READ supplementaryMessage NOTIFY supplementaryChanged);
 
 	/// Indicates whether the supplementary message is an error.

src/services/polkit/gobjectref.hpp

@@ -0,0 +1,65 @@
+#pragma once
+
+#include <glib-object.h>
+
+namespace qs::service::polkit {
+
+struct GObjectNoRefTag {};
+constexpr GObjectNoRefTag G_OBJECT_NO_REF;
+
+template <typename T>
+class GObjectRef {
+public:
+	explicit GObjectRef(T* ptr = nullptr): ptr(ptr) {
+		if (this->ptr) {
+			g_object_ref(this->ptr);
+		}
+	}
+
+	explicit GObjectRef(T* ptr, GObjectNoRefTag /*tag*/): ptr(ptr) {}
+
+	~GObjectRef() {
+		if (this->ptr) {
+			g_object_unref(this->ptr);
+		}
+	}
+
+	// We do handle self-assignment in a more general case by checking the
+	// included pointers rather than the wrapper objects themselves.
+	// NOLINTBEGIN(bugprone-unhandled-self-assignment)
+
+	GObjectRef(const GObjectRef& other): GObjectRef(other.ptr) {}
+	GObjectRef& operator=(const GObjectRef& other) {
+		if (*this == other) return *this;
+		if (this->ptr) {
+			g_object_unref(this->ptr);
+		}
+		this->ptr = other.ptr;
+		if (this->ptr) {
+			g_object_ref(this->ptr);
+		}
+		return *this;
+	}
+
+	GObjectRef(GObjectRef&& other) noexcept: ptr(other.ptr) { other.ptr = nullptr; }
+	GObjectRef& operator=(GObjectRef&& other) noexcept {
+		if (*this == other) return *this;
+		if (this->ptr) {
+			g_object_unref(this->ptr);
+		}
+		this->ptr = other.ptr;
+		other.ptr = nullptr;
+		return *this;
+	}
+
+	// NOLINTEND(bugprone-unhandled-self-assignment)
+
+	[[nodiscard]] T* get() const { return this->ptr; }
+	T* operator->() const { return this->ptr; }
+
+	bool operator==(const GObjectRef<T>& other) const { return this->ptr == other.ptr; }
+
+private:
+	T* ptr;
+};
+} // namespace qs::service::polkit

src/services/polkit/identity.cpp

@@ -1,8 +1,11 @@
 #include "identity.hpp"
+#include <type_traits>
 #include <utility>
+#include <vector>
 
 #include <qobject.h>
 #include <qtmetamacros.h>
+#include <qtypes.h>
 #include <sys/types.h>
 
 #define POLKIT_AGENT_I_KNOW_API_IS_SUBJECT_TO_CHANGE
@@ -12,53 +15,77 @@
 #define signals Q_SIGNALS
 #include <grp.h>
 #include <pwd.h>
+#include <unistd.h>
+
+#include "gobjectref.hpp"
 
 namespace qs::service::polkit {
 Identity::Identity(
     id_t id,
     QString name,
     QString displayName,
     bool isGroup,
-    PolkitIdentity* polkitIdentity,
+    GObjectRef<PolkitIdentity> polkitIdentity,
     QObject* parent
 )
     : QObject(parent)
-    , polkitIdentity(polkitIdentity)
+    , polkitIdentity(std::move(polkitIdentity))
     , mId(id)
     , mName(std::move(name))
     , mDisplayName(std::move(displayName))
     , mIsGroup(isGroup) {}
 
 Identity::~Identity() = default;
 
-Identity* Identity::fromPolkitIdentity(PolkitIdentity* identity) {
-	if (POLKIT_IS_UNIX_USER(identity)) {
-		auto uid = polkit_unix_user_get_uid(POLKIT_UNIX_USER(identity));
-		auto* pw = getpwuid(uid);
+Identity* Identity::fromPolkitIdentity(GObjectRef<PolkitIdentity> identity) {
+	if (POLKIT_IS_UNIX_USER(identity.get())) {
+		auto uid = polkit_unix_user_get_uid(POLKIT_UNIX_USER(identity.get()));
+
+		auto bufSize = sysconf(_SC_GETPW_R_SIZE_MAX);
+		// The call can fail with -1, in this case choose a default that is
+		// big enough.
+		if (bufSize == -1) bufSize = 16384;
+		auto buffer = std::vector<char>(bufSize);
+
+		std::aligned_storage_t<sizeof(passwd), alignof(passwd)> pwBuf;
+		passwd* pw = nullptr;
+		getpwuid_r(uid, reinterpret_cast<passwd*>(&pwBuf), buffer.data(), bufSize, &pw);
+
 		auto name =
 		    (pw && pw->pw_name && *pw->pw_name) ? QString::fromUtf8(pw->pw_name) : QString::number(uid);
+
 		return new Identity(
 		    uid,
 		    name,
 		    (pw && pw->pw_gecos && *pw->pw_gecos) ? QString::fromUtf8(pw->pw_gecos) : name,
 		    false,
-		    identity
+		    std::move(identity)
 		);
 	}
 
-	if (POLKIT_IS_UNIX_GROUP(identity)) {
-		auto gid = polkit_unix_group_get_gid(POLKIT_UNIX_GROUP(identity));
-		auto* gr = getgrgid(gid);
+	if (POLKIT_IS_UNIX_GROUP(identity.get())) {
+		auto gid = polkit_unix_group_get_gid(POLKIT_UNIX_GROUP(identity.get()));
+
+		auto bufSize = sysconf(_SC_GETGR_R_SIZE_MAX);
+		// The call can fail with -1, in this case choose a default that is
+		// big enough.
+		if (bufSize == -1) bufSize = 16384;
+		auto buffer = std::vector<char>(bufSize);
+
+		std::aligned_storage_t<sizeof(group), alignof(group)> grBuf;
+		group* gr = nullptr;
+		getgrgid_r(gid, reinterpret_cast<group*>(&grBuf), buffer.data(), bufSize, &gr);
+
 		auto name =
 		    (gr && gr->gr_name && *gr->gr_name) ? QString::fromUtf8(gr->gr_name) : QString::number(gid);
-		return new Identity(gid, name, name, true, identity);
+		return new Identity(gid, name, name, true, std::move(identity));
 	}
 
 	// A different type of identity is netgroup.
 	return nullptr;
 }
 
-id_t Identity::id() const { return this->mId; }
+quint32 Identity::id() const { return this->mId; }
 const QString& Identity::name() const { return this->mName; }
 const QString& Identity::displayName() const { return this->mDisplayName; }
 bool Identity::isGroup() const { return this->mIsGroup; }

src/services/polkit/identity.hpp

@@ -3,16 +3,21 @@
 #include <qobject.h>
 #include <qqmlintegration.h>
 
-using PolkitIdentity = struct _PolkitIdentity;
+#include "gobjectref.hpp"
+
+// _PolkitIdentity is considered a reserved identifier, but I am specifically
+// forward declaring this reserved name.
+using PolkitIdentity = struct _PolkitIdentity; // NOLINT(bugprone-reserved-identifier)
 
 namespace qs::service::polkit {
 //! Represents a user or group that can be used to authenticate.
 class Identity: public QObject {
 	Q_OBJECT;
+	Q_DISABLE_COPY_MOVE(Identity);
 
 	// clang-format off
 	/// The Id of the identity. If the identity is a user, this is the user's uid. See @@isGroup.
-	Q_PROPERTY(id_t id READ id CONSTANT);
+	Q_PROPERTY(quint32 id READ id CONSTANT);
 
 	/// The name of the user or group.
 	///
@@ -36,19 +41,19 @@ class Identity: public QObject {
 	    QString name,
 	    QString displayName,
 	    bool isGroup,
-	    PolkitIdentity* polkitIdentity,
+	    GObjectRef<PolkitIdentity> polkitIdentity,
 	    QObject* parent = nullptr
 	);
 	~Identity() override;
 
-	static Identity* fromPolkitIdentity(PolkitIdentity* identity);
+	static Identity* fromPolkitIdentity(GObjectRef<PolkitIdentity> identity);
 
-	[[nodiscard]] id_t id() const;
+	[[nodiscard]] quint32 id() const;
 	[[nodiscard]] const QString& name() const;
 	[[nodiscard]] const QString& displayName() const;
 	[[nodiscard]] bool isGroup() const;
 
-	PolkitIdentity* polkitIdentity;
+	GObjectRef<PolkitIdentity> polkitIdentity;
 
 private:
 	id_t mId;