PROTON-2714: add annotations for printf-like format strings and fix linux compiler warnings #397

jiridanek · 2023-04-19T16:39:42Z

No description provided.

jiridanek · 2023-04-19T21:38:34Z

c/src/core/connection_driver.c

@@ -43,7 +43,7 @@ static pn_event_t *batch_next(pn_connection_driver_t *d) {
  /* Log the next event that will be processed */
  pn_event_t *next = pn_collector_next(d->collector);
  if (next && PN_SHOULD_LOG(&d->transport->logger, PN_SUBSYSTEM_EVENT, PN_LEVEL_DEBUG)) {
-    pni_logger_log_msg_inspect(&d->transport->logger, PN_SUBSYSTEM_EVENT, PN_LEVEL_DEBUG, next, "");
+    pni_logger_log_msg_inspect(&d->transport->logger, PN_SUBSYSTEM_EVENT, PN_LEVEL_DEBUG, next, "%s", "");


there is only this one line where the annotations lead to siliness needed not to generate a warning (empty formatting string on gcc generates warning)

jiridanek · 2023-04-19T21:39:14Z

c/src/ssl/openssl.c

@@ -1034,7 +1037,7 @@ static ssize_t process_input_ssl( pn_transport_t *transport, unsigned int layer,
    transport->present_layers |= LAYER_SSL;
  }

-  ssl_log( transport, PN_LEVEL_TRACE, "process_input_ssl( data size=%d )",available );
+  ssl_log( transport, PN_LEVEL_TRACE, "process_input_ssl( data size=%zu )",available );


this may rather need the portable format string macros, not sure if necessary

jiridanek · 2023-04-19T21:39:32Z

c/src/core/logger.c

@@ -201,7 +201,7 @@ void pni_logger_log_raw(pn_logger_t *logger, pn_log_subsystem_t subsystem, pn_lo
  const char *start = &bytes.start[bytes.size-size];
  for (unsigned i = 0; i < size; i+=16) {
    pn_fixed_string_t out = pn_fixed_string(buf, sizeof(buf));
-    pn_fixed_string_addf(&out, "%04x/%04x: ", i, size);
+    pn_fixed_string_addf(&out, "%04x/%04zx: ", i, size);


this may rather need the portable format string macros, not sure if necessary

jiridanek · 2023-04-19T22:16:38Z

It looks that msvc wants its annotation on both declaration and definition,

C:\Users\jdanek\Source\Repos\jiridanek\qpid-proton\c\src\core\object\string.c(197) : warning C28251: Inconsistent annotation for 'pn_string_format': this instance has no annotations. See c:\users\jdanek\source\repos\jiridanek\qpid-proton\c\src\core\object_private.h(187).

edit: https://learn.microsoft.com/en-us/cpp/code-quality/c28251?view=msvc-170

jiridanek · 2023-04-19T22:23:04Z

But it seems to be good enough to produce reports

C:\Users\jdanek\Source\Repos\jiridanek\qpid-proton\c\src\ssl\schannel.cpp(997) : warning C6328: Size mismatch: 'unsigned __int64' passed as _Param_(4) when 'int' is required in call to 'ssl_log'.
C:\Users\jdanek\Source\Repos\jiridanek\qpid-proton\c\src\ssl\schannel.cpp(1100) : warning C6328: Size mismatch: 'unsigned __int64' passed as _Param_(4) when 'int' is required in call to 'ssl_log'.
C:\Users\jdanek\Source\Repos\jiridanek\qpid-proton\c\src\ssl\schannel.cpp(1166) : warning C6328: Size mismatch: 'unsigned __int64' passed as _Param_(4) when 'int' is required in call to 'ssl_log'.
C:\Users\jdanek\Source\Repos\jiridanek\qpid-proton\c\src\ssl\schannel.cpp(1177) : warning C6328: Size mismatch: 'unsigned __int64' passed as _Param_(4) when 'int' is required in call to 'ssl_log'.
C:\Users\jdanek\Source\Repos\jiridanek\qpid-proton\c\src\ssl\schannel.cpp(1226) : warning C6328: Size mismatch: 'unsigned __int64' passed as _Param_(4) when 'int' is required in call to 'ssl_log'.
C:\Users\jdanek\Source\Repos\jiridanek\qpid-proton\c\src\ssl\schannel.cpp(1335) : warning C6328: Size mismatch: 'unsigned __int64' passed as _Param_(4) when 'int' is required in call to 'ssl_log'.
C:\Users\jdanek\Source\Repos\jiridanek\qpid-proton\c\src\ssl\schannel.cpp(1375) : warning C6328: Size mismatch: 'unsigned __int64' passed as _Param_(4) when 'int' is required in call to 'ssl_log'.
C:\Users\jdanek\Source\Repos\jiridanek\qpid-proton\c\src\ssl\schannel.cpp(1398) : warning C6328: Size mismatch: 'unsigned __int64' passed as _Param_(4) when 'int' is required in call to 'ssl_log'.
C:\Users\jdanek\Source\Repos\jiridanek\qpid-proton\c\src\ssl\schannel.cpp(1432) : warning C6328: Size mismatch: 'unsigned __int64' passed as _Param_(4) when 'int' is required in call to 'ssl_log'.
C:\Users\jdanek\Source\Repos\jiridanek\qpid-proton\c\src\ssl\schannel.cpp(1627) : warning C6328: Size mismatch: 'unsigned __int64' passed as _Param_(4) when 'int' is required in call to 'ssl_log'.
C:\Users\jdanek\Source\Repos\jiridanek\qpid-proton\c\src\ssl\schannel.cpp(1704) : warning C6328: Size mismatch: 'unsigned __int64' passed as _Param_(4) when 'int' is required in call to 'ssl_log'.
C:\Users\jdanek\Source\Repos\jiridanek\qpid-proton\c\src\ssl\schannel.cpp(1769) : warning C6328: Size mismatch: 'unsigned __int64' passed as _Param_(4) when 'int' is required in call to 'ssl_log'.
C:\Users\jdanek\Source\Repos\jiridanek\qpid-proton\c\src\ssl\schannel.cpp(1808) : warning C6328: Size mismatch: '__int64' passed as _Param_(4) when 'int' is required in call to 'ssl_log'.
C:\Users\jdanek\Source\Repos\jiridanek\qpid-proton\c\src\reactor\io\windows\iocp.c(1168) : warning C6328: Size mismatch: 'unsigned __int64' passed as _Param_(2) when 'int' is required in call to 'iocp_log'.

astitcher · 2023-04-20T13:39:52Z

I feel that we had these annotations in the original proton code base - I wonder where they went?
In any case I think this is a good direction - but after the 0.39 release!

jiridanek · 2023-04-20T13:48:35Z

I feel that we had these annotations in the original proton code base - I wonder where they went?

They are still there, but only applied to two of the functions. I'm rewriting those here to use the macros from Facebook.

In any case I think this is a good direction - but after the 0.39 release!

Sure, I only decided to raise the PR now because otherwise I might forget to do it after the release. It found two broken format strings on Linux and quite a lot on Windows, but I guess this is not a release blocker. All about size_t printing; did not investigate, but it looks like somebody was switching to larger type and missed formatting strings while making the change?

c/include/proton/annotations.h

jiridanek · 2023-04-20T13:52:03Z

c/include/proton/annotations.h

+
+// warn format placeholders
+
+// NOTE: this will only do checking in msvc with versions that support /analyze


It only seems to check when one actually uses /analyze, using the highest tier of latest visual studio does not show any warnings from this without that option. Neither /W4 helps any.

and the checks get lost in the noise very easily

jiridanek · 2023-10-14T12:30:28Z

Sorry for creating the duplicate, I did not remember I already created a PR for this. It's actually good, since I already have it (mostly) reviewed, and the time has come preparing for next release, so it is a good time to do this.

jiridanek force-pushed the jd_2023_04_19_annotate_formatters branch from 367decd to 03a5c0b Compare April 19, 2023 21:15

jiridanek changed the title ~~PROTON-xxxx: add compiler function and parameter safety annotations~~ PROTON-2714: add annotations for printf-like format strings and fix compiler warnings Apr 19, 2023

jiridanek force-pushed the jd_2023_04_19_annotate_formatters branch from 33c6c02 to bfcf355 Compare April 19, 2023 21:27

jiridanek commented Apr 19, 2023

View reviewed changes

jiridanek requested a review from astitcher April 19, 2023 22:26

jiridanek marked this pull request as ready for review April 19, 2023 22:26

jiridanek changed the title ~~PROTON-2714: add annotations for printf-like format strings and fix compiler warnings~~ PROTON-2714: add annotations for printf-like format strings and fix linux compiler warnings Apr 19, 2023

jiridanek commented Apr 20, 2023

View reviewed changes

c/include/proton/annotations.h Show resolved Hide resolved

jiridanek commented Apr 20, 2023

View reviewed changes

jiridanek force-pushed the jd_2023_04_19_annotate_formatters branch from 4d9a529 to fb6e82f Compare October 14, 2023 12:20

jiridanek mentioned this pull request Oct 14, 2023

PROTON-2772: add parameter safety annotations for printf format strings #405

Closed

jiridanek added this to the v0.40.0 milestone Oct 14, 2023

jiridanek added 2 commits October 14, 2023 14:25

PROTON-2772: add parameter safety annotations for printf format strings

a321bf9

PROTON-2714: fix wrong printf format specifiers

2b7aa94

jiridanek force-pushed the jd_2023_04_19_annotate_formatters branch from fb6e82f to 2b7aa94 Compare October 14, 2023 12:28

jiridanek merged commit fc98855 into apache:main Oct 14, 2023
3 checks passed

jiridanek deleted the jd_2023_04_19_annotate_formatters branch October 14, 2023 13:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PROTON-2714: add annotations for printf-like format strings and fix linux compiler warnings #397

PROTON-2714: add annotations for printf-like format strings and fix linux compiler warnings #397

jiridanek commented Apr 19, 2023

jiridanek Apr 19, 2023

jiridanek Apr 19, 2023

jiridanek Apr 19, 2023

jiridanek commented Apr 19, 2023 •

edited

Loading

jiridanek commented Apr 19, 2023 •

edited

Loading

astitcher commented Apr 20, 2023

jiridanek commented Apr 20, 2023

jiridanek Apr 20, 2023

jiridanek Apr 20, 2023

jiridanek commented Oct 14, 2023


		// warn format placeholders

		// NOTE: this will only do checking in msvc with versions that support /analyze

PROTON-2714: add annotations for printf-like format strings and fix linux compiler warnings #397

PROTON-2714: add annotations for printf-like format strings and fix linux compiler warnings #397

Conversation

jiridanek commented Apr 19, 2023

jiridanek Apr 19, 2023

Choose a reason for hiding this comment

jiridanek Apr 19, 2023

Choose a reason for hiding this comment

jiridanek Apr 19, 2023

Choose a reason for hiding this comment

jiridanek commented Apr 19, 2023 • edited Loading

jiridanek commented Apr 19, 2023 • edited Loading

astitcher commented Apr 20, 2023

jiridanek commented Apr 20, 2023

jiridanek Apr 20, 2023

Choose a reason for hiding this comment

jiridanek Apr 20, 2023

Choose a reason for hiding this comment

jiridanek commented Oct 14, 2023

jiridanek commented Apr 19, 2023 •

edited

Loading

jiridanek commented Apr 19, 2023 •

edited

Loading