On Thu, Mar 24, 2011 at 04:51:58PM -0400, Kevin A. McGrail wrote:
> On 3/24/2011 4:30 PM, Frederik Deweerdt wrote:
> >Hi Kevin,
> >
> >On Thu, Mar 24, 2011 at 03:47:25PM -0400, Kevin A. McGrail wrote:
> >>If it were me and this race condition occurred, shouldn't there also
> >>be a log call of some sort inside the m == NULL loop?
> >How could we output something with m->priv->flags unavailable? Would
> >stderr be OK?
> >
> >Regards,
> >Frederik
> Hi Frederik,
>
> I would definitely not use stderr at least but perhaps the call to
> message dump should pass flags from message_process so that flags
> var can be used instead of the message flags. Your thoughts?
That's fine with me, it's pretty low risk because the code in libspamc.c
is immune to the crash anyway.
The NULL deref may only happen in hypothetical out-of-tree users.
The only drawback I see is that it changes the external API. Here's an
updated patch.
Regards,
Frederik
-----
When calling message_dump with a NULL m argument, m is checked against
being NULL before calling message write, but m is derefenced in the
libspamc_log call below.
The patch below returns if 'm' is NULL, and prints an error message.
To avoid having deref'ing 'm' for flags, those are passed to
message_dump()
diff --git a/spamc/libspamc.c b/spamc/libspamc.c
index 12e5048..e8fb1ce 100644
--- a/spamc/libspamc.c
+++ b/spamc/libspamc.c
@@ -848,17 +848,23 @@ long message_write(int fd, struct message *m)
}
}
-void message_dump(int in_fd, int out_fd, struct message *m)
+void message_dump(int in_fd, int out_fd, struct message *m, int flags)
{
char buf[8196];
int bytes;
- if (m != NULL && m->type != MESSAGE_NONE) {
+ if (m == NULL) {
+ libspamc_log(flags, LOG_ERR, "oops! message_dump called with NULL
message");
+ return;
+ }
+
+ if (m->type != MESSAGE_NONE) {
message_write(out_fd, m);
}
+
while ((bytes = full_read(in_fd, 1, buf, 8192, 8192)) > 0) {
if (bytes != full_write(out_fd, 1, buf, bytes)) {
- libspamc_log(m->priv->flags, LOG_ERR, "oops! message_dump of %d
returned different",
+ libspamc_log(flags, LOG_ERR, "oops! message_dump of %d returned
different",
bytes);
}
}
@@ -1564,7 +1570,7 @@ int message_process(struct transport *trans, char
*username, int max_size,
return EX_NOTSPAM;
}
else {
- message_dump(in_fd, out_fd, &m);
+ message_dump(in_fd, out_fd, &m, flags);
message_cleanup(&m);
return ret;
}
diff --git a/spamc/libspamc.h b/spamc/libspamc.h
index 300eb62..1f2afff 100644
--- a/spamc/libspamc.h
+++ b/spamc/libspamc.h
@@ -284,7 +284,7 @@ int message_tell(struct transport *tp, const char
*username, int flags,
* will be MESSAGE_ERROR) it will be message_writed. Then, fd_in will be piped
* to fd_out intol EOF. This is particularly useful if you get back an
* EX_TOOBIG. */
-void message_dump(int in_fd, int out_fd, struct message *m);
+void message_dump(int in_fd, int out_fd, struct message *m, int flags);
/* Do a message_read->message_filter->message_write sequence, handling errors
* appropriately with dump_message or appropriate CHECK_ONLY output. Returns
diff --git a/spamc/spamc.c b/spamc/spamc.c
index ed2fd6d..e609da5 100644
--- a/spamc/spamc.c
+++ b/spamc/spamc.c
@@ -1010,7 +1010,7 @@ main(int argc, char *argv[])
/* bug 5412: spamc -x should not output the message on error */
if ((flags & SPAMC_SAFE_FALLBACK) || result == EX_TOOBIG) {
get_output_fd(&out_fd);
- message_dump(STDIN_FILENO, out_fd, &m);
+ message_dump(STDIN_FILENO, out_fd, &m, flags);
}
/* else, do NOT get_output_fd() (bug 5478) */
}
