Re: [systemd-devel] [PATCH 3/4] util: add function getting proc environ

[Lennart Poettering]

On Wed, 19.11.14 11:01, Jakub Filak (jfi...@redhat.com) wrote:

>  
> +int get_process_environ(pid_t pid, char **environ) {

If this is really just about pushing this into the journal: the
journal is actually binary safe, we could just drop the data there
without escaping it. That said, it certainly doesn't help readability,
and processability if fields are unnecesarily binary, hence it's
probably a good idea to escape things as your patch does it.

> +_cleanup_fclose_ FILE *f = NULL;
> +_cleanup_free_ char *outcome = NULL;
> +int c;
> +const char *p;
> +char escaped[4];
> +size_t allocated = 0, sz = 0, escaped_len = 0;
> +
> +assert(pid >= 0);
> +assert(environ);
> +
> +p = procfs_file_alloca(pid, "environ");
> +
> +f = fopen(p, "re");
> +if (!f)
> +return -errno;
> +
> +while ((c = fgetc(f)) != EOF) {
> +if (c == '\0') {
> +escaped[0] = '\n';
> +escaped_len = 1;
> +}
> +else
> +escaped_len = cescape_char(c, escaped);
> +
> +if (!GREEDY_REALLOC(outcome, allocated, sz + escaped_len + 
> 1))
> +return -ENOMEM;
> +
> +memcpy(outcome + sz, escaped, escaped_len);
> +sz += escaped_len;
> +}

I'd probably just always prealloc 5 chars more in each iteration and
then use cescape_char() directly on the resulting buffer instead of
copying things into a temporary buffer first, and then reallocating to
the precise length and then copying it over to the real buffer.

It not only makes the code a bit quicker but also is most likely a bit
wiser resource-wise since the extra copy and realloc is usually more
expensive than a few bytes of memory added extra.

Otherwise looks great!

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] [PATCH 3/4] util: add function getting proc environ

[Jakub Filak]

On the contrary of env, the added function returns all characters
cescaped, because it improves reproducibility.
---
 src/shared/util.c| 160 +--
 src/shared/util.h|   1 +
 src/test/test-util.c |   6 +-
 3 files changed, 109 insertions(+), 58 deletions(-)

diff --git a/src/shared/util.c b/src/shared/util.c
index d62d90c..448efa5 100644
--- a/src/shared/util.c
+++ b/src/shared/util.c
@@ -174,6 +174,69 @@ char* first_word(const char *s, const char *word) {
 return (char*) p;
 }
 
+static size_t cescape_char(char c, char *buf) {
+char * buf_old = buf;
+
+switch (c) {
+
+case '\a':
+*(buf++) = '\\';
+*(buf++) = 'a';
+break;
+case '\b':
+*(buf++) = '\\';
+*(buf++) = 'b';
+break;
+case '\f':
+*(buf++) = '\\';
+*(buf++) = 'f';
+break;
+case '\n':
+*(buf++) = '\\';
+*(buf++) = 'n';
+break;
+case '\r':
+*(buf++) = '\\';
+*(buf++) = 'r';
+break;
+case '\t':
+*(buf++) = '\\';
+*(buf++) = 't';
+break;
+case '\v':
+*(buf++) = '\\';
+*(buf++) = 'v';
+break;
+case '\\':
+*(buf++) = '\\';
+*(buf++) = '\\';
+break;
+case '"':
+*(buf++) = '\\';
+*(buf++) = '"';
+break;
+case '\'':
+*(buf++) = '\\';
+*(buf++) = '\'';
+break;
+
+default:
+/* For special chars we prefer octal over
+ * hexadecimal encoding, simply because glib's
+ * g_strescape() does the same */
+if ((c < ' ') || (c >= 127)) {
+*(buf++) = '\\';
+*(buf++) = octchar((unsigned char) c >> 6);
+*(buf++) = octchar((unsigned char) c >> 3);
+*(buf++) = octchar((unsigned char) c);
+} else
+*(buf++) = c;
+break;
+}
+
+return buf - buf_old;
+}
+
 int close_nointr(int fd) {
 assert(fd >= 0);
 
@@ -905,6 +968,45 @@ DEFINE_FN_GET_PROCESS_FULL_FILE(maps)
 DEFINE_FN_GET_PROCESS_FULL_FILE(limits)
 DEFINE_FN_GET_PROCESS_FULL_FILE(cgroup)
 
+int get_process_environ(pid_t pid, char **environ) {
+_cleanup_fclose_ FILE *f = NULL;
+_cleanup_free_ char *outcome = NULL;
+int c;
+const char *p;
+char escaped[4];
+size_t allocated = 0, sz = 0, escaped_len = 0;
+
+assert(pid >= 0);
+assert(environ);
+
+p = procfs_file_alloca(pid, "environ");
+
+f = fopen(p, "re");
+if (!f)
+return -errno;
+
+while ((c = fgetc(f)) != EOF) {
+if (c == '\0') {
+escaped[0] = '\n';
+escaped_len = 1;
+}
+else
+escaped_len = cescape_char(c, escaped);
+
+if (!GREEDY_REALLOC(outcome, allocated, sz + escaped_len + 1))
+return -ENOMEM;
+
+memcpy(outcome + sz, escaped, escaped_len);
+sz += escaped_len;
+}
+
+outcome[sz] = '\0';
+*environ = outcome;
+outcome = NULL;
+
+return 0;
+}
+
 char *strnappend(const char *s, const char *suffix, size_t b) {
 size_t a;
 char *r;
@@ -1284,63 +1386,7 @@ char *cescape(const char *s) {
 return NULL;
 
 for (f = s, t = r; *f; f++)
-
-switch (*f) {
-
-case '\a':
-*(t++) = '\\';
-*(t++) = 'a';
-break;
-case '\b':
-*(t++) = '\\';
-*(t++) = 'b';
-break;
-case '\f':
-*(t++) = '\\';
-*(t++) = 'f';
-break;
-case '\n':
-*(t++) = '\\';
-*(t++) = 'n';
-break;
-case '\r':
-*(t++) = '\\';
-*(t++) = 'r';
-