Small fixes to run with gcc 10.2.1 on Debian Sid

[johnaman]

These fixes allow project to build to completion and filemq_selftest to run successfully

[ferdnyc]

I'm always a little wary of one magic number being replaced by another magic number... is there any testable property that actually governs the maximum length of the log_prefix? Should one be added, perhaps even as a #define somewhere, so that the code can use log_prefix [MAX_PREFIX_LEN] or similar?

The comment @ lines 259-260 claims the string is truncated to "~20 characters":

filemq2/src/fmq_server_engine.inc

Lines 259 to 260 in e9732d1

	// Set log file prefix; this string will be added to log data, to make
	// log data more searchable. The string is truncated to ~20 chars.

...which makes 41 seem like severe overkill already, to say nothing of 48.

Is the comment lying? (If so, it has no business being there.)

Perhaps the code should truncate the string to MAX_PREFIX_LEN-1 characters, to prevent any possibility of buffer overflows in the future, when someone inevitably changes one value but not the other. (They're very far apart!)

[johnaman]

For the sake of completeness, here is the reply that I emailed:
As I recall, I was definitely getting overflows. A quick grep reveals that there seems to be no truncation occurring within filemq2:

$ grep log_prefix *.c *.inc *.h
fmq_server_engine.inc: char log_prefix; // Default log prefix
fmq_server_engine.inc: char log_prefix [41]; // Log prefix string
fmq_server_engine.inc:engine_set_log_prefix (client_t client, const char string)
fmq_server_engine.inc: snprintf (self->log_prefix, sizeof (self->log_prefix) - 1,
fmq_server_engine.inc: engine_set_log_prefix (NULL, NULL);
fmq_server_engine.inc: engine_set_log_prefix (&self->client, server->log_prefix);
fmq_server_engine.inc: engine_set_log_prefix (&self->client, " TERMINATED ***");
fmq_server_engine.inc: self->log_prefix, s_state_name [self->state]);
fmq_server_engine.inc: self->log_prefix, s_event_name [self->event]);
fmq_server_engine.inc: self->log_prefix);
fmq_server_engine.inc: zsys_debug ("%s: Send message to client", self->log_prefix);
fmq_server_engine.inc: zsys_debug ("%s: $ terminate", self->log_prefix);
fmq_server_engine.inc: self->log_prefix);
fmq_server_engine.inc: zsys_debug ("%s: Send message to client", self->log_prefix);
fmq_server_engine.inc: zsys_debug ("%s: $ terminate", self->log_prefix);
fmq_server_engine.inc: zsys_debug ("%s: $ store client subscription", self->log_prefix);
fmq_server_engine.inc: self->log_prefix);
fmq_server_engine.inc: zsys_debug ("%s: Send message to client", self->log_prefix);
fmq_server_engine.inc: zsys_debug ("%s: $ check for client data", self->log_prefix);
fmq_server_engine.inc: zsys_debug ("%s: $ store client credit", self->log_prefix);
fmq_server_engine.inc: zsys_debug ("%s: $ check for client data", self->log_prefix);
fmq_server_engine.inc: zsys_debug ("%s: $ check for client data", self->log_prefix);
fmq_server_engine.inc: self->log_prefix);
fmq_server_engine.inc: zsys_debug ("%s: Send message to client", self->log_prefix);
fmq_server_engine.inc: zsys_debug ("%s: $ terminate", self->log_prefix);
fmq_server_engine.inc: self->log_prefix);
fmq_server_engine.inc: zsys_debug ("%s: Send message to client", self->log_prefix);
fmq_server_engine.inc: zsys_debug ("%s: $ terminate", self->log_prefix);
fmq_server_engine.inc: self->log_prefix);
fmq_server_engine.inc: zsys_debug ("%s: Send message to client", self->log_prefix);
fmq_server_engine.inc: zsys_debug ("%s: $ terminate", self->log_prefix);
fmq_server_engine.inc: zsys_debug ("%s: $ get next patch for client", self->log_prefix);
fmq_server_engine.inc: self->log_prefix);
fmq_server_engine.inc: zsys_debug ("%s: Send message to client", self->log_prefix);
fmq_server_engine.inc: zsys_debug ("%s: $ check for client data", self->log_prefix);
fmq_server_engine.inc: zsys_debug ("%s: $ get next patch for client", self->log_prefix);
fmq_server_engine.inc: self->log_prefix);
fmq_server_engine.inc: zsys_debug ("%s: Send message to client", self->log_prefix);
fmq_server_engine.inc: zsys_debug ("%s: $ check for client data", self->log_prefix);
fmq_server_engine.inc: zsys_debug ("%s: $ handle client no credit", self->log_prefix);
fmq_server_engine.inc: zsys_debug ("%s: $ handle client finished", self->log_prefix);
fmq_server_engine.inc: zsys_debug ("%s: $ store client credit", self->log_prefix);
fmq_server_engine.inc: zsys_debug ("%s: $ check for client data", self->log_prefix);
fmq_server_engine.inc: self->log_prefix);
fmq_server_engine.inc: zsys_debug ("%s: Send message to client", self->log_prefix);
fmq_server_engine.inc: zsys_debug ("%s: $ terminate", self->log_prefix);
fmq_server_engine.inc: self->log_prefix);
fmq_server_engine.inc: zsys_debug ("%s: Send message to client", self->log_prefix);
fmq_server_engine.inc: zsys_debug ("%s: $ terminate", self->log_prefix);
fmq_server_engine.inc: self->log_prefix);
fmq_server_engine.inc: zsys_debug ("%s: Send message to client", self->log_prefix);
fmq_server_engine.inc: zsys_debug ("%s: $ terminate", self->log_prefix);
fmq_server_engine.inc: zsys_debug ("%s: $ terminate", self->log_prefix);
fmq_server_engine.inc: self->log_prefix);
fmq_server_engine.inc: zsys_debug ("%s: Send message to client", self->log_prefix);
fmq_server_engine.inc: zsys_debug ("%s: $ terminate", self->log_prefix);
fmq_server_engine.inc: self->log_prefix, s_event_name [self->exception]);
fmq_server_engine.inc: self->log_prefix, s_state_name [self->state]);
fmq_server_engine.inc: zsys_debug ("%s: API command=%s", self->log_prefix, method);
fmq_server_engine.inc: self->log_prefix = args? (char *) args: "";

nor does this function (fmq_server_engine.inc):

static void
engine_set_log_prefix (client_t *client, const char *string)
{
if (client) {
s_client_t *self = (s_client_t *) client;
snprintf (self->log_prefix, sizeof (self->log_prefix) - 1,
"%6d:%-33s", self->unique_id, string);
}
}

And a grep for zmq_debug in zmq itself shows many messages longer than 48.

Is the comment lying? (If so, it has no business being there.)

I believe it is.

[ferdnyc]

Is the comment lying? (If so, it has no business being there.)

I believe it is.

I think so too. Though on the face of it, the value of 41 is correct, since 6+1(the colon)+33 == 40, and the truncation is being attempted in the snprintf() call since that's the width of the buffer.

The problem is, someone misunderstood or misused snprintf() — its second argument is the length of the output string including the trailing null byte. But the code is using sizeof() - 1, which reduces it to 40, making the snprintf() potentially one character too wide. And string truncation is done by setting the precision of the field, not the width. %-33s will right-pad short strings with spaces (why?), but do nothing to truncate long strings.

The fix is to just change that snprintf() call to:

snprintf (self->log_prefix, sizeof (self->log_prefix),
          "%6d:%.33s", self->unique_id, string);

That'll use up to the full buffer size of 41 bytes (including \0) without needlessly padding, and will truncate any messages longer than that to 40 characters (plus \0) to ensure there are no overflows.

[ferdnyc]

(If for some reason the right-padding for short strings is necessary, then %-33.33s would both pad and truncate.)