From: Rasmus Villemoes <linux@rasmusvillemoes.dk> To: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Andrew Morton <akpm@linux-foundation.org>, Trond Myklebust <trond.myklebust@primarydata.com>, "J. Bruce Fields" <bfields@fieldses.org>, "David S. Miller" <davem@davemloft.net>, linux-kernel@vger.kernel.org, linux-nfs@vger.kernel.org, netdev@vger.kernel.org Subject: Re: [PATCH v3 3/3] lib/string_helpers.c: Change semantics of string_escape_mem Date: Mon, 23 Feb 2015 23:55:51 +0100 [thread overview] Message-ID: <87k2z86xvs.fsf@rasmusvillemoes.dk> (raw) In-Reply-To: <1424695820.14897.10.camel@linux.intel.com> (Andy Shevchenko's message of "Mon, 23 Feb 2015 14:50:20 +0200") On Mon, Feb 23 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: >> >> > So, why couldn't we split this to separate test case? It seems I already >> >> > pointed this out. >> >> > >> >> >> >> This actually provides better coverage >> > >> > I do not see much advantage of doing so. You may create a loop with >> > random number for in-size and check. So, I prefer to see separate case >> > for that. >> >> It's not about the size, it's about exercising all the various escape_* >> helpers, to ensure that they all respect the end of the buffer, while >> still returning the correct would-be output size. For that, one needs to >> call string_escape_mem with various combinations of flags and input >> buffers. The logic for that is already in place in test_string_escape >> and its caller, and I see no point in duplicating all that. > > Thanks for clarification. > >> If you insist on a separate function for doing the overflow testing, >> I'll just rip it out from my code and let you add such a test later. > > What about to make it a separate function *and* call from inside of > test_string_escape? Would it work for you? See my earlier point about "quite a lot of state to pass". But if this static __init void test_string_escape_overflow(const char *in, int p, char *out_real, int out_size, unsigned int flags, const char *esc, int q_test, const char *name) { int q_real; memset(out_real, 'Z', out_size); q_real = string_escape_mem(in, p, out_real, 0, flags, esc); if (q_real != q_test) pr_warn("Test '%s' failed: flags = %u, osz = 0, expected %d, got %d\n", name, flags, q_test, q_real); if (memchr_inv(out_real, 'Z', out_size)) pr_warn("Test '%s' failed: osz = 0 but string_escape_mem wrote to the buffer\n", name); } is what you want, sure, have it your way. I need to fix fs/proc/array.c in 3/3 as well, to make the kernel compile+boot and make the series bisectable. Before I send v4 please let me know what you think about this (the minimal fix I could come up with): diff --git a/fs/proc/array.c b/fs/proc/array.c index 1295a00ca316..20f2d50e2dba 100644 --- a/fs/proc/array.c +++ b/fs/proc/array.c @@ -99,10 +99,9 @@ static inline void task_name(struct seq_file *m, struct task_struct *p) buf = m->buf + m->count; /* Ignore error for now */ - string_escape_str(tcomm, &buf, m->size - m->count, - ESCAPE_SPACE | ESCAPE_SPECIAL, "\n\\"); + m->count += string_escape_str(tcomm, buf, m->size - m->count, + ESCAPE_SPACE | ESCAPE_SPECIAL, "\n\\"); - m->count = buf - m->buf; seq_putc(m, '\n'); } [Longer-term I think it would be a lot better not to poke around in the internals of struct seq_file. One way is to do the escaping into a stack buffer (2*sizeof(p->comm) should be enough) and then use something like seq_write(m, buffer, min(sizeof(buffer), return-value-from-string_escape_str)). Another option is to do everything with a single seq_printf call, something like seq_printf(m, "Name:\t%*pEcs\n, (int)strlen(tcomm), tcomm) That will escape more than just \ and \n, but that would IMO be an improvement. But of course this is out of scope for this series.] Rasmus
WARNING: multiple messages have this Message-ID (diff)
From: Rasmus Villemoes <linux-qQsb+v5E8BnlAoU/VqSP6n9LOBIZ5rWg@public.gmane.org> To: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>, Trond Myklebust <trond.myklebust-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org>, "J. Bruce Fields" <bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>, "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org Subject: Re: [PATCH v3 3/3] lib/string_helpers.c: Change semantics of string_escape_mem Date: Mon, 23 Feb 2015 23:55:51 +0100 [thread overview] Message-ID: <87k2z86xvs.fsf@rasmusvillemoes.dk> (raw) In-Reply-To: <1424695820.14897.10.camel-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> (Andy Shevchenko's message of "Mon, 23 Feb 2015 14:50:20 +0200") On Mon, Feb 23 2015, Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote: >> >> > So, why couldn't we split this to separate test case? It seems I already >> >> > pointed this out. >> >> > >> >> >> >> This actually provides better coverage >> > >> > I do not see much advantage of doing so. You may create a loop with >> > random number for in-size and check. So, I prefer to see separate case >> > for that. >> >> It's not about the size, it's about exercising all the various escape_* >> helpers, to ensure that they all respect the end of the buffer, while >> still returning the correct would-be output size. For that, one needs to >> call string_escape_mem with various combinations of flags and input >> buffers. The logic for that is already in place in test_string_escape >> and its caller, and I see no point in duplicating all that. > > Thanks for clarification. > >> If you insist on a separate function for doing the overflow testing, >> I'll just rip it out from my code and let you add such a test later. > > What about to make it a separate function *and* call from inside of > test_string_escape? Would it work for you? See my earlier point about "quite a lot of state to pass". But if this static __init void test_string_escape_overflow(const char *in, int p, char *out_real, int out_size, unsigned int flags, const char *esc, int q_test, const char *name) { int q_real; memset(out_real, 'Z', out_size); q_real = string_escape_mem(in, p, out_real, 0, flags, esc); if (q_real != q_test) pr_warn("Test '%s' failed: flags = %u, osz = 0, expected %d, got %d\n", name, flags, q_test, q_real); if (memchr_inv(out_real, 'Z', out_size)) pr_warn("Test '%s' failed: osz = 0 but string_escape_mem wrote to the buffer\n", name); } is what you want, sure, have it your way. I need to fix fs/proc/array.c in 3/3 as well, to make the kernel compile+boot and make the series bisectable. Before I send v4 please let me know what you think about this (the minimal fix I could come up with): diff --git a/fs/proc/array.c b/fs/proc/array.c index 1295a00ca316..20f2d50e2dba 100644 --- a/fs/proc/array.c +++ b/fs/proc/array.c @@ -99,10 +99,9 @@ static inline void task_name(struct seq_file *m, struct task_struct *p) buf = m->buf + m->count; /* Ignore error for now */ - string_escape_str(tcomm, &buf, m->size - m->count, - ESCAPE_SPACE | ESCAPE_SPECIAL, "\n\\"); + m->count += string_escape_str(tcomm, buf, m->size - m->count, + ESCAPE_SPACE | ESCAPE_SPECIAL, "\n\\"); - m->count = buf - m->buf; seq_putc(m, '\n'); } [Longer-term I think it would be a lot better not to poke around in the internals of struct seq_file. One way is to do the escaping into a stack buffer (2*sizeof(p->comm) should be enough) and then use something like seq_write(m, buffer, min(sizeof(buffer), return-value-from-string_escape_str)). Another option is to do everything with a single seq_printf call, something like seq_printf(m, "Name:\t%*pEcs\n, (int)strlen(tcomm), tcomm) That will escape more than just \ and \n, but that would IMO be an improvement. But of course this is out of scope for this series.] Rasmus -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2015-02-23 22:55 UTC|newest] Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-01-28 13:25 [PATCH 0/2] Two printf fixes Rasmus Villemoes 2015-01-28 13:25 ` [PATCH 1/2] lib/vsprintf.c: Fix potential NULL deref in hex_string Rasmus Villemoes 2015-01-28 14:53 ` Andy Shevchenko 2015-01-28 15:49 ` Rasmus Villemoes 2015-01-28 16:43 ` Andy Shevchenko 2015-01-28 13:25 ` [PATCH 2/2] string_helpers: Change semantics of string_escape_mem Rasmus Villemoes 2015-01-28 15:05 ` Andy Shevchenko 2015-01-29 10:03 ` [PATCH v2 0/3] Two printf fixes Rasmus Villemoes 2015-01-29 10:03 ` [PATCH v2 1/3] lib/vsprintf.c: Fix potential NULL deref in hex_string Rasmus Villemoes 2015-01-29 10:43 ` Andy Shevchenko 2015-01-29 10:03 ` [PATCH v2 2/3] lib/string_helpers.c: Refactor string_escape_mem Rasmus Villemoes 2015-01-29 12:12 ` Andy Shevchenko 2015-01-29 13:10 ` Rasmus Villemoes 2015-01-29 13:37 ` Andy Shevchenko 2015-01-29 19:33 ` Jeff Epler 2015-01-30 10:14 ` Andy Shevchenko 2015-01-29 10:03 ` [PATCH v2 3/3] lib/string_helpers.c: Change semantics of string_escape_mem Rasmus Villemoes 2015-01-29 13:29 ` Andy Shevchenko 2015-01-29 14:29 ` Rasmus Villemoes 2015-01-30 10:27 ` Andy Shevchenko 2015-01-30 23:39 ` Rasmus Villemoes 2015-01-30 23:39 ` Rasmus Villemoes 2015-02-02 10:56 ` Andy Shevchenko 2015-02-09 23:44 ` [PATCH v3 0/3] Two printf fixes Rasmus Villemoes 2015-02-09 23:44 ` Rasmus Villemoes 2015-02-09 23:44 ` [PATCH v3 1/3] lib/vsprintf.c: Fix potential NULL deref in hex_string Rasmus Villemoes 2015-02-09 23:44 ` [PATCH v3 2/3] lib/string_helpers.c: Refactor string_escape_mem Rasmus Villemoes 2015-02-10 12:16 ` Andy Shevchenko 2015-02-09 23:44 ` [PATCH v3 3/3] lib/string_helpers.c: Change semantics of string_escape_mem Rasmus Villemoes 2015-02-09 23:44 ` Rasmus Villemoes 2015-02-10 12:32 ` Andy Shevchenko 2015-02-10 12:32 ` Andy Shevchenko 2015-02-10 13:02 ` Rasmus Villemoes 2015-02-10 14:22 ` Andy Shevchenko 2015-02-10 14:22 ` Andy Shevchenko 2015-02-21 1:35 ` Rasmus Villemoes 2015-02-23 12:50 ` Andy Shevchenko 2015-02-23 12:50 ` Andy Shevchenko 2015-02-23 22:55 ` Rasmus Villemoes [this message] 2015-02-23 22:55 ` Rasmus Villemoes 2015-03-02 12:37 ` Andy Shevchenko 2015-03-02 12:37 ` Andy Shevchenko 2015-03-02 23:03 ` Rasmus Villemoes 2015-03-03 10:26 ` Andy Shevchenko 2015-03-03 10:26 ` Andy Shevchenko 2015-03-03 23:20 ` [PATCH v4 0/3] Two printf fixes Rasmus Villemoes 2015-03-03 23:20 ` [PATCH v4 1/3] lib/vsprintf.c: Fix potential NULL deref in hex_string Rasmus Villemoes 2015-03-03 23:20 ` [PATCH v4 2/3] lib/string_helpers.c: Refactor string_escape_mem Rasmus Villemoes 2015-03-04 10:51 ` Andy Shevchenko 2015-03-03 23:20 ` [PATCH v4 3/3] lib/string_helpers.c: Change semantics of string_escape_mem Rasmus Villemoes 2015-03-04 11:49 ` Andy Shevchenko 2015-03-04 11:49 ` Andy Shevchenko
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=87k2z86xvs.fsf@rasmusvillemoes.dk \ --to=linux@rasmusvillemoes.dk \ --cc=akpm@linux-foundation.org \ --cc=andriy.shevchenko@linux.intel.com \ --cc=bfields@fieldses.org \ --cc=davem@davemloft.net \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-nfs@vger.kernel.org \ --cc=netdev@vger.kernel.org \ --cc=trond.myklebust@primarydata.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.