From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> To: Rasmus Villemoes <linux@rasmusvillemoes.dk> 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, 02 Mar 2015 14:37:26 +0200 [thread overview] Message-ID: <1425299846.14897.124.camel@linux.intel.com> (raw) In-Reply-To: <87k2z86xvs.fsf@rasmusvillemoes.dk> On Mon, 2015-02-23 at 23:55 +0100, Rasmus Villemoes wrote: > On Mon, Feb 23 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > >> 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. Something like above, though might be few variables can be defined inside it, such as out_real, out_size. > 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\\"); Just a nitpick: what if we keep buf arithmetics in place, i.e. buf += string_escape_str(); m->count = … Also shouldn't we check if seq_overflow is set before even trying to escape? Otherwise it will return something which is bigger that 0 and advance m->count too far. > > - 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.] It should be %pT and reconsider policy how we print task name in different cases (vsprintf.c::comm_name()). -- Andy Shevchenko <andriy.shevchenko@intel.com> Intel Finland Oy
WARNING: multiple messages have this Message-ID (diff)
From: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> To: Rasmus Villemoes <linux-qQsb+v5E8BnlAoU/VqSP6n9LOBIZ5rWg@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, 02 Mar 2015 14:37:26 +0200 [thread overview] Message-ID: <1425299846.14897.124.camel@linux.intel.com> (raw) In-Reply-To: <87k2z86xvs.fsf-qQsb+v5E8BnlAoU/VqSP6n9LOBIZ5rWg@public.gmane.org> On Mon, 2015-02-23 at 23:55 +0100, Rasmus Villemoes wrote: > On Mon, Feb 23 2015, Andy Shevchenko <andriy.shevchenko-VuQAYsv1563A2t4k+OOtxA@public.gmane.orgom> wrote: > >> 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. Something like above, though might be few variables can be defined inside it, such as out_real, out_size. > 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\\"); Just a nitpick: what if we keep buf arithmetics in place, i.e. buf += string_escape_str(); m->count = … Also shouldn't we check if seq_overflow is set before even trying to escape? Otherwise it will return something which is bigger that 0 and advance m->count too far. > > - 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.] It should be %pT and reconsider policy how we print task name in different cases (vsprintf.c::comm_name()). -- Andy Shevchenko <andriy.shevchenko-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> Intel Finland Oy -- 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-03-02 12:37 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 2015-02-23 22:55 ` Rasmus Villemoes 2015-03-02 12:37 ` Andy Shevchenko [this message] 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=1425299846.14897.124.camel@linux.intel.com \ --to=andriy.shevchenko@linux.intel.com \ --cc=akpm@linux-foundation.org \ --cc=bfields@fieldses.org \ --cc=davem@davemloft.net \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-nfs@vger.kernel.org \ --cc=linux@rasmusvillemoes.dk \ --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.