[systemd-devel] [PATCH 2/2] journal: get rid of LINE_MAX in sd_journal_printv*()
Zbigniew Jędrzejewski-Szmek
zbyszek at in.waw.pl
Mon Apr 15 12:29:09 PDT 2013
On Mon, Apr 15, 2013 at 06:41:52PM +0200, harald at redhat.com wrote:
> From: Harald Hoyer <harald at redhat.com>
>
> use stack_size_guess() to get rid of LINE_MAX
> ---
> src/journal/journal-send.c | 99 +++++++++++++++++++++++++++++++----------
> src/journal/test-journal-send.c | 92 +++++++++++++++++++++++++++++++++-----
> 2 files changed, 156 insertions(+), 35 deletions(-)
>
> diff --git a/src/journal/journal-send.c b/src/journal/journal-send.c
> index 4b9109a..2ff04b4 100644
> --- a/src/journal/journal-send.c
> +++ b/src/journal/journal-send.c
> @@ -26,6 +26,7 @@
> #include <unistd.h>
> #include <fcntl.h>
> #include <printf.h>
> +#include <sys/resource.h>
>
> #define SD_JOURNAL_SUPPRESS_LOCATION
>
> @@ -35,15 +36,17 @@
>
> #define SNDBUF_SIZE (8*1024*1024)
>
> -#define ALLOCA_CODE_FUNC(f, func) \
> - do { \
> - size_t _fl; \
> - const char *_func = (func); \
> - char **_f = &(f); \
> - _fl = strlen(_func) + 1; \
> - *_f = alloca(_fl + 10); \
> - memcpy(*_f, "CODE_FUNC=", 10); \
> - memcpy(*_f + 10, _func, _fl); \
> +#define ALLOCA_CODE_FUNC(f, func) \
> + do { \
> + size_t _fl; \
> + const char *_func = (func); \
> + char **_f = &(f); \
> + _fl = strlen(_func) + 1; \
> + if (stack_size_guess() < (_fl + 10)) \
Can't _fl + 10 overflow (on 32 bit)?
> + return -ENOMEM; \
> + *_f = alloca(_fl + 10); \
> + memcpy(*_f, "CODE_FUNC=", 10); \
> + memcpy(*_f + 10, _func, _fl); \
> } while(false)
>
> /* We open a single fd, and we'll share it with the current process,
> @@ -85,11 +88,9 @@ _public_ int sd_journal_print(int priority, const char *format, ...) {
> }
>
> _public_ int sd_journal_printv(int priority, const char *format, va_list ap) {
> -
> - /* FIXME: Instead of limiting things to LINE_MAX we could do a
> - C99 variable-length array on the stack here in a loop. */
> -
> - char buffer[8 + LINE_MAX], p[11]; struct iovec iov[2];
> + char p[11], *buffer; struct iovec iov[2];
> + int len = 0;
> + rlim_t sts = RLIM_INFINITY;
>
> if (priority < 0 || priority > 7)
> return -EINVAL;
> @@ -97,12 +98,37 @@ _public_ int sd_journal_printv(int priority, const char *format, va_list ap) {
> if (!format)
> return -EINVAL;
>
> - snprintf(p, sizeof(p), "PRIORITY=%i", priority & LOG_PRIMASK);
> - char_array_0(p);
> + {
> + va_list aq;
> + va_copy(aq, ap);
> + len = vsnprintf(NULL, 0, format, aq);
> + va_end(aq);
> + }
> + if (len <= 0)
> + return -EINVAL;
> +
> + sts = stack_size_guess();
> +
> + /* Limit the maximum size to the send buffer minus some
> + bytes for the rest
> + */
> + if (sts == RLIM_INFINITY)
> + sts = SNDBUF_SIZE - 1024;
> +
> + /* Spare at least one page_size of stack */
> + if (sts < (unsigned)(len + page_size()))
> + len = sts - page_size();
> +
> + if (len < 0)
> + return -ENOMEM;
> +
> + buffer = alloca(len + 9);
>
> memcpy(buffer, "MESSAGE=", 8);
> - vsnprintf(buffer+8, sizeof(buffer) - 8, format, ap);
> - char_array_0(buffer);
> + vsnprintf(buffer+8, len+1, format, ap);
> +
> + snprintf(p, sizeof(p), "PRIORITY=%i", priority & LOG_PRIMASK);
> + char_array_0(p);
>
> zero(iov);
> IOVEC_SET_STRING(iov[0], buffer);
> @@ -478,9 +504,11 @@ _public_ int sd_journal_print_with_location(int priority, const char *file, cons
> }
>
> _public_ int sd_journal_printv_with_location(int priority, const char *file, const char *line, const char *func, const char *format, va_list ap) {
> - char buffer[8 + LINE_MAX], p[11];
> + char p[11], *buffer = NULL;
> struct iovec iov[5];
> char *f;
> + int len = 0;
> + rlim_t sts = RLIM_INFINITY;
>
> if (priority < 0 || priority > 7)
> return -EINVAL;
> @@ -488,12 +516,37 @@ _public_ int sd_journal_printv_with_location(int priority, const char *file, con
> if (_unlikely_(!format))
> return -EINVAL;
>
> - snprintf(p, sizeof(p), "PRIORITY=%i", priority & LOG_PRIMASK);
> - char_array_0(p);
> + {
> + va_list aq;
> + va_copy(aq, ap);
> + len = vsnprintf(NULL, 0, format, aq);
> + va_end(aq);
> + }
> + if (len <= 0)
> + return -EINVAL;
> +
> + sts = stack_size_guess();
> +
> + /* Limit the maximum size to the send buffer minus some
> + bytes for the rest
> + */
> + if (sts == RLIM_INFINITY)
> + sts = SNDBUF_SIZE - 1024;
> +
> + /* Spare at least one page_size of stack */
> + if (sts < (unsigned)(len + page_size()))
> + len = sts - page_size();
> +
> + if (len < 0)
> + return -ENOMEM;
> +
> + buffer = alloca(len + 9);
>
> memcpy(buffer, "MESSAGE=", 8);
> - vsnprintf(buffer+8, sizeof(buffer) - 8, format, ap);
> - char_array_0(buffer);
> + vsnprintf(buffer+8, len+1, format, ap);
> +
> + snprintf(p, sizeof(p), "PRIORITY=%i", priority & LOG_PRIMASK);
> + char_array_0(p);
This part is from the original code, but it's ugly. We *know*
that the output must fit, so I think it would be better
to say
assert_se(snprintf(p, sizeof(p), ...) < sizeof(p));
Zbyszek
> /* func is initialized from __func__ which is not a macro, but
> * a static const char[], hence cannot easily be prefixed with
> diff --git a/src/journal/test-journal-send.c b/src/journal/test-journal-send.c
> index 3e986ed..b2ff422 100644
> --- a/src/journal/test-journal-send.c
> +++ b/src/journal/test-journal-send.c
> @@ -22,34 +22,66 @@
> #include <systemd/sd-journal.h>
> #include <stdlib.h>
> #include <unistd.h>
> -
> +#include <stdio.h>
> +#include <sys/resource.h>
> #include "log.h"
>
> +#define HUGE_SIZE (4096*1024)
> +
> +static int huge_message_send(void)
> +{
> + char *huge;
> + int r;
> + int asz = random() % (long int)sysconf(_SC_PAGESIZE);
> + huge = alloca(asz);
> + memset(huge, 'x', asz-1);
> + huge[asz-1] = 0;
> + r = sd_journal_print(LOG_NOTICE, "huge_message_send: %s", huge);
> + if (r)
> + return r;
> +
> + r = huge_message_send();
> + return r;
> +}
> +
> +
> int main(int argc, char *argv[]) {
> - char huge[4096*1024];
> + int r;
> + char huge[HUGE_SIZE];
> + struct rlimit rlim;
>
> log_set_max_level(LOG_DEBUG);
>
> - sd_journal_print(LOG_INFO, "piepapo");
> + r = sd_journal_print(LOG_INFO, "piepapo");
> + if (r != 0)
> + return 1;
>
> - sd_journal_send("MESSAGE=foobar",
> + r = sd_journal_send("MESSAGE=foobar",
> "VALUE=%i", 7,
> NULL);
> + if (r != 0)
> + return 1;
>
> errno = ENOENT;
> - sd_journal_perror("Foobar");
> + r = sd_journal_perror("Foobar");
> + if (r != 0)
> + return 1;
>
> - sd_journal_perror("");
> + r = sd_journal_perror("");
> + if (r != 0)
> + return 1;
>
> memset(huge, 'x', sizeof(huge));
> memcpy(huge, "HUGE=", 5);
> char_array_0(huge);
>
> - sd_journal_send("MESSAGE=Huge field attached",
> + r = sd_journal_send("MESSAGE=Huge field attached",
> huge,
> NULL);
> + if (r != 0)
> + return 1;
>
> - sd_journal_send("MESSAGE=uiui",
> + r = sd_journal_send("MESSAGE=uiui",
> "VALUE=A",
> "VALUE=B",
> "VALUE=C",
> @@ -58,12 +90,16 @@ int main(int argc, char *argv[]) {
> "OTHERVALUE=Y",
> "WITH_BINARY=this is a binary value \a",
> NULL);
> + if (r != 0)
> + return 1;
>
> syslog(LOG_NOTICE, "Hello World!");
>
> - sd_journal_print(LOG_NOTICE, "Hello World");
> + r = sd_journal_print(LOG_NOTICE, "Hello World!");
> + if (r != 0)
> + return 1;
>
> - sd_journal_send("MESSAGE=Hello World!",
> + r = sd_journal_send("MESSAGE=Hello World!",
> "MESSAGE_ID=52fb62f99e2c49d89cfbf9d6de5e3555",
> "PRIORITY=5",
> "HOME=%s", getenv("HOME"),
> @@ -71,8 +107,40 @@ int main(int argc, char *argv[]) {
> "PAGE_SIZE=%li", sysconf(_SC_PAGESIZE),
> "N_CPUS=%li", sysconf(_SC_NPROCESSORS_ONLN),
> NULL);
> -
> - sleep(10);
> + if (r != 0)
> + return 1;
> +
> + r = sd_journal_print(LOG_NOTICE, "huge string: %s", huge);
> + if (r != 0)
> + return 1;
> +
> + r = getrlimit(RLIMIT_STACK, &rlim);
> + if (r) {
> + perror("getrlimit");
> + return 1;
> + }
> +
> + rlim.rlim_cur = (rlim_t)sysconf(_SC_PAGESIZE) * 100;
> + r = setrlimit(RLIMIT_STACK, &rlim);
> + if (r) {
> + perror("setrlimit");
> + return 1;
> + }
> +
> + r = sd_journal_print(LOG_NOTICE, "huge string: %s", huge);
> + if (r != -ENOMEM)
> + return 1;
> +
> + rlim.rlim_cur = (rlim_t)sysconf(_SC_PAGESIZE) * 100 + HUGE_SIZE;
> + r = setrlimit(RLIMIT_STACK, &rlim);
> + if (r) {
> + perror("setrlimit");
> + return 1;
> + }
> +
> + r = huge_message_send();
> + if (r != -ENOMEM)
> + return 1;
>
> return 0;
> }
> --
> 1.8.2
>
> _______________________________________________
> systemd-devel mailing list
> systemd-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
>
More information about the systemd-devel
mailing list