DISPATCH-2355: Implement the `format` attribute for `qd_log_impl` and other such functions to hopefully have compile-time check for format strings (#1630)
This is backported code from skupper-router, https://github.com/skupperproject/skupper-router/issues/1011
diff --git a/include/qpid/dispatch/error.h b/include/qpid/dispatch/error.h
index 0bbce40..1b4f928 100644
--- a/include/qpid/dispatch/error.h
+++ b/include/qpid/dispatch/error.h
@@ -68,7 +68,8 @@
*/
#define qd_verror(code, fmt, ap) qd_error_vimpl(code, __FILE__, __LINE__, fmt, ap)
-qd_error_t qd_error_impl(qd_error_t code, const char *file, int line, const char *fmt, ...);
+qd_error_t qd_error_impl(qd_error_t code, const char *file, int line, const char *fmt, ...)
+ __attribute__((format(printf, 4, 5)));
qd_error_t qd_error_vimpl(qd_error_t code, const char *file, int line, const char *fmt, va_list ap);
/**
@@ -117,6 +118,7 @@
*/
#define qd_error_errno(errnum, ...) qd_error_errno_impl(errnum, __FILE__, __LINE__, __VA_ARGS__)
-qd_error_t qd_error_errno_impl(int errnum, const char *file, int line, const char *fmt, ...);
+qd_error_t qd_error_errno_impl(int errnum, const char *file, int line, const char *fmt, ...)
+ __attribute__((format(printf, 4, 5)));
#endif
diff --git a/include/qpid/dispatch/log.h b/include/qpid/dispatch/log.h
index 6265029..c0c2e27 100644
--- a/include/qpid/dispatch/log.h
+++ b/include/qpid/dispatch/log.h
@@ -44,13 +44,15 @@
/**@internal*/
bool qd_log_enabled(qd_log_source_t *source, qd_log_level_t level);
/**@internal*/
-void qd_log_impl(qd_log_source_t *source, qd_log_level_t level, const char *file, int line, const char *fmt, ...);
+void qd_log_impl(qd_log_source_t *source, qd_log_level_t level, const char *file, int line, const char *fmt, ...)
+ __attribute__((format(printf, 5, 6)));
/**
* Another version of the qd_log_impl function. This function unconditionally writes the the message to the log file.
* It does not check to see if the passed in log level is enabled.
*/
-void qd_log_impl_v1(qd_log_source_t *source, qd_log_level_t level, const char *file, int line, const char *fmt, ...);
+void qd_log_impl_v1(qd_log_source_t *source, qd_log_level_t level, const char *file, int line, const char *fmt, ...)
+ __attribute__((format(printf, 5, 6)));
void qd_vlog_impl(qd_log_source_t *source, qd_log_level_t level, bool check_level, const char *file, int line, const char *fmt, va_list ap);
/** Log a message
@@ -80,6 +82,6 @@
/** Maximum length for a log message */
int qd_log_max_len();
-void qd_format_string(char* buf, int buf_size, const char *fmt, ...);
+void qd_format_string(char *buf, int buf_size, const char *fmt, ...) __attribute__((format(printf, 3, 4)));
#endif
diff --git a/src/aprintf.h b/src/aprintf.h
index b259d44..99c424b 100644
--- a/src/aprintf.h
+++ b/src/aprintf.h
@@ -36,6 +36,6 @@
- n > 0: printing was truncated and would have printed n characters. *begin == end-1
- n < 0: error (return value of vsnprintf) no change to *begin
*/
-int aprintf(char **begin, char *end, const char *format, ...);
+int aprintf(char **begin, char *end, const char *format, ...) __attribute__((format(printf, 3, 4)));
#endif
diff --git a/src/error.c b/src/error.c
index beb4e43..7998f6b 100644
--- a/src/error.c
+++ b/src/error.c
@@ -207,7 +207,7 @@
va_start(arglist, fmt);
vaprintf(&begin, end, fmt, arglist);
va_end(arglist);
- aprintf(&begin, end, ": ", errnum);
+ aprintf(&begin, end, ": ");
char *em = ts.error_message;
if(strerror_r(errnum, begin, end - begin) != 0) {
snprintf(begin, end - begin, "Unknown error %d", errnum);
diff --git a/src/log.c b/src/log.c
index 0bada29..ab848e1 100644
--- a/src/log.c
+++ b/src/log.c
@@ -657,7 +657,7 @@
if (error_in_output) {
char msg[TEXT_MAX];
snprintf(msg, sizeof(msg), "Failed to open log file '%s'", outputFile);
- qd_error(QD_ERROR_CONFIG, msg);
+ qd_error(QD_ERROR_CONFIG, "%s", msg);
}
if (error_in_enable) {
qd_error(QD_ERROR_CONFIG, "'%s' is not a valid log level. Should be one of {%s}.", enable, level_names);
diff --git a/src/message.c b/src/message.c
index 1f9f3d6..8f810b4 100644
--- a/src/message.c
+++ b/src/message.c
@@ -343,7 +343,7 @@
char *begin = buffer;
char *end = buffer + len - sizeof(REPR_END); /* Save space for ending */
bool first = true;
- aprintf(&begin, end, "Message{", msg);
+ aprintf(&begin, end, "Message{");
print_field(msg, QD_FIELD_MESSAGE_ID, "message-id", flags, &first, &begin, end);
print_field(msg, QD_FIELD_USER_ID, "user-id", flags, &first, &begin, end);
print_field(msg, QD_FIELD_TO, "to", flags, &first, &begin, end);
diff --git a/src/python_embedded.c b/src/python_embedded.c
index 0fd6a45..a7e40bc 100644
--- a/src/python_embedded.c
+++ b/src/python_embedded.c
@@ -584,10 +584,10 @@
// Parse an iterator to a python object.
static PyObject *py_iter_parse(qd_iterator_t *iter)
{
- qd_parsed_field_t *parsed=0;
+ qd_parsed_field_t *parsed = 0;
if (iter && (parsed = qd_parse(iter))) {
if (!qd_parse_ok(parsed)) {
- qd_error(QD_ERROR_MESSAGE, qd_parse_error(parsed));
+ qd_error(QD_ERROR_MESSAGE, "%s", qd_parse_error(parsed));
qd_parse_free(parsed);
return 0;
}
diff --git a/src/router_core/core_client_api.c b/src/router_core/core_client_api.c
index c292eed..e24561f 100644
--- a/src/router_core/core_client_api.c
+++ b/src/router_core/core_client_api.c
@@ -181,8 +181,8 @@
NULL, // target terminus
&receiver_endpoint,
client);
- qd_log(core->log, QD_LOG_TRACE,
- "New core client created c=%p", client);
+ qd_log(core->log, QD_LOG_TRACE, //
+ "New core client created c=%p", (void *) client);
return client;
}
@@ -221,8 +221,8 @@
qd_hash_free(client->correlations);
free(client->reply_to);
- qd_log(client->core->log, QD_LOG_TRACE,
- "Core client freed c=%p", client);
+ qd_log(client->core->log, QD_LOG_TRACE, //
+ "Core client freed c=%p", (void *) client);
free_qdrc_client_t(client);
}
@@ -239,8 +239,8 @@
qdrc_client_request_done_CT_t done_cb)
{
qd_log(client->core->log, QD_LOG_TRACE,
- "New core client request created c=%p, rc=%p",
- client, request_context);
+ "New core client request created c=%p, rc=%p", (void *) client,
+ request_context);
qdrc_client_request_t *req = new_qdrc_client_request_t();
ZERO(req);
@@ -296,9 +296,9 @@
DEQ_REMOVE_HEAD_N(SEND_Q, client->send_queue);
req->on_send_queue = false;
- qd_log(client->core->log, QD_LOG_TRACE,
- "Core client request sent c=%p, rc=%p dlv=%p cid=%s",
- client, req->req_context, req->delivery,
+ qd_log(client->core->log, QD_LOG_TRACE, //
+ "Core client request sent c=%p, rc=%p dlv=%p cid=%s", //
+ (void *) client, req->req_context, (void *) req->delivery, //
*req->correlation_id ? req->correlation_id : "<none>");
if (!presettled && req->on_ack_cb) {
@@ -363,9 +363,9 @@
error);
}
- qd_log(client->core->log, QD_LOG_TRACE,
- "Freeing core client request c=%p, rc=%p (%s)",
- client, req->req_context,
+ qd_log(client->core->log, QD_LOG_TRACE, //
+ "Freeing core client request c=%p, rc=%p (%s)", //
+ (void *) client, req->req_context, //
error ? error : "request complete");
free_qdrc_client_request_t(req);
@@ -397,8 +397,8 @@
{
qdrc_client_t *client = (qdrc_client_t *)context;
- qd_log(client->core->log, QD_LOG_TRACE,
- "Core client sender 2nd attach c=%p", client);
+ qd_log(client->core->log, QD_LOG_TRACE, //
+ "Core client sender 2nd attach c=%p", (void *) client);
if (!client->sender_up) {
client->sender_up = true;
@@ -415,8 +415,8 @@
{
qdrc_client_t *client = (qdrc_client_t *)context;
- qd_log(client->core->log, QD_LOG_TRACE,
- "Core client receiver 2nd attach c=%p", client);
+ qd_log(client->core->log, QD_LOG_TRACE, //
+ "Core client receiver 2nd attach c=%p", (void *) client);
if (!client->receiver_up) {
client->receiver_up = true;
@@ -437,9 +437,9 @@
qdr_core_t *core = client->core;
client->tx_credit += available_credit;
- qd_log(core->log, QD_LOG_TRACE,
- "Core client sender flow granted c=%p credit=%d d=%s",
- client, client->tx_credit, (drain) ? "T" : "F");
+ qd_log(core->log, QD_LOG_TRACE, //
+ "Core client sender flow granted c=%p credit=%d d=%s", //
+ (void *) client, client->tx_credit, (drain) ? "T" : "F");
if (client->tx_credit > 0) {
_flush_send_queue_CT(client);
}
@@ -464,9 +464,9 @@
{
qdrc_client_t *client = (qdrc_client_t *)context;
- qd_log(client->core->log, QD_LOG_TRACE,
- "Core client sender update c=%p dlv=%p d=%"PRIx64" %s",
- client, delivery, disposition,
+ qd_log(client->core->log, QD_LOG_TRACE, //
+ "Core client sender update c=%p dlv=%p d=%" PRIx64 " %s", //
+ (void *) client, (void *) delivery, disposition, //
settled ? "settled" : "unsettled");
if (disposition) {
@@ -497,7 +497,7 @@
qd_log(client->core->log, QD_LOG_DEBUG,
"Core client could not find request for disposition update"
" client=%p delivery=%p",
- client, delivery);
+ (void *) client, (void *) delivery);
}
}
}
@@ -512,8 +512,8 @@
bool complete = qd_message_receive_complete(message);
qd_log(core->log, QD_LOG_TRACE,
- "Core client received msg c=%p complete=%s",
- client, complete ? "T" : "F");
+ "Core client received msg c=%p complete=%s", //
+ (void *) client, complete ? "T" : "F");
if (complete) {
uint64_t disposition = PN_ACCEPTED;
@@ -527,10 +527,9 @@
qd_hash_retrieve(client->correlations, cid_iter, (void **)&req);
qd_iterator_free(cid_iter);
if (req) {
-
qd_log(core->log, QD_LOG_TRACE,
- "Core client received msg c=%p rc=%p cid=%s",
- client, req->req_context, req->correlation_id);
+ "Core client received msg c=%p rc=%p cid=%s", //
+ (void *) client, req->req_context, req->correlation_id);
qd_hash_remove_by_handle(client->correlations, req->hash_handle);
qd_hash_handle_free(req->hash_handle);
@@ -578,8 +577,8 @@
{
qdrc_client_t *client = (qdrc_client_t *)client_context;
- qd_log(client->core->log, QD_LOG_TRACE,
- "Core client sender detached c=%p", client);
+ qd_log(client->core->log, QD_LOG_TRACE, //
+ "Core client sender detached c=%p", (void *) client);
if (client->sender_up) {
client->sender_up = false;
@@ -611,8 +610,8 @@
{
qdrc_client_t *client = (qdrc_client_t *)client_context;
- qd_log(client->core->log, QD_LOG_TRACE,
- "Core client receiver detached c=%p", client);
+ qd_log(client->core->log, QD_LOG_TRACE, //
+ "Core client receiver detached c=%p", (void *) client);
if (client->receiver_up) {
client->receiver_up = false;
diff --git a/src/router_core/modules/test_hooks/core_test_hooks.c b/src/router_core/modules/test_hooks/core_test_hooks.c
index d6cba2d..5a68b6a 100644
--- a/src/router_core/modules/test_hooks/core_test_hooks.c
+++ b/src/router_core/modules/test_hooks/core_test_hooks.c
@@ -651,7 +651,7 @@
_on_conn_event,
0, 0, 0, tc);
- qd_log(test_module->core->log, QD_LOG_TRACE, "client test registered %p", tc->conn_events);
+ qd_log(test_module->core->log, QD_LOG_TRACE, "client test registered %p", (void *) tc->conn_events);
}
diff --git a/src/server.c b/src/server.c
index 807fe78..c95e43c 100644
--- a/src/server.c
+++ b/src/server.c
@@ -633,6 +633,8 @@
/* Log the description, set the transport condition (name, description) close the transport tail. */
void connect_fail(qd_connection_t *ctx, const char *name, const char *description, ...)
+ __attribute__((format(printf, 3, 4)));
+void connect_fail(qd_connection_t *ctx, const char *name, const char *description, ...)
{
va_list ap;
va_start(ap, description);
@@ -655,7 +657,6 @@
}
}
-
/* Get the host IP address for the remote end */
static void set_rhost_port(qd_connection_t *ctx) {
pn_transport_t *tport = pn_connection_transport(ctx->pn_conn);
@@ -1081,11 +1082,11 @@
pn_condition_get_name(condition), pn_condition_get_description(condition));
strcpy(ctx->connector->conn_msg, conn_msg);
- qd_log(qd_server->log_source, QD_LOG_INFO, conn_msg);
+ qd_log(qd_server->log_source, QD_LOG_INFO, "%s", conn_msg);
} else {
qd_format_string(conn_msg, 300, "[C%"PRIu64"] Connection to %s failed", ctx->connection_id, config->host_port);
strcpy(ctx->connector->conn_msg, conn_msg);
- qd_log(qd_server->log_source, QD_LOG_INFO, conn_msg);
+ qd_log(qd_server->log_source, QD_LOG_INFO, "%s", conn_msg);
}
} else if (ctx && ctx->listener) { /* Incoming connection */
if (condition && pn_condition_is_set(condition)) {
diff --git a/src/terminus_private.h b/src/terminus_private.h
index 8757ea3..47fb178 100644
--- a/src/terminus_private.h
+++ b/src/terminus_private.h
@@ -28,7 +28,10 @@
// do pointer & length arithmetic without overflowing the destination buffer in
// qdr_terminus_format()
//
-static inline size_t safe_snprintf(char *str, size_t size, const char *format, ...) {
+static inline size_t safe_snprintf(char *str, size_t size, const char *format, ...)
+ __attribute__((format(printf, 3, 4)));
+size_t safe_snprintf(char *str, size_t size, const char *format, ...)
+{
// max size allowed must be INT_MAX (since vsnprintf returns an int)
if (size == 0 || size > INT_MAX) {
return 0;
diff --git a/tests/c_unittests/test_terminus.cpp b/tests/c_unittests/test_terminus.cpp
index 0e5057b..494d6fc 100644
--- a/tests/c_unittests/test_terminus.cpp
+++ b/tests/c_unittests/test_terminus.cpp
@@ -49,37 +49,37 @@
SUBCASE("valid_inputs") {
SUBCASE("") {
- len = safe_snprintf(output, LEN + 10, TEST_MESSAGE);
+ len = safe_snprintf(output, LEN + 10, "%s", TEST_MESSAGE);
CHECK(LEN == len);
CHECK(output == TEST_MESSAGE);
}
SUBCASE("") {
- len = safe_snprintf(output, LEN + 1, TEST_MESSAGE);
+ len = safe_snprintf(output, LEN + 1, "%s", TEST_MESSAGE);
CHECK(LEN == len);
CHECK(output == TEST_MESSAGE);
}
SUBCASE("") {
- len = safe_snprintf(output, LEN, TEST_MESSAGE);
+ len = safe_snprintf(output, LEN, "%s", TEST_MESSAGE);
CHECK(LEN - 1 == len);
CHECK(output == "somethin");
}
SUBCASE("") {
- len = safe_snprintf(output, 0, TEST_MESSAGE);
+ len = safe_snprintf(output, 0, "%s", TEST_MESSAGE);
CHECK(0 == len);
}
SUBCASE("") {
output[0] = 'a';
- len = safe_snprintf(output, 1, TEST_MESSAGE);
+ len = safe_snprintf(output, 1, "%s", TEST_MESSAGE);
CHECK(0 == len);
CHECK('\0' == output[0]);
}
SUBCASE("") {
- len = safe_snprintf(output, (int)-1, TEST_MESSAGE);
+ len = safe_snprintf(output, (int) -1, "%s", TEST_MESSAGE);
CHECK(0 == len);
}
}
@@ -90,7 +90,7 @@
vsnprintf_stub::rc = -1;
output[0] = 'a';
- len = safe_snprintf(output, LEN + 10, TEST_MESSAGE);
+ len = safe_snprintf(output, LEN + 10, "%s", TEST_MESSAGE);
CHECK(0 == len);
CHECK('\0' == output[0]);
}
