Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Small fixes to run with gcc 10.2.1 on Debian Sid #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Small fixes to run with gcc 10.2.1 on Debian Sid #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions autogen.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,14 @@

# Script to generate all required files from fresh git checkout.

command -v libtool >/dev/null 2>&1
# Debian and Ubuntu do not shipt libtool anymore, but OSX does not ship libtoolize.
command -v libtoolize >/dev/null 2>&1
if [ $? -ne 0 ]; then
echo "autogen.sh: error: could not find libtool. libtool is required to run autogen.sh." 1>&2
exit 1
command -v libtool >/dev/null 2>&1
if [ $? -ne 0 ]; then
echo "autogen.sh: error: could not find libtool. libtool is required to run autogen.sh." 1>&2
exit 1
fi
fi

command -v autoreconf >/dev/null 2>&1
Expand Down
4 changes: 2 additions & 2 deletions src/fmq_client.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -541,7 +541,7 @@ fmq_client_test (bool verbose)
zchunk_destroy (&chunk);
zfile_close (sfile);
zfile_restat (sfile);
char *sdigest = zfile_digest (sfile);
const char *sdigest = zfile_digest (sfile);
assert (sdigest);
zsys_info ("fmq_client_test: Server file digest %s", sdigest);

Expand All @@ -554,7 +554,7 @@ fmq_client_test (bool verbose)
zfile_t *cfile = zfile_new ("./fmqclient", "test_file.txt");
assert (cfile);
zfile_restat (cfile);
char *cdigest = zfile_digest (cfile);
const char *cdigest = zfile_digest (cfile);
assert (cdigest);
zsys_info ("fmq_client_test: Client file digest %s", cdigest);
assert (streq (sdigest, cdigest));
Expand Down
2 changes: 1 addition & 1 deletion src/fmq_server.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ sub_patch_add (sub_t *self, zdir_patch_t *patch)
zsys_debug ("path=%s, op=%d, vpath=%s", zdir_patch_path (patch),
zdir_patch_op (patch), zdir_patch_vpath (patch));
zhash_insert (self->cache,
zdir_patch_digest (patch), zdir_patch_vpath (patch));
zdir_patch_digest (patch), (void *)zdir_patch_vpath (patch));
}

zsys_debug ("+++ adding following patch to client list +++");
Expand Down
2 changes: 1 addition & 1 deletion src/fmq_server_engine.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ typedef struct {
int wakeup; // zloop timer for client alarms
void *ticket; // zloop ticket for client timeouts
event_t wakeup_event; // Wake up with this event
char log_prefix [41]; // Log prefix string
char log_prefix [48]; // Log prefix string
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@ferdnyc ferdnyc Nov 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} s_client_t;

static int
Expand Down