Re: [PATCH] src: Threat pid as signed

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Oct 07, 2016 at 11:22:34AM +0200, Michal Privoznik wrote:
This initially started as a fix of some debug printing in
virCgroupDetect. However it turned out that other places suffer
from the similar problem. While dealing with pids, esp. in cases
where we cannot use pid_t for ABI stability reasons, we often
chose an unsigned integer type. This makes no sense as pid_t is
signed.
Also, new syntax-check rule is introduced so we won't repeat this
mistake.

Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
---

Technically, this is v2 of [1], but like 99% of the patch is
different, so I'm sending this as a completely new patch.

1: https://www.redhat.com/archives/libvir-list/2016-October/msg00254.html

cfg.mk                            |  8 ++++++++
src/locking/lock_driver_sanlock.c |  4 ++--
src/lxc/lxc_controller.c          |  4 ++--
src/lxc/lxc_domain.c              |  8 ++++----
src/lxc/lxc_driver.c              |  4 ++--
src/lxc/lxc_monitor.c             |  3 +--
src/lxc/lxc_process.c             |  8 ++++----
src/qemu/qemu_process.c           | 12 ++++++------
src/util/vircgroup.c              | 24 ++++++++++++------------
src/util/viridentity.c            |  2 +-
src/util/virpolkit.c              |  1 +
src/util/virprocess.c             | 14 ++++++--------
src/util/virsystemd.c             |  9 +++++----
src/util/virutil.c                |  4 ++--
14 files changed, 56 insertions(+), 49 deletions(-)

diff --git a/cfg.mk b/cfg.mk
index 9f5949c..b33b1e2 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -581,6 +581,11 @@ sc_prohibit_int_assign_bool:
	halt='use bool type for boolean values'				\
	  $(_sc_search_regexp)

+sc_prohibit_unsigned_pid:
+	@prohibit='\<unsigned\> [^,=]+pid'						\

I'd also add ';' and '(' in this  ^^ list of characters, but that's your
call on those.

+	halt='use signed type for pid values'				\
+	  $(_sc_search_regexp)
+
# Many of the function names below came from this filter:
# git grep -B2 '\<_('|grep -E '\.c- *[[:alpha:]_][[:alnum:]_]* ?\(.*[,;]$' \
# |sed 's/.*\.c-  *//'|perl -pe 's/ ?\(.*//'|sort -u \
@@ -1214,6 +1219,9 @@ exclude_file_name_regexp--sc_prohibit_include_public_headers_brackets = \
exclude_file_name_regexp--sc_prohibit_int_ijk = \
  ^(src/remote_protocol-structs|src/remote/remote_protocol\.x|cfg\.mk|include/libvirt/libvirt.+|src/admin_protocol-structs|src/admin/admin_protocol\.x)$$

+exclude_file_name_regexp--sc_prohibit_unsigned_pid = \
+  ^(include/libvirt/libvirt-qemu.h|src/(driver-hypervisor.h|libvirt-qemu.c|locking/lock_protocol.x|lxc/lxc_monitor_protocol.x|qemu/qemu_driver.c|remote/qemu_protocol.x|util/virpolkit.c|util/virsystemd.c)|tests/virpolkittest.c|tools/virsh-domain.c)$$
+

It's regexp, so dots need to be escaped.  Also if this needs such
exceptions, I don't see the point in the syntax-check.  But anyway, I
think all protocols and all public APIs should have this exception, so
it can be made shorter.  I came up with this:

^(include/libvirt/.*\.h|src/(qemu/qemu_driver\.c|driver-hypervisor\.h|libvirt(-[a-z]*)?\.c|.*\.x|util/vir(polkit|systemd)\.c)|tests/virpolkittest\.c|tools/virsh-domain\.c)$$

diff --git a/src/util/virpolkit.c b/src/util/virpolkit.c
index e7e46b8..d8c35f1 100644
--- a/src/util/virpolkit.c
+++ b/src/util/virpolkit.c
@@ -82,6 +82,7 @@ int virPolkitCheckAuth(const char *actionid,
    VIR_INFO("Checking PID %lld running as %d",
             (long long) pid, uid);

+    /* Yes, PolicyKit really takes pid ad uint. */
    if (virDBusCallMethod(sysbus,
                          &reply,
                          NULL,

You can safely drop this ^^ hunk.

diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index 8b9f9c5..718c4a2 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -608,8 +608,7 @@ int virProcessGetPids(pid_t pid, size_t *npids, pid_t **pids)
    *npids = 0;
    *pids = NULL;

-    if (virAsprintf(&taskPath, "/proc/%llu/task",
-                    (unsigned long long)pid) < 0)
+    if (virAsprintf(&taskPath, "/proc/%llu/task", (long long) pid) < 0)
        goto cleanup;

    if (virDirOpen(&dir, taskPath) < 0)

I see you changed lot of '(type)val' casts to '(type) val', and even
though I used to hate this, I'm kinda more in favour of the latter
nowadays.  And guess what, I looked up how it looks in the code and
after this patch, the ratio of non-spaced to spaced type casts changed
from 751:3394 to 727:3417 (very roughly), so Yay!

ACK with nits fixed.

Martin

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]