On 2019-11-27, Damien Miller <djm@xxxxxxxxxxx> wrote:
> applied, thanks. Surprised the fairly aggressive set of -W flags we use
> by default didn't catch this...
This can be caught with -Wpedantic, which seems to catch a few more
instances of this, as well as passing non-void pointers to printf for
the `%p` format specifier. Patches attached for these.
However, it also catches conversion from dlsym result (void *) to
function pointer, which is not allowed in ISO C, but POSIX requires it
to work:
"Note that conversion from a void * pointer to a function pointer is
not defined by the ISO C standard. This standard requires this
conversion to work correctly on conforming implementations."
The only way I've found to get around this warning is to cast the
address of the function pointer lvalue to void **, then dereference,
as in
*(void **)&fn = dlsym(handle, "foo");
There are three instances of this in ssh-sk.c and one in ssh-pkcs11.c.
I'm not aware of a way to turn on the rest of -Wpedantic except for
object-function pointer conversions, but I do think it is a good idea
to turn it on, so perhaps it is worth addressing...
From 098526485ee6fe0a47bbff7b56a8c2339c7b1549 Mon Sep 17 00:00:00 2001
From: Michael Forney <mforney@xxxxxxxxxxx>
Date: Wed, 27 Nov 2019 19:16:26 -0800
Subject: [PATCH 1/3] printf %p specifier requires `void *` argument
---
monitor.c | 4 ++--
session.c | 2 +-
ssh-pkcs11-helper.c | 2 +-
ssh-pkcs11.c | 16 +++++++++-------
sshbuf-misc.c | 2 +-
5 files changed, 14 insertions(+), 12 deletions(-)
diff --git a/monitor.c b/monitor.c
index 64eca98d..e021c81c 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1166,7 +1166,7 @@ mm_answer_keyallowed(struct ssh *ssh, int sock, struct sshbuf *m)
(r = sshbuf_get_u32(m, &pubkey_auth_attempt)) != 0)
fatal("%s: buffer error: %s", __func__, ssh_err(r));
- debug3("%s: key_from_blob: %p", __func__, key);
+ debug3("%s: key_from_blob: %p", __func__, (void *)key);
if (key != NULL && authctxt->valid) {
/* These should not make it past the privsep child */
@@ -1434,7 +1434,7 @@ mm_answer_keyverify(struct ssh *ssh, int sock, struct sshbuf *m)
ret = sshkey_verify(key, signature, signaturelen, data, datalen,
sigalg, ssh->compat, &sig_details);
- debug3("%s: %s %p signature %s%s%s", __func__, auth_method, key,
+ debug3("%s: %s %p signature %s%s%s", __func__, auth_method, (void *)key,
(ret == 0) ? "verified" : "unverified",
(ret != 0) ? ": " : "", (ret != 0) ? ssh_err(ret) : "");
diff --git a/session.c b/session.c
index 80738b92..2b1a5d4e 100644
--- a/session.c
+++ b/session.c
@@ -1787,7 +1787,7 @@ session_dump(void)
s->used,
s->next_unused,
s->self,
- s,
+ (void *)s,
s->chanid,
(long)s->pid);
}
diff --git a/ssh-pkcs11-helper.c b/ssh-pkcs11-helper.c
index 219ce9b5..1131832c 100644
--- a/ssh-pkcs11-helper.c
+++ b/ssh-pkcs11-helper.c
@@ -96,7 +96,7 @@ lookup_key(struct sshkey *k)
struct pkcs11_keyinfo *ki;
TAILQ_FOREACH(ki, &pkcs11_keylist, next) {
- debug("check %p %s", ki, ki->providername);
+ debug("check %p %s", (void *)ki, ki->providername);
if (sshkey_equal(k, ki->key))
return (ki->key);
}
diff --git a/ssh-pkcs11.c b/ssh-pkcs11.c
index 09f1ea34..89a83b39 100644
--- a/ssh-pkcs11.c
+++ b/ssh-pkcs11.c
@@ -112,7 +112,7 @@ pkcs11_provider_finalize(struct pkcs11_provider *p)
CK_ULONG i;
debug("pkcs11_provider_finalize: %p refcount %d valid %d",
- p, p->refcount, p->valid);
+ (void *)p, p->refcount, p->valid);
if (!p->valid)
return;
for (i = 0; i < p->nslots; i++) {
@@ -135,10 +135,12 @@ pkcs11_provider_finalize(struct pkcs11_provider *p)
static void
pkcs11_provider_unref(struct pkcs11_provider *p)
{
- debug("pkcs11_provider_unref: %p refcount %d", p, p->refcount);
+ debug("pkcs11_provider_unref: %p refcount %d", (void *)p, p->refcount);
if (--p->refcount <= 0) {
- if (p->valid)
- error("pkcs11_provider_unref: %p still valid", p);
+ if (p->valid) {
+ error("pkcs11_provider_unref: %p still valid",
+ (void *)p);
+ }
free(p->name);
free(p->slotlist);
free(p->slotinfo);
@@ -166,7 +168,7 @@ pkcs11_provider_lookup(char *provider_id)
struct pkcs11_provider *p;
TAILQ_FOREACH(p, &pkcs11_providers, next) {
- debug("check %p %s", p, p->name);
+ debug("check %p %s", (void *)p, p->name);
if (!strcmp(provider_id, p->name))
return (p);
}
@@ -323,7 +325,7 @@ pkcs11_check_obj_bool_attrib(struct pkcs11_key *k11, CK_OBJECT_HANDLE obj,
}
*val = flag != 0;
debug("%s: provider %p slot %lu object %lu: attrib %lu = %d",
- __func__, k11->provider, k11->slotidx, obj, type, *val);
+ __func__, (void *)k11->provider, k11->slotidx, obj, type, *val);
return (0);
}
@@ -415,7 +417,7 @@ pkcs11_rsa_private_encrypt(int flen, const u_char *from, u_char *to, RSA *rsa,
int rval = -1;
if ((k11 = RSA_get_ex_data(rsa, rsa_idx)) == NULL) {
- error("RSA_get_ex_data failed for rsa %p", rsa);
+ error("RSA_get_ex_data failed for rsa %p", (void *)rsa);
return (-1);
}
diff --git a/sshbuf-misc.c b/sshbuf-misc.c
index a73f008b..d4714ee1 100644
--- a/sshbuf-misc.c
+++ b/sshbuf-misc.c
@@ -65,7 +65,7 @@ sshbuf_dump_data(const void *s, size_t len, FILE *f)
void
sshbuf_dump(struct sshbuf *buf, FILE *f)
{
- fprintf(f, "buffer %p len = %zu\n", buf, sshbuf_len(buf));
+ fprintf(f, "buffer %p len = %zu\n", (void *)buf, sshbuf_len(buf));
sshbuf_dump_data(sshbuf_ptr(buf), sshbuf_len(buf), f);
}
--
2.20.1
From f6dacbf00b0185f231d5ef8080617f3b2f6c497a Mon Sep 17 00:00:00 2001
From: Michael Forney <mforney@xxxxxxxxxxx>
Date: Wed, 27 Nov 2019 19:17:26 -0800
Subject: [PATCH 2/3] Fix sha2 MAKE_CLONE no-op definition
The point of the dummy declaration is so that MAKE_CLONE(...) can have
a trailing semicolon without introducing an empty declaration. So,
the macro replacement text should *not* have a trailing semicolon,
just like DEF_WEAK.
---
openbsd-compat/sha2.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/openbsd-compat/sha2.c b/openbsd-compat/sha2.c
index e63324c9..e36cc24e 100644
--- a/openbsd-compat/sha2.c
+++ b/openbsd-compat/sha2.c
@@ -42,7 +42,7 @@
!defined(HAVE_SHA512UPDATE)
/* no-op out, similar to DEF_WEAK but only needed here */
-#define MAKE_CLONE(x, y) void __ssh_compat_make_clone_##x_##y(void);
+#define MAKE_CLONE(x, y) void __ssh_compat_make_clone_##x_##y(void)
#include <string.h>
#include <sha2.h>
--
2.20.1
From af14b036ab9f4d97f59a7f2394bc3c3f215031b1 Mon Sep 17 00:00:00 2001
From: Michael Forney <mforney@xxxxxxxxxxx>
Date: Wed, 27 Nov 2019 19:37:17 -0800
Subject: [PATCH 3/3] Remove trailing semicolon after RB_GENERATE_STATIC
This expands to a series of function definitions, so the semicolon is
not necessary (in fact, it is not allowed in ISO C).
---
krl.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/krl.c b/krl.c
index aa8318cf..01522b8f 100644
--- a/krl.c
+++ b/krl.c
@@ -60,7 +60,7 @@ struct revoked_serial {
};
static int serial_cmp(struct revoked_serial *a, struct revoked_serial *b);
RB_HEAD(revoked_serial_tree, revoked_serial);
-RB_GENERATE_STATIC(revoked_serial_tree, revoked_serial, tree_entry, serial_cmp);
+RB_GENERATE_STATIC(revoked_serial_tree, revoked_serial, tree_entry, serial_cmp)
/* Tree of key IDs */
struct revoked_key_id {
@@ -69,7 +69,7 @@ struct revoked_key_id {
};
static int key_id_cmp(struct revoked_key_id *a, struct revoked_key_id *b);
RB_HEAD(revoked_key_id_tree, revoked_key_id);
-RB_GENERATE_STATIC(revoked_key_id_tree, revoked_key_id, tree_entry, key_id_cmp);
+RB_GENERATE_STATIC(revoked_key_id_tree, revoked_key_id, tree_entry, key_id_cmp)
/* Tree of blobs (used for keys and fingerprints) */
struct revoked_blob {
@@ -79,7 +79,7 @@ struct revoked_blob {
};
static int blob_cmp(struct revoked_blob *a, struct revoked_blob *b);
RB_HEAD(revoked_blob_tree, revoked_blob);
-RB_GENERATE_STATIC(revoked_blob_tree, revoked_blob, tree_entry, blob_cmp);
+RB_GENERATE_STATIC(revoked_blob_tree, revoked_blob, tree_entry, blob_cmp)
/* Tracks revoked certs for a single CA */
struct revoked_certs {
--
2.20.1
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@xxxxxxxxxxx
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
