* [PATCH 0/2] Two printf fixes @ 2015-01-28 13:25 Rasmus Villemoes 2015-01-28 13:25 ` [PATCH 1/2] lib/vsprintf.c: Fix potential NULL deref in hex_string Rasmus Villemoes ` (2 more replies) 0 siblings, 3 replies; 52+ messages in thread From: Rasmus Villemoes @ 2015-01-28 13:25 UTC (permalink / raw) To: Andy Shevchenko, Andrew Morton, Trond Myklebust, J. Bruce Fields, David S. Miller Cc: Rasmus Villemoes, linux-kernel, linux-nfs, netdev Both %pE and %ph are unusable in kasprintf(), since the occurrence of either will trigger an oops during the first vsnprintf call where kasprintf tries to find the correct size to allocate. These oopses could be papered over with somewhat smaller patches than these, but then the return value from vsnprintf would still not reflect the actual size needed. For %pE, this requires a change of semantics of string_escape_mem and hence an annoyingly large diffstat. Whether this is 3.20, 3.21 and/or -stable material (or /dev/null material, for that matter ;-)) I'll leave to others to decide. Rasmus Villemoes (2): lib/vsprintf.c: Fix potential NULL deref in hex_string string_helpers: Change semantics of string_escape_mem include/linux/string_helpers.h | 10 +-- lib/string_helpers.c | 195 ++++++++++++++++------------------------- lib/test-string_helpers.c | 37 ++++---- lib/vsprintf.c | 25 ++++-- net/sunrpc/cache.c | 8 +- 5 files changed, 119 insertions(+), 156 deletions(-) -- 2.1.3 ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH 1/2] lib/vsprintf.c: Fix potential NULL deref in hex_string 2015-01-28 13:25 [PATCH 0/2] Two printf fixes Rasmus Villemoes @ 2015-01-28 13:25 ` Rasmus Villemoes 2015-01-28 14:53 ` Andy Shevchenko 2015-01-28 13:25 ` [PATCH 2/2] string_helpers: Change semantics of string_escape_mem Rasmus Villemoes 2015-01-29 10:03 ` [PATCH v2 0/3] Two printf fixes Rasmus Villemoes 2 siblings, 1 reply; 52+ messages in thread From: Rasmus Villemoes @ 2015-01-28 13:25 UTC (permalink / raw) To: Andy Shevchenko, Andrew Morton, Jiri Kosina, Randy Dunlap, Fabian Frederick, Bjorn Helgaas, Ryan Mallon, Masanari Iida Cc: Rasmus Villemoes, linux-kernel The helper hex_string() is broken in two ways. First, it doesn't increment buf regardless of whether there is room to print, so callers such as kasprintf() that try to probe the correct storage to allocate will get a too small return value. But even worse, kasprintf() (and likely anyone else trying to find the size of the result) pass NULL for buf and 0 for size, so we also have end == NULL. But this means that the end-1 in hex_string() is (char*)-1, so buf < end-1 is true and we get a NULL pointer deref. I double-checked this with a trivial kernel module that just did a kasprintf(GFP_KERNEL, "%14ph", "CrashBoomBang"). Nobody seems to be using %ph with kasprintf, but we might as well fix it before it hits someone. Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> --- lib/vsprintf.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/lib/vsprintf.c b/lib/vsprintf.c index ec337f64f52d..0d57be58448f 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -748,11 +748,19 @@ char *resource_string(char *buf, char *end, struct resource *res, return string(buf, end, sym, spec); } +static char* +write_bytes(char *buf, char *end, const char *bytes, unsigned count) +{ + if (buf < end) + memcpy(buf, bytes, min_t(unsigned, end - buf, count)); + return buf + count; +} + static noinline_for_stack char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec, const char *fmt) { - int i, len = 1; /* if we pass '%ph[CDN]', field width remains + int i, j, len = 1; /* if we pass '%ph[CDN]', field width remains negative value, fallback to the default */ char separator; @@ -782,11 +790,16 @@ char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec, if (spec.field_width > 0) len = min_t(int, spec.field_width, 64); - for (i = 0; i < len && buf < end - 1; i++) { - buf = hex_byte_pack(buf, addr[i]); + for (i = 0; i < len; i += 8) { + char tmp[24]; /* 8*2 hex chars + 8 separators */ + char *t = tmp; - if (buf < end && separator && i != len - 1) - *buf++ = separator; + for (j = i; j < len && j < i+8; ++j) { + t = hex_byte_pack(t, addr[j]); + if (separator && j != len-1) + *t++ = separator; + } + buf = write_bytes(buf, end, tmp, t - tmp); } return buf; -- 2.1.3 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH 1/2] lib/vsprintf.c: Fix potential NULL deref in hex_string 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 0 siblings, 1 reply; 52+ messages in thread From: Andy Shevchenko @ 2015-01-28 14:53 UTC (permalink / raw) To: Rasmus Villemoes Cc: Andrew Morton, Jiri Kosina, Randy Dunlap, Fabian Frederick, Bjorn Helgaas, Ryan Mallon, Masanari Iida, linux-kernel On Wed, 2015-01-28 at 14:25 +0100, Rasmus Villemoes wrote: > The helper hex_string() is broken in two ways. First, it doesn't > increment buf regardless of whether there is room to print, so callers > such as kasprintf() that try to probe the correct storage to allocate > will get a too small return value. But even worse, kasprintf() (and > likely anyone else trying to find the size of the result) pass NULL > for buf and 0 for size, so we also have end == NULL. But this means > that the end-1 in hex_string() is (char*)-1, so buf < end-1 is true > and we get a NULL pointer deref. I double-checked this with a trivial > kernel module that just did a kasprintf(GFP_KERNEL, "%14ph", > "CrashBoomBang"). Good catch, though I don't like the implementation of fix. What about the following? diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 8690798..47b36ddd 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -783,11 +783,20 @@ char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec, if (spec.field_width > 0) len = min_t(int, spec.field_width, 64); - for (i = 0; i < len && buf < end - 1; i++) { - buf = hex_byte_pack(buf, addr[i]); + for (i = 0; i < len; i++) { + if (buf < end) + *buf = hex_asc_hi(addr[i]); + ++buf; + + if (buf < end) + *buf = hex_asc_lo(addr[i]); + ++buf; - if (buf < end && separator && i != len - 1) - *buf++ = separator; + if (separator && i != len - 1) { + if (buf < end) + *buf = separator; + ++buf; + } } return buf; > Nobody seems to be using %ph with kasprintf, but we might as well fix > it before it hits someone. -- Andy Shevchenko <andriy.shevchenko@intel.com> Intel Finland Oy ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH 1/2] lib/vsprintf.c: Fix potential NULL deref in hex_string 2015-01-28 14:53 ` Andy Shevchenko @ 2015-01-28 15:49 ` Rasmus Villemoes 2015-01-28 16:43 ` Andy Shevchenko 0 siblings, 1 reply; 52+ messages in thread From: Rasmus Villemoes @ 2015-01-28 15:49 UTC (permalink / raw) To: Andy Shevchenko Cc: Andrew Morton, Jiri Kosina, Randy Dunlap, Fabian Frederick, Bjorn Helgaas, Ryan Mallon, Masanari Iida, linux-kernel On Wed, Jan 28 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Wed, 2015-01-28 at 14:25 +0100, Rasmus Villemoes wrote: >> The helper hex_string() is broken in two ways. First, it doesn't >> increment buf regardless of whether there is room to print, so callers >> such as kasprintf() that try to probe the correct storage to allocate >> will get a too small return value. But even worse, kasprintf() (and >> likely anyone else trying to find the size of the result) pass NULL >> for buf and 0 for size, so we also have end == NULL. But this means >> that the end-1 in hex_string() is (char*)-1, so buf < end-1 is true >> and we get a NULL pointer deref. I double-checked this with a trivial >> kernel module that just did a kasprintf(GFP_KERNEL, "%14ph", >> "CrashBoomBang"). > > Good catch, though I don't like the implementation of fix. > > What about the following? > > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > index 8690798..47b36ddd 100644 > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -783,11 +783,20 @@ char *hex_string(char *buf, char *end, u8 *addr, > struct printf_spec spec, > if (spec.field_width > 0) > len = min_t(int, spec.field_width, 64); > > - for (i = 0; i < len && buf < end - 1; i++) { > - buf = hex_byte_pack(buf, addr[i]); > + for (i = 0; i < len; i++) { > + if (buf < end) > + *buf = hex_asc_hi(addr[i]); > + ++buf; > + > + if (buf < end) > + *buf = hex_asc_lo(addr[i]); > + ++buf; > > - if (buf < end && separator && i != len - 1) > - *buf++ = separator; > + if (separator && i != len - 1) { > + if (buf < end) > + *buf = separator; > + ++buf; > + } > } > > return buf; I had exactly that at one point. I think the only reason I ended up doing it the other way was that I wanted to introduce the write_bytes helper, and then use that to do some simplifications and optimizations for other %p helpers. Many of those write their output to a temporary buffer, knowing exactly how much there is, then delegate to string(), which then recomputes the string length before printing. But all that can be done another time, so I'm fine with this also. Rasmus ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH 1/2] lib/vsprintf.c: Fix potential NULL deref in hex_string 2015-01-28 15:49 ` Rasmus Villemoes @ 2015-01-28 16:43 ` Andy Shevchenko 0 siblings, 0 replies; 52+ messages in thread From: Andy Shevchenko @ 2015-01-28 16:43 UTC (permalink / raw) To: Rasmus Villemoes Cc: Andrew Morton, Jiri Kosina, Randy Dunlap, Fabian Frederick, Bjorn Helgaas, Ryan Mallon, Masanari Iida, linux-kernel On Wed, 2015-01-28 at 16:49 +0100, Rasmus Villemoes wrote: > On Wed, Jan 28 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > On Wed, 2015-01-28 at 14:25 +0100, Rasmus Villemoes wrote: > >> The helper hex_string() is broken in two ways. First, it doesn't > >> increment buf regardless of whether there is room to print, so callers > >> such as kasprintf() that try to probe the correct storage to allocate > >> will get a too small return value. But even worse, kasprintf() (and > >> likely anyone else trying to find the size of the result) pass NULL > >> for buf and 0 for size, so we also have end == NULL. But this means > >> that the end-1 in hex_string() is (char*)-1, so buf < end-1 is true > >> and we get a NULL pointer deref. I double-checked this with a trivial > >> kernel module that just did a kasprintf(GFP_KERNEL, "%14ph", > >> "CrashBoomBang"). > > > > Good catch, though I don't like the implementation of fix. > > > > What about the following? > > > > > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > > index 8690798..47b36ddd 100644 > > --- a/lib/vsprintf.c > > +++ b/lib/vsprintf.c > > @@ -783,11 +783,20 @@ char *hex_string(char *buf, char *end, u8 *addr, > > struct printf_spec spec, > > if (spec.field_width > 0) > > len = min_t(int, spec.field_width, 64); > > > > - for (i = 0; i < len && buf < end - 1; i++) { > > - buf = hex_byte_pack(buf, addr[i]); > > + for (i = 0; i < len; i++) { > > + if (buf < end) > > + *buf = hex_asc_hi(addr[i]); > > + ++buf; > > + > > + if (buf < end) > > + *buf = hex_asc_lo(addr[i]); > > + ++buf; > > > > - if (buf < end && separator && i != len - 1) > > - *buf++ = separator; > > + if (separator && i != len - 1) { > > + if (buf < end) > > + *buf = separator; > > + ++buf; > > + } > > } > > > > return buf; > > I had exactly that at one point. I think the only reason I ended up > doing it the other way was that I wanted to introduce the write_bytes > helper, and then use that to do some simplifications and optimizations > for other %p helpers. Many of those write their output to a temporary > buffer, knowing exactly how much there is, then delegate to string(), > which then recomputes the string length before printing. But all that > can be done another time, so I'm fine with this also. > I ACK the version w/o write_bytes() helper. Meanwhile you may submit your helper with usage of it as another patch. -- Andy Shevchenko <andriy.shevchenko@linux.intel.com> Intel Finland Oy ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH 2/2] string_helpers: Change semantics of string_escape_mem 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 13:25 ` Rasmus Villemoes 2015-01-28 15:05 ` Andy Shevchenko 2015-01-29 10:03 ` [PATCH v2 0/3] Two printf fixes Rasmus Villemoes 2 siblings, 1 reply; 52+ messages in thread From: Rasmus Villemoes @ 2015-01-28 13:25 UTC (permalink / raw) To: Andy Shevchenko, Andrew Morton, Trond Myklebust, J. Bruce Fields, David S. Miller Cc: Rasmus Villemoes, linux-kernel, linux-nfs, netdev The current semantics of string_escape_mem are inadequate for one of its two current users, vsnprintf(). If that is to honour its contract, it must know how much space would be needed for the entire escaped buffer, and string_escape_mem provides no way of obtaining that (short of allocating a large enough buffer (~4 times input string) to let it play with, and that's definitely a big no-no inside vsnprintf). So change the semantics for string_escape_mem to be more snprintf-like: Return the size of the output that would be generated if the destination buffer was big enough, but of course still only write to the part of dst it is allowed to, and don't do '\0'-termination. It is then up to the caller to detect whether output was truncated and to append a '\0' if desired. This also fixes a bug in the escaped_string() helper function, which used to unconditionally pass a length of "end-buf" to string_escape_mem(); since the latter doesn't check osz for being insanely large, it would happily write to dst. For example, kasprintf(GFP_KERNEL, "something and then %pE", ...); is an easy way to trigger an oops. The patch is somewhat larger than I'd like, but I couldn't find a way of splitting it into smaller pieces. Implementation-wise, I changed the various escape_* helpers to return true if they handled the character, updating dst appropriately, false otherwise. Maybe there's a more elegant way, but this seems to work. In test-string_helpers.c, I removed the now meaningless -ENOMEM test, and replaced it with testing for getting the expected return value even if the buffer is too small. Also ensure that nothing is written when osz==0. In net/sunrpc/cache.c, I think qword_add still has the same semantics. Someone should definitely double-check this. Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> --- include/linux/string_helpers.h | 10 +-- lib/string_helpers.c | 195 ++++++++++++++++------------------------- lib/test-string_helpers.c | 37 ++++---- lib/vsprintf.c | 2 +- net/sunrpc/cache.c | 8 +- 5 files changed, 101 insertions(+), 151 deletions(-) diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h index 6eb567ac56bc..7a082aa183a8 100644 --- a/include/linux/string_helpers.h +++ b/include/linux/string_helpers.h @@ -47,22 +47,22 @@ static inline int string_unescape_any_inplace(char *buf) #define ESCAPE_ANY_NP (ESCAPE_ANY | ESCAPE_NP) #define ESCAPE_HEX 0x20 -int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, +size_t string_escape_mem(const char *src, size_t isz, char *dst, size_t osz, unsigned int flags, const char *esc); -static inline int string_escape_mem_any_np(const char *src, size_t isz, - char **dst, size_t osz, const char *esc) +static inline size_t string_escape_mem_any_np(const char *src, size_t isz, + char *dst, size_t osz, const char *esc) { return string_escape_mem(src, isz, dst, osz, ESCAPE_ANY_NP, esc); } -static inline int string_escape_str(const char *src, char **dst, size_t sz, +static inline size_t string_escape_str(const char *src, char *dst, size_t sz, unsigned int flags, const char *esc) { return string_escape_mem(src, strlen(src), dst, sz, flags, esc); } -static inline int string_escape_str_any_np(const char *src, char **dst, +static inline size_t string_escape_str_any_np(const char *src, char *dst, size_t sz, const char *esc) { return string_escape_str(src, dst, sz, ESCAPE_ANY_NP, esc); diff --git a/lib/string_helpers.c b/lib/string_helpers.c index 58b78ba57439..288bacca74aa 100644 --- a/lib/string_helpers.c +++ b/lib/string_helpers.c @@ -243,28 +243,20 @@ int string_unescape(char *src, char *dst, size_t size, unsigned int flags) } EXPORT_SYMBOL(string_unescape); -static int escape_passthrough(unsigned char c, char **dst, size_t *osz) +static bool escape_passthrough(unsigned char c, char **dst, char *end) { char *out = *dst; - if (*osz < 1) - return -ENOMEM; - - *out++ = c; - - *dst = out; - *osz -= 1; - - return 1; + if (out < end) + *out = c; + *dst = out + 1; + return true; } -static int escape_space(unsigned char c, char **dst, size_t *osz) +static bool escape_space(unsigned char c, char **dst, char *end) { - char *out = *dst; unsigned char to; - - if (*osz < 2) - return -ENOMEM; + char *out = *dst; switch (c) { case '\n': @@ -283,25 +275,22 @@ static int escape_space(unsigned char c, char **dst, size_t *osz) to = 'f'; break; default: - return 0; + return false; } - *out++ = '\\'; - *out++ = to; - - *dst = out; - *osz -= 2; + if (out + 0 < end) + out[0] = '\\'; + if (out + 1 < end) + out[1] = to; - return 1; + *dst = out + 2; + return true; } -static int escape_special(unsigned char c, char **dst, size_t *osz) +static bool escape_special(unsigned char c, char **dst, char *end) { - char *out = *dst; unsigned char to; - - if (*osz < 2) - return -ENOMEM; + char *out = *dst; switch (c) { case '\\': @@ -314,71 +303,66 @@ static int escape_special(unsigned char c, char **dst, size_t *osz) to = 'e'; break; default: - return 0; + return false; } - *out++ = '\\'; - *out++ = to; + if (out + 0 < end) + out[0] = '\\'; + if (out + 1 < end) + out[1] = to; - *dst = out; - *osz -= 2; - - return 1; + *dst = out + 2; + return true; } -static int escape_null(unsigned char c, char **dst, size_t *osz) +static bool escape_null(unsigned char c, char **dst, char *end) { char *out = *dst; - if (*osz < 2) - return -ENOMEM; - if (c) - return 0; - - *out++ = '\\'; - *out++ = '0'; + return false; - *dst = out; - *osz -= 2; + if (out + 0 < end) + out[0] = '\\'; + if (out + 1 < end) + out[1] = '0'; - return 1; + *dst = out + 2; + return true; } -static int escape_octal(unsigned char c, char **dst, size_t *osz) +static bool escape_octal(unsigned char c, char **dst, char *end) { char *out = *dst; - if (*osz < 4) - return -ENOMEM; - - *out++ = '\\'; - *out++ = ((c >> 6) & 0x07) + '0'; - *out++ = ((c >> 3) & 0x07) + '0'; - *out++ = ((c >> 0) & 0x07) + '0'; + if (out + 0 < end) + out[0] = '\\'; + if (out + 1 < end) + out[1] = ((c >> 6) & 0x07) + '0'; + if (out + 2 < end) + out[2] = ((c >> 3) & 0x07) + '0'; + if (out + 3 < end) + out[3] = ((c >> 0) & 0x07) + '0'; - *dst = out; - *osz -= 4; - - return 1; + *dst = out + 4; + return true; } -static int escape_hex(unsigned char c, char **dst, size_t *osz) +static bool escape_hex(unsigned char c, char **dst, char *end) { char *out = *dst; - if (*osz < 4) - return -ENOMEM; + if (out + 0 < end) + out[0] = '\\'; + if (out + 1 < end) + out[1] = 'x'; + if (out + 2 < end) + out[2] = hex_asc_hi(c); + if (out + 3 < end) + out[3] = hex_asc_lo(c); - *out++ = '\\'; - *out++ = 'x'; - *out++ = hex_asc_hi(c); - *out++ = hex_asc_lo(c); - - *dst = out; - *osz -= 4; - - return 1; + *dst = out + 4; + return true; } /** @@ -430,19 +414,17 @@ static int escape_hex(unsigned char c, char **dst, size_t *osz) * it if needs. * * Return: - * The amount of the characters processed to the destination buffer, or - * %-ENOMEM if the size of buffer is not enough to put an escaped character is - * returned. - * - * Even in the case of error @dst pointer will be updated to point to the byte - * after the last processed character. + * The total size of the escaped output that would be generated for + * the given input and flags. To check whether the output was + * truncated, compare the return value to osz. There is room left in + * dst for a '\0' terminator if and only if ret < osz. */ -int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, - unsigned int flags, const char *esc) +size_t string_escape_mem(const char *src, size_t isz, char *dst, size_t osz, + unsigned int flags, const char *esc) { - char *out = *dst, *p = out; + char *p = dst; + char *end = dst + osz; bool is_dict = esc && *esc; - int ret = 0; while (isz--) { unsigned char c = *src++; @@ -462,55 +444,26 @@ int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, (is_dict && !strchr(esc, c))) { /* do nothing */ } else { - if (flags & ESCAPE_SPACE) { - ret = escape_space(c, &p, &osz); - if (ret < 0) - break; - if (ret > 0) - continue; - } - - if (flags & ESCAPE_SPECIAL) { - ret = escape_special(c, &p, &osz); - if (ret < 0) - break; - if (ret > 0) - continue; - } - - if (flags & ESCAPE_NULL) { - ret = escape_null(c, &p, &osz); - if (ret < 0) - break; - if (ret > 0) - continue; - } + if (flags & ESCAPE_SPACE && escape_space(c, &p, end)) + continue; + + if (flags & ESCAPE_SPECIAL && escape_special(c, &p, end)) + continue; + + if (flags & ESCAPE_NULL && escape_null(c, &p, end)) + continue; /* ESCAPE_OCTAL and ESCAPE_HEX always go last */ - if (flags & ESCAPE_OCTAL) { - ret = escape_octal(c, &p, &osz); - if (ret < 0) - break; + if (flags & ESCAPE_OCTAL && escape_octal(c, &p, end)) continue; - } - if (flags & ESCAPE_HEX) { - ret = escape_hex(c, &p, &osz); - if (ret < 0) - break; + + if (flags & ESCAPE_HEX && escape_hex(c, &p, end)) continue; - } } - ret = escape_passthrough(c, &p, &osz); - if (ret < 0) - break; + escape_passthrough(c, &p, end); } - *dst = p; - - if (ret < 0) - return ret; - - return p - out; + return p - dst; } EXPORT_SYMBOL(string_escape_mem); diff --git a/lib/test-string_helpers.c b/lib/test-string_helpers.c index ab0d30e1e18f..5f95114a2f86 100644 --- a/lib/test-string_helpers.c +++ b/lib/test-string_helpers.c @@ -264,12 +264,12 @@ static __init void test_string_escape(const char *name, const struct test_string_2 *s2, unsigned int flags, const char *esc) { - int q_real = 512; - char *out_test = kmalloc(q_real, GFP_KERNEL); - char *out_real = kmalloc(q_real, GFP_KERNEL); + size_t out_size = 512; + char *out_test = kmalloc(out_size, GFP_KERNEL); + char *out_real = kmalloc(out_size, GFP_KERNEL); char *in = kmalloc(256, GFP_KERNEL); - char *buf = out_real; - int p = 0, q_test = 0; + size_t p = 0, q_test = 0; + size_t q_real; if (!out_test || !out_real || !in) goto out; @@ -301,29 +301,26 @@ static __init void test_string_escape(const char *name, q_test += len; } - q_real = string_escape_mem(in, p, &buf, q_real, flags, esc); + q_real = string_escape_mem(in, p, out_real, out_size, flags, esc); test_string_check_buf(name, flags, in, p, out_real, q_real, out_test, q_test); + + 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 %zu, got %zu\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); + out: kfree(in); kfree(out_real); kfree(out_test); } -static __init void test_string_escape_nomem(void) -{ - char *in = "\eb \\C\007\"\x90\r]"; - char out[64], *buf = out; - int rc = -ENOMEM, ret; - - ret = string_escape_str_any_np(in, &buf, strlen(in), NULL); - if (ret == rc) - return; - - pr_err("Test 'escape nomem' failed: got %d instead of %d\n", ret, rc); -} - static int __init test_string_helpers_init(void) { unsigned int i; @@ -342,8 +339,6 @@ static int __init test_string_helpers_init(void) for (i = 0; i < (ESCAPE_ANY_NP | ESCAPE_HEX) + 1; i++) test_string_escape("escape 1", escape1, i, TEST_STRING_2_DICT_1); - test_string_escape_nomem(); - return -EINVAL; } module_init(test_string_helpers_init); diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 0d57be58448f..a3e474f9957f 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -1165,7 +1165,7 @@ char *escaped_string(char *buf, char *end, u8 *addr, struct printf_spec spec, len = spec.field_width < 0 ? 1 : spec.field_width; /* Ignore the error. We print as many characters as we can */ - string_escape_mem(addr, len, &buf, end - buf, flags, NULL); + buf += string_escape_mem(addr, len, buf, buf < end ? end - buf : 0, flags, NULL); return buf; } diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c index 33fb105d4352..22c4418057f4 100644 --- a/net/sunrpc/cache.c +++ b/net/sunrpc/cache.c @@ -1068,12 +1068,14 @@ void qword_add(char **bpp, int *lp, char *str) { char *bp = *bpp; int len = *lp; - int ret; + int ret, written; if (len < 0) return; - ret = string_escape_str(str, &bp, len, ESCAPE_OCTAL, "\\ \n\t"); - if (ret < 0 || ret == len) + ret = string_escape_str(str, bp, len, ESCAPE_OCTAL, "\\ \n\t"); + written = min(ret, len); + bp += written; + if (ret >= len) len = -1; else { len -= ret; -- 2.1.3 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH 2/2] string_helpers: Change semantics of string_escape_mem 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 0 siblings, 0 replies; 52+ messages in thread From: Andy Shevchenko @ 2015-01-28 15:05 UTC (permalink / raw) To: Rasmus Villemoes Cc: Andrew Morton, Trond Myklebust, J. Bruce Fields, David S. Miller, linux-kernel, linux-nfs, netdev On Wed, 2015-01-28 at 14:25 +0100, Rasmus Villemoes wrote: > The current semantics of string_escape_mem are inadequate for one of > its two current users, vsnprintf(). If that is to honour its contract, > it must know how much space would be needed for the entire escaped > buffer, and string_escape_mem provides no way of obtaining that (short > of allocating a large enough buffer (~4 times input string) to let it > play with, and that's definitely a big no-no inside vsnprintf). > > So change the semantics for string_escape_mem to be more > snprintf-like: Return the size of the output that would be generated > if the destination buffer was big enough, but of course still only > write to the part of dst it is allowed to, and don't do > '\0'-termination. It is then up to the caller to detect whether output > was truncated and to append a '\0' if desired. > > This also fixes a bug in the escaped_string() helper function, which > used to unconditionally pass a length of "end-buf" to > string_escape_mem(); since the latter doesn't check osz for being > insanely large, it would happily write to dst. For example, > kasprintf(GFP_KERNEL, "something and then %pE", ...); is an easy way > to trigger an oops. > The patch is somewhat larger than I'd like, but I couldn't find a way > of splitting it into smaller pieces. Implementation-wise, I changed > the various escape_* helpers to return true if they handled the > character, updating dst appropriately, false otherwise. Maybe there's > a more elegant way, but this seems to work. Can we split this to at least two parts: internal API changes to string_escape_mem() and the rest? > In test-string_helpers.c, I removed the now meaningless -ENOMEM test, > and replaced it with testing for getting the expected return value > even if the buffer is too small. Also ensure that nothing is written > when osz==0. > > In net/sunrpc/cache.c, I think qword_add still has the same > semantics. Someone should definitely double-check this. -- Andy Shevchenko <andriy.shevchenko@intel.com> Intel Finland Oy ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 0/3] Two printf fixes 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 13:25 ` [PATCH 2/2] string_helpers: Change semantics of string_escape_mem Rasmus Villemoes @ 2015-01-29 10:03 ` Rasmus Villemoes 2015-01-29 10:03 ` [PATCH v2 1/3] lib/vsprintf.c: Fix potential NULL deref in hex_string Rasmus Villemoes ` (3 more replies) 2 siblings, 4 replies; 52+ messages in thread From: Rasmus Villemoes @ 2015-01-29 10:03 UTC (permalink / raw) To: Andy Shevchenko, Andrew Morton, Trond Myklebust, J. Bruce Fields, David S. Miller Cc: Rasmus Villemoes, linux-kernel, linux-nfs, netdev Both %pE and %ph are unusable in kasprintf(), since the occurrence of either will trigger an oops during the first vsnprintf call where kasprintf tries to find the correct size to allocate. These oopses could be papered over with somewhat smaller patches than these, but then the return value from vsnprintf would still not reflect the actual size needed. For %pE, this requires a change of semantics of string_escape_mem and hence an annoyingly large diffstat. Whether this is 3.20, 3.21 and/or -stable material (or /dev/null material, for that matter ;-)) I'll leave to others to decide. v2: Suggestions from Andy Shevchenko: * Simpler fix of hex_string(). * The string_escape_mem change is split in two, 2/3 updating the internal helpers and 3/3 then changing the external interface. Rasmus Villemoes (3): lib/vsprintf.c: Fix potential NULL deref in hex_string lib/string_helpers.c: Refactor string_escape_mem lib/string_helpers.c: Change semantics of string_escape_mem include/linux/string_helpers.h | 10 +-- lib/string_helpers.c | 191 ++++++++++++++++------------------------- lib/test-string_helpers.c | 37 ++++---- lib/vsprintf.c | 18 ++-- net/sunrpc/cache.c | 8 +- 5 files changed, 111 insertions(+), 153 deletions(-) -- 2.1.3 ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 1/3] lib/vsprintf.c: Fix potential NULL deref in hex_string 2015-01-29 10:03 ` [PATCH v2 0/3] Two printf fixes Rasmus Villemoes @ 2015-01-29 10:03 ` 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 ` (2 subsequent siblings) 3 siblings, 1 reply; 52+ messages in thread From: Rasmus Villemoes @ 2015-01-29 10:03 UTC (permalink / raw) To: Andy Shevchenko, Andrew Morton, Jiri Kosina, Randy Dunlap, Fabian Frederick, Bjorn Helgaas, Ryan Mallon, Masanari Iida Cc: Rasmus Villemoes, linux-kernel The helper hex_string() is broken in two ways. First, it doesn't increment buf regardless of whether there is room to print, so callers such as kasprintf() that try to probe the correct storage to allocate will get a too small return value. But even worse, kasprintf() (and likely anyone else trying to find the size of the result) pass NULL for buf and 0 for size, so we also have end == NULL. But this means that the end-1 in hex_string() is (char*)-1, so buf < end-1 is true and we get a NULL pointer deref. I double-checked this with a trivial kernel module that just did a kasprintf(GFP_KERNEL, "%14ph", "CrashBoomBang"). Nobody seems to be using %ph with kasprintf, but we might as well fix it before it hits someone. Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> --- lib/vsprintf.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/lib/vsprintf.c b/lib/vsprintf.c index ec337f64f52d..3568e3906777 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -782,11 +782,19 @@ char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec, if (spec.field_width > 0) len = min_t(int, spec.field_width, 64); - for (i = 0; i < len && buf < end - 1; i++) { - buf = hex_byte_pack(buf, addr[i]); + for (i = 0; i < len; ++i) { + if (buf < end) + *buf = hex_asc_hi(addr[i]); + ++buf; + if (buf < end) + *buf = hex_asc_lo(addr[i]); + ++buf; - if (buf < end && separator && i != len - 1) - *buf++ = separator; + if (separator && i != len - 1) { + if (buf < end) + *buf = separator; + ++buf; + } } return buf; -- 2.1.3 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH v2 1/3] lib/vsprintf.c: Fix potential NULL deref in hex_string 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 0 siblings, 0 replies; 52+ messages in thread From: Andy Shevchenko @ 2015-01-29 10:43 UTC (permalink / raw) To: Rasmus Villemoes Cc: Andrew Morton, Jiri Kosina, Randy Dunlap, Fabian Frederick, Bjorn Helgaas, Ryan Mallon, Masanari Iida, linux-kernel On Thu, 2015-01-29 at 11:03 +0100, Rasmus Villemoes wrote: > The helper hex_string() is broken in two ways. First, it doesn't > increment buf regardless of whether there is room to print, so callers > such as kasprintf() that try to probe the correct storage to allocate > will get a too small return value. But even worse, kasprintf() (and > likely anyone else trying to find the size of the result) pass NULL > for buf and 0 for size, so we also have end == NULL. But this means > that the end-1 in hex_string() is (char*)-1, so buf < end-1 is true > and we get a NULL pointer deref. I double-checked this with a trivial > kernel module that just did a kasprintf(GFP_KERNEL, "%14ph", > "CrashBoomBang"). > > Nobody seems to be using %ph with kasprintf, but we might as well fix > it before it hits someone. > > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > lib/vsprintf.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > index ec337f64f52d..3568e3906777 100644 > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -782,11 +782,19 @@ char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec, > if (spec.field_width > 0) > len = min_t(int, spec.field_width, 64); > > - for (i = 0; i < len && buf < end - 1; i++) { > - buf = hex_byte_pack(buf, addr[i]); > + for (i = 0; i < len; ++i) { > + if (buf < end) > + *buf = hex_asc_hi(addr[i]); > + ++buf; > + if (buf < end) > + *buf = hex_asc_lo(addr[i]); > + ++buf; > > - if (buf < end && separator && i != len - 1) > - *buf++ = separator; > + if (separator && i != len - 1) { > + if (buf < end) > + *buf = separator; > + ++buf; > + } > } > > return buf; -- Andy Shevchenko <andriy.shevchenko@linux.intel.com> Intel Finland Oy ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 2/3] lib/string_helpers.c: Refactor string_escape_mem 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:03 ` Rasmus Villemoes 2015-01-29 12:12 ` Andy Shevchenko 2015-01-29 10:03 ` [PATCH v2 3/3] lib/string_helpers.c: Change semantics of string_escape_mem Rasmus Villemoes 2015-02-09 23:44 ` Rasmus Villemoes 3 siblings, 1 reply; 52+ messages in thread From: Rasmus Villemoes @ 2015-01-29 10:03 UTC (permalink / raw) To: Andy Shevchenko, Andrew Morton, Mathias Krause Cc: Rasmus Villemoes, linux-kernel When printf is given the format specifier %pE, it needs a way of obtaining the total output size that would be generated if the buffer was large enough, and string_escape_mem doesn't easily provide that. This is a refactorization of string_escape_mem in preparation of changing its external API to provide that information. Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> --- lib/string_helpers.c | 182 ++++++++++++++++++++------------------------------- 1 file changed, 72 insertions(+), 110 deletions(-) diff --git a/lib/string_helpers.c b/lib/string_helpers.c index 58b78ba57439..e14dd8555760 100644 --- a/lib/string_helpers.c +++ b/lib/string_helpers.c @@ -243,29 +243,21 @@ int string_unescape(char *src, char *dst, size_t size, unsigned int flags) } EXPORT_SYMBOL(string_unescape); -static int escape_passthrough(unsigned char c, char **dst, size_t *osz) +static bool escape_passthrough(unsigned char c, char **dst, char *end) { char *out = *dst; - if (*osz < 1) - return -ENOMEM; - - *out++ = c; - - *dst = out; - *osz -= 1; - - return 1; + if (out < end) + *out = c; + *dst = out + 1; + return true; } -static int escape_space(unsigned char c, char **dst, size_t *osz) +static bool escape_space(unsigned char c, char **dst, char *end) { char *out = *dst; unsigned char to; - if (*osz < 2) - return -ENOMEM; - switch (c) { case '\n': to = 'n'; @@ -283,26 +275,23 @@ static int escape_space(unsigned char c, char **dst, size_t *osz) to = 'f'; break; default: - return 0; + return false; } - *out++ = '\\'; - *out++ = to; + if (out + 0 < end) + out[0] = '\\'; + if (out + 1 < end) + out[1] = to; - *dst = out; - *osz -= 2; - - return 1; + *dst = out + 2; + return true; } -static int escape_special(unsigned char c, char **dst, size_t *osz) +static bool escape_special(unsigned char c, char **dst, char *end) { char *out = *dst; unsigned char to; - if (*osz < 2) - return -ENOMEM; - switch (c) { case '\\': to = '\\'; @@ -314,71 +303,66 @@ static int escape_special(unsigned char c, char **dst, size_t *osz) to = 'e'; break; default: - return 0; + return false; } - *out++ = '\\'; - *out++ = to; + if (out + 0 < end) + out[0] = '\\'; + if (out + 1 < end) + out[1] = to; - *dst = out; - *osz -= 2; - - return 1; + *dst = out + 2; + return true; } -static int escape_null(unsigned char c, char **dst, size_t *osz) +static bool escape_null(unsigned char c, char **dst, char *end) { char *out = *dst; - if (*osz < 2) - return -ENOMEM; - if (c) - return 0; - - *out++ = '\\'; - *out++ = '0'; + return false; - *dst = out; - *osz -= 2; + if (out + 0 < end) + out[0] = '\\'; + if (out + 1 < end) + out[1] = '0'; - return 1; + *dst = out + 2; + return true; } -static int escape_octal(unsigned char c, char **dst, size_t *osz) +static bool escape_octal(unsigned char c, char **dst, char *end) { char *out = *dst; - if (*osz < 4) - return -ENOMEM; - - *out++ = '\\'; - *out++ = ((c >> 6) & 0x07) + '0'; - *out++ = ((c >> 3) & 0x07) + '0'; - *out++ = ((c >> 0) & 0x07) + '0'; + if (out + 0 < end) + out[0] = '\\'; + if (out + 1 < end) + out[1] = ((c >> 6) & 0x07) + '0'; + if (out + 2 < end) + out[2] = ((c >> 3) & 0x07) + '0'; + if (out + 3 < end) + out[3] = ((c >> 0) & 0x07) + '0'; - *dst = out; - *osz -= 4; - - return 1; + *dst = out + 4; + return true; } -static int escape_hex(unsigned char c, char **dst, size_t *osz) +static bool escape_hex(unsigned char c, char **dst, char *end) { char *out = *dst; - if (*osz < 4) - return -ENOMEM; - - *out++ = '\\'; - *out++ = 'x'; - *out++ = hex_asc_hi(c); - *out++ = hex_asc_lo(c); - - *dst = out; - *osz -= 4; + if (out + 0 < end) + out[0] = '\\'; + if (out + 1 < end) + out[1] = 'x'; + if (out + 2 < end) + out[2] = hex_asc_hi(c); + if (out + 3 < end) + out[3] = hex_asc_lo(c); - return 1; + *dst = out + 4; + return true; } /** @@ -440,9 +424,10 @@ static int escape_hex(unsigned char c, char **dst, size_t *osz) int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, unsigned int flags, const char *esc) { - char *out = *dst, *p = out; + char *p = *dst; + char *end = p + osz; bool is_dict = esc && *esc; - int ret = 0; + int ret; while (isz--) { unsigned char c = *src++; @@ -462,55 +447,32 @@ int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, (is_dict && !strchr(esc, c))) { /* do nothing */ } else { - if (flags & ESCAPE_SPACE) { - ret = escape_space(c, &p, &osz); - if (ret < 0) - break; - if (ret > 0) - continue; - } - - if (flags & ESCAPE_SPECIAL) { - ret = escape_special(c, &p, &osz); - if (ret < 0) - break; - if (ret > 0) - continue; - } - - if (flags & ESCAPE_NULL) { - ret = escape_null(c, &p, &osz); - if (ret < 0) - break; - if (ret > 0) - continue; - } + if (flags & ESCAPE_SPACE && escape_space(c, &p, end)) + continue; + + if (flags & ESCAPE_SPECIAL && escape_special(c, &p, end)) + continue; + + if (flags & ESCAPE_NULL && escape_null(c, &p, end)) + continue; /* ESCAPE_OCTAL and ESCAPE_HEX always go last */ - if (flags & ESCAPE_OCTAL) { - ret = escape_octal(c, &p, &osz); - if (ret < 0) - break; + if (flags & ESCAPE_OCTAL && escape_octal(c, &p, end)) continue; - } - if (flags & ESCAPE_HEX) { - ret = escape_hex(c, &p, &osz); - if (ret < 0) - break; + + if (flags & ESCAPE_HEX && escape_hex(c, &p, end)) continue; - } } - ret = escape_passthrough(c, &p, &osz); - if (ret < 0) - break; + escape_passthrough(c, &p, end); + } + if (p > end) { + *dst = end; + return -ENOMEM; } + ret = p - *dst; *dst = p; - - if (ret < 0) - return ret; - - return p - out; + return ret; } EXPORT_SYMBOL(string_escape_mem); -- 2.1.3 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] lib/string_helpers.c: Refactor string_escape_mem 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 0 siblings, 1 reply; 52+ messages in thread From: Andy Shevchenko @ 2015-01-29 12:12 UTC (permalink / raw) To: Rasmus Villemoes; +Cc: Andrew Morton, Mathias Krause, linux-kernel On Thu, 2015-01-29 at 11:03 +0100, Rasmus Villemoes wrote: > When printf is given the format specifier %pE, it needs a way of > obtaining the total output size that would be generated if the buffer > was large enough, and string_escape_mem doesn't easily provide > that. This is a refactorization of string_escape_mem in preparation of > changing its external API to provide that information. Few comments below. > > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> > --- > lib/string_helpers.c | 182 ++++++++++++++++++++------------------------------- > 1 file changed, 72 insertions(+), 110 deletions(-) > > diff --git a/lib/string_helpers.c b/lib/string_helpers.c > index 58b78ba57439..e14dd8555760 100644 > --- a/lib/string_helpers.c > +++ b/lib/string_helpers.c > @@ -243,29 +243,21 @@ int string_unescape(char *src, char *dst, size_t size, unsigned int flags) > } > EXPORT_SYMBOL(string_unescape); > > -static int escape_passthrough(unsigned char c, char **dst, size_t *osz) > +static bool escape_passthrough(unsigned char c, char **dst, char *end) > { > char *out = *dst; > > - if (*osz < 1) > - return -ENOMEM; > - > - *out++ = c; > - > - *dst = out; > - *osz -= 1; > - > - return 1; > + if (out < end) > + *out = c; > + *dst = out + 1; > + return true; > } > > -static int escape_space(unsigned char c, char **dst, size_t *osz) > +static bool escape_space(unsigned char c, char **dst, char *end) > { > char *out = *dst; > unsigned char to; > > - if (*osz < 2) > - return -ENOMEM; > - > switch (c) { > case '\n': > to = 'n'; > @@ -283,26 +275,23 @@ static int escape_space(unsigned char c, char **dst, size_t *osz) > to = 'f'; > break; > default: > - return 0; > + return false; > } > > - *out++ = '\\'; > - *out++ = to; > + if (out + 0 < end) > + out[0] = '\\'; > + if (out + 1 < end) > + out[1] = to; Could we do this in the same way like for hex_string, i.e. if (out < end) *out = '\\'; ++out; … *dst = out; return true; ? > > - *dst = out; > - *osz -= 2; > - > - return 1; > + *dst = out + 2; > + return true; > } > > -static int escape_special(unsigned char c, char **dst, size_t *osz) > +static bool escape_special(unsigned char c, char **dst, char *end) > { > char *out = *dst; > unsigned char to; > > - if (*osz < 2) > - return -ENOMEM; > - > switch (c) { > case '\\': > to = '\\'; > @@ -314,71 +303,66 @@ static int escape_special(unsigned char c, char **dst, size_t *osz) > to = 'e'; > break; > default: > - return 0; > + return false; > } > > - *out++ = '\\'; > - *out++ = to; > + if (out + 0 < end) > + out[0] = '\\'; > + if (out + 1 < end) > + out[1] = to; Ditto. > > - *dst = out; > - *osz -= 2; > - > - return 1; > + *dst = out + 2; > + return true; > } > > -static int escape_null(unsigned char c, char **dst, size_t *osz) > +static bool escape_null(unsigned char c, char **dst, char *end) > { > char *out = *dst; > > - if (*osz < 2) > - return -ENOMEM; > - > if (c) > - return 0; > - > - *out++ = '\\'; > - *out++ = '0'; > + return false; > > - *dst = out; > - *osz -= 2; > + if (out + 0 < end) > + out[0] = '\\'; > + if (out + 1 < end) > + out[1] = '0'; Ditto. > > - return 1; > + *dst = out + 2; > + return true; > } > > -static int escape_octal(unsigned char c, char **dst, size_t *osz) > +static bool escape_octal(unsigned char c, char **dst, char *end) > { > char *out = *dst; > > - if (*osz < 4) > - return -ENOMEM; > - > - *out++ = '\\'; > - *out++ = ((c >> 6) & 0x07) + '0'; > - *out++ = ((c >> 3) & 0x07) + '0'; > - *out++ = ((c >> 0) & 0x07) + '0'; > + if (out + 0 < end) > + out[0] = '\\'; > + if (out + 1 < end) > + out[1] = ((c >> 6) & 0x07) + '0'; > + if (out + 2 < end) > + out[2] = ((c >> 3) & 0x07) + '0'; > + if (out + 3 < end) > + out[3] = ((c >> 0) & 0x07) + '0'; Ditto. > > - *dst = out; > - *osz -= 4; > - > - return 1; > + *dst = out + 4; > + return true; > } > > -static int escape_hex(unsigned char c, char **dst, size_t *osz) > +static bool escape_hex(unsigned char c, char **dst, char *end) > { > char *out = *dst; > > - if (*osz < 4) > - return -ENOMEM; > - > - *out++ = '\\'; > - *out++ = 'x'; > - *out++ = hex_asc_hi(c); > - *out++ = hex_asc_lo(c); > - > - *dst = out; > - *osz -= 4; > + if (out + 0 < end) > + out[0] = '\\'; > + if (out + 1 < end) > + out[1] = 'x'; > + if (out + 2 < end) > + out[2] = hex_asc_hi(c); > + if (out + 3 < end) > + out[3] = hex_asc_lo(c); Ditto. > > - return 1; > + *dst = out + 4; > + return true; > } > > /** > @@ -440,9 +424,10 @@ static int escape_hex(unsigned char c, char **dst, size_t *osz) > int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, > unsigned int flags, const char *esc) > { > - char *out = *dst, *p = out; > + char *p = *dst; Leave 'out' here and… > + char *end = p + osz; > bool is_dict = esc && *esc; > - int ret = 0; > + int ret; > > while (isz--) { > unsigned char c = *src++; > @@ -462,55 +447,32 @@ int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, > (is_dict && !strchr(esc, c))) { > /* do nothing */ > } else { > - if (flags & ESCAPE_SPACE) { > - ret = escape_space(c, &p, &osz); > - if (ret < 0) > - break; > - if (ret > 0) > - continue; > - } > - > - if (flags & ESCAPE_SPECIAL) { > - ret = escape_special(c, &p, &osz); > - if (ret < 0) > - break; > - if (ret > 0) > - continue; > - } > - > - if (flags & ESCAPE_NULL) { > - ret = escape_null(c, &p, &osz); > - if (ret < 0) > - break; > - if (ret > 0) > - continue; > - } > + if (flags & ESCAPE_SPACE && escape_space(c, &p, end)) > + continue; > + > + if (flags & ESCAPE_SPECIAL && escape_special(c, &p, end)) > + continue; > + > + if (flags & ESCAPE_NULL && escape_null(c, &p, end)) > + continue; > > /* ESCAPE_OCTAL and ESCAPE_HEX always go last */ > - if (flags & ESCAPE_OCTAL) { > - ret = escape_octal(c, &p, &osz); > - if (ret < 0) > - break; > + if (flags & ESCAPE_OCTAL && escape_octal(c, &p, end)) > continue; > - } > - if (flags & ESCAPE_HEX) { > - ret = escape_hex(c, &p, &osz); > - if (ret < 0) > - break; > + > + if (flags & ESCAPE_HEX && escape_hex(c, &p, end)) > continue; > - } > } > > - ret = escape_passthrough(c, &p, &osz); > - if (ret < 0) > - break; > + escape_passthrough(c, &p, end); > + } + black line. > + if (p > end) { > + *dst = end; > + return -ENOMEM; > } > > + ret = p - *dst; > *dst = p; > - > - if (ret < 0) > - return ret; > - > - return p - out; …and do not change the logic right now. Just substitute if (ret < 0) by above if (p > end). > + return ret; > } > EXPORT_SYMBOL(string_escape_mem); -- Andy Shevchenko <andriy.shevchenko@intel.com> Intel Finland Oy ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] lib/string_helpers.c: Refactor string_escape_mem 2015-01-29 12:12 ` Andy Shevchenko @ 2015-01-29 13:10 ` Rasmus Villemoes 2015-01-29 13:37 ` Andy Shevchenko 0 siblings, 1 reply; 52+ messages in thread From: Rasmus Villemoes @ 2015-01-29 13:10 UTC (permalink / raw) To: Andy Shevchenko; +Cc: Andrew Morton, Mathias Krause, linux-kernel On Thu, Jan 29 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: >> >> -static int escape_space(unsigned char c, char **dst, size_t *osz) >> +static bool escape_space(unsigned char c, char **dst, char *end) >> { >> char *out = *dst; >> unsigned char to; >> >> - if (*osz < 2) >> - return -ENOMEM; >> - >> switch (c) { >> case '\n': >> to = 'n'; >> @@ -283,26 +275,23 @@ static int escape_space(unsigned char c, char **dst, size_t *osz) >> to = 'f'; >> break; >> default: >> - return 0; >> + return false; >> } >> >> - *out++ = '\\'; >> - *out++ = to; >> + if (out + 0 < end) >> + out[0] = '\\'; >> + if (out + 1 < end) >> + out[1] = to; > > Could we do this in the same way like for hex_string, i.e. > > if (out < end) > *out = '\\'; > ++out; > > … > > *dst = out; > return true; > > ? We could, but I don't think either is more readable than the other. Hence I chose the one requiring 2n+1 lines instead of 3n+1 lines. Had this been in vsprintf.c I would stick to the pattern you suggest. >> @@ -440,9 +424,10 @@ static int escape_hex(unsigned char c, char **dst, size_t *osz) >> int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, >> unsigned int flags, const char *esc) >> { >> - char *out = *dst, *p = out; >> + char *p = *dst; > > Leave 'out' here and… > >> + char *end = p + osz; >> bool is_dict = esc && *esc; >> - int ret = 0; >> + int ret; >> >> while (isz--) { >> unsigned char c = *src++; >> @@ -462,55 +447,32 @@ int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, >> (is_dict && !strchr(esc, c))) { >> /* do nothing */ >> } else { >> - if (flags & ESCAPE_SPACE) { >> - ret = escape_space(c, &p, &osz); >> - if (ret < 0) >> - break; >> - if (ret > 0) >> - continue; >> - } >> - >> - if (flags & ESCAPE_SPECIAL) { >> - ret = escape_special(c, &p, &osz); >> - if (ret < 0) >> - break; >> - if (ret > 0) >> - continue; >> - } >> - >> - if (flags & ESCAPE_NULL) { >> - ret = escape_null(c, &p, &osz); >> - if (ret < 0) >> - break; >> - if (ret > 0) >> - continue; >> - } >> + if (flags & ESCAPE_SPACE && escape_space(c, &p, end)) >> + continue; >> + >> + if (flags & ESCAPE_SPECIAL && escape_special(c, &p, end)) >> + continue; >> + >> + if (flags & ESCAPE_NULL && escape_null(c, &p, end)) >> + continue; >> >> /* ESCAPE_OCTAL and ESCAPE_HEX always go last */ >> - if (flags & ESCAPE_OCTAL) { >> - ret = escape_octal(c, &p, &osz); >> - if (ret < 0) >> - break; >> + if (flags & ESCAPE_OCTAL && escape_octal(c, &p, end)) >> continue; >> - } >> - if (flags & ESCAPE_HEX) { >> - ret = escape_hex(c, &p, &osz); >> - if (ret < 0) >> - break; >> + >> + if (flags & ESCAPE_HEX && escape_hex(c, &p, end)) >> continue; >> - } >> } >> >> - ret = escape_passthrough(c, &p, &osz); >> - if (ret < 0) >> - break; >> + escape_passthrough(c, &p, end); >> + } > > + black line. > >> + if (p > end) { >> + *dst = end; >> + return -ENOMEM; >> } >> >> + ret = p - *dst; >> *dst = p; >> - >> - if (ret < 0) >> - return ret; >> - >> - return p - out; > > …and do not change the logic right now. Just substitute if (ret < 0) by > above if (p > end). > I'm not sure I follow. How does this change the logic? We return -ENOMEM if and only if the entire output didn't fit, while still updating *dst to point to one past the last output character. If the output did fit, we return the size of the output. One thing that only occured to me now is that we may now leave a partial escape sequence at the end of the buffer. I can't see how this can reasonably be avoided while still doing a meaningful refactorization preparing it for the next patch. Rasmus ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] lib/string_helpers.c: Refactor string_escape_mem 2015-01-29 13:10 ` Rasmus Villemoes @ 2015-01-29 13:37 ` Andy Shevchenko 2015-01-29 19:33 ` Jeff Epler 0 siblings, 1 reply; 52+ messages in thread From: Andy Shevchenko @ 2015-01-29 13:37 UTC (permalink / raw) To: Rasmus Villemoes; +Cc: Andrew Morton, Mathias Krause, linux-kernel On Thu, 2015-01-29 at 14:10 +0100, Rasmus Villemoes wrote: > On Thu, Jan 29 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > >> - *out++ = '\\'; > >> - *out++ = to; > >> + if (out + 0 < end) > >> + out[0] = '\\'; > >> + if (out + 1 < end) > >> + out[1] = to; > > > > Could we do this in the same way like for hex_string, i.e. > > > > if (out < end) > > *out = '\\'; > > ++out; > > > > … > > > > *dst = out; > > return true; > > > > ? > > We could, but I don't think either is more readable than the > other. Hence I chose the one requiring 2n+1 lines instead of 3n+1 > lines. Had this been in vsprintf.c I would stick to the pattern you > suggest. I think the pattern with 3n lines is a bit more readable. > >> + ret = p - *dst; > >> *dst = p; > >> - > >> - if (ret < 0) > >> - return ret; > >> - > >> - return p - out; > > > > …and do not change the logic right now. Just substitute if (ret < 0) by > > above if (p > end). > > > > I'm not sure I follow. How does this change the logic? We return -ENOMEM > if and only if the entire output didn't fit, while still updating *dst > to point to one past the last output character. If the output did fit, > we return the size of the output. I meant to do the arithmetics like it was in the original code. > One thing that only occured to me now is that we may now leave a partial > escape sequence at the end of the buffer. I can't see how this can > reasonably be avoided I don't know which way is better, but you may check how it was done in the original code. First it was checking for enough space, and only after filling the destination buffer. fs/proc/array.c, for example, originally might leave the back slash at the end. > while still doing a meaningful refactorization > preparing it for the next patch. > > Rasmus -- Andy Shevchenko <andriy.shevchenko@intel.com> Intel Finland Oy ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] lib/string_helpers.c: Refactor string_escape_mem 2015-01-29 13:37 ` Andy Shevchenko @ 2015-01-29 19:33 ` Jeff Epler 2015-01-30 10:14 ` Andy Shevchenko 0 siblings, 1 reply; 52+ messages in thread From: Jeff Epler @ 2015-01-29 19:33 UTC (permalink / raw) To: Andy Shevchenko Cc: Rasmus Villemoes, Andrew Morton, Mathias Krause, linux-kernel [discussing the repeated three-line idiom] > if (out < end) > *out = '\\'; > ++out; Instead of open-coding this each time, perhaps it would be appropriate to define a macro to possibly put a character and also advance the pointer. Assuming that the locals are consistently "out" and "end", something like #define addch(c) do { \ if(out < end) *out = c; \ ++out; \ } while(0) modulo any errors or differences with standard kernel coding style. Jeff ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 2/3] lib/string_helpers.c: Refactor string_escape_mem 2015-01-29 19:33 ` Jeff Epler @ 2015-01-30 10:14 ` Andy Shevchenko 0 siblings, 0 replies; 52+ messages in thread From: Andy Shevchenko @ 2015-01-30 10:14 UTC (permalink / raw) To: Jeff Epler; +Cc: Rasmus Villemoes, Andrew Morton, Mathias Krause, linux-kernel On Thu, 2015-01-29 at 13:33 -0600, Jeff Epler wrote: > [discussing the repeated three-line idiom] > > if (out < end) > > *out = '\\'; > > ++out; > > Instead of open-coding this each time, perhaps it would be appropriate > to define a macro to possibly put a character and also advance the > pointer. > > Assuming that the locals are consistently "out" and "end", something > like Why not inline function? > > #define addch(c) do { \ > if(out < end) *out = c; \ > ++out; \ > } while(0) > > modulo any errors or differences with standard kernel coding style. What about vsprintf.c ? -- Andy Shevchenko <andriy.shevchenko@intel.com> Intel Finland Oy ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 3/3] lib/string_helpers.c: Change semantics of string_escape_mem 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:03 ` [PATCH v2 2/3] lib/string_helpers.c: Refactor string_escape_mem Rasmus Villemoes @ 2015-01-29 10:03 ` Rasmus Villemoes 2015-01-29 13:29 ` Andy Shevchenko 2015-02-09 23:44 ` Rasmus Villemoes 3 siblings, 1 reply; 52+ messages in thread From: Rasmus Villemoes @ 2015-01-29 10:03 UTC (permalink / raw) To: Andy Shevchenko, Andrew Morton, Trond Myklebust, J. Bruce Fields, David S. Miller Cc: Rasmus Villemoes, linux-kernel, linux-nfs, netdev The current semantics of string_escape_mem are inadequate for one of its two current users, vsnprintf(). If that is to honour its contract, it must know how much space would be needed for the entire escaped buffer, and string_escape_mem provides no way of obtaining that (short of allocating a large enough buffer (~4 times input string) to let it play with, and that's definitely a big no-no inside vsnprintf). So change the semantics for string_escape_mem to be more snprintf-like: Return the size of the output that would be generated if the destination buffer was big enough, but of course still only write to the part of dst it is allowed to, and don't do '\0'-termination. It is then up to the caller to detect whether output was truncated and to append a '\0' if desired. This also fixes a bug in the escaped_string() helper function, which used to unconditionally pass a length of "end-buf" to string_escape_mem(); since the latter doesn't check osz for being insanely large, it would happily write to dst. For example, kasprintf(GFP_KERNEL, "something and then %pE", ...); is an easy way to trigger an oops. In test-string_helpers.c, I removed the now meaningless -ENOMEM test, and replaced it with testing for getting the expected return value even if the buffer is too small. Also ensure that nothing is written when osz==0. In net/sunrpc/cache.c, I think qword_add still has the same semantics. Someone should definitely double-check this. Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> --- include/linux/string_helpers.h | 10 +++++----- lib/string_helpers.c | 25 ++++++++----------------- lib/test-string_helpers.c | 37 ++++++++++++++++--------------------- lib/vsprintf.c | 2 +- net/sunrpc/cache.c | 8 +++++--- 5 files changed, 35 insertions(+), 47 deletions(-) diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h index 6eb567ac56bc..7a082aa183a8 100644 --- a/include/linux/string_helpers.h +++ b/include/linux/string_helpers.h @@ -47,22 +47,22 @@ static inline int string_unescape_any_inplace(char *buf) #define ESCAPE_ANY_NP (ESCAPE_ANY | ESCAPE_NP) #define ESCAPE_HEX 0x20 -int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, +size_t string_escape_mem(const char *src, size_t isz, char *dst, size_t osz, unsigned int flags, const char *esc); -static inline int string_escape_mem_any_np(const char *src, size_t isz, - char **dst, size_t osz, const char *esc) +static inline size_t string_escape_mem_any_np(const char *src, size_t isz, + char *dst, size_t osz, const char *esc) { return string_escape_mem(src, isz, dst, osz, ESCAPE_ANY_NP, esc); } -static inline int string_escape_str(const char *src, char **dst, size_t sz, +static inline size_t string_escape_str(const char *src, char *dst, size_t sz, unsigned int flags, const char *esc) { return string_escape_mem(src, strlen(src), dst, sz, flags, esc); } -static inline int string_escape_str_any_np(const char *src, char **dst, +static inline size_t string_escape_str_any_np(const char *src, char *dst, size_t sz, const char *esc) { return string_escape_str(src, dst, sz, ESCAPE_ANY_NP, esc); diff --git a/lib/string_helpers.c b/lib/string_helpers.c index e14dd8555760..05d4a583d20e 100644 --- a/lib/string_helpers.c +++ b/lib/string_helpers.c @@ -414,20 +414,17 @@ static bool escape_hex(unsigned char c, char **dst, char *end) * it if needs. * * Return: - * The amount of the characters processed to the destination buffer, or - * %-ENOMEM if the size of buffer is not enough to put an escaped character is - * returned. - * - * Even in the case of error @dst pointer will be updated to point to the byte - * after the last processed character. + * The total size of the escaped output that would be generated for + * the given input and flags. To check whether the output was + * truncated, compare the return value to osz. There is room left in + * dst for a '\0' terminator if and only if ret < osz. */ -int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, - unsigned int flags, const char *esc) +size_t string_escape_mem(const char *src, size_t isz, char *dst, size_t osz, + unsigned int flags, const char *esc) { - char *p = *dst; + char *p = dst; char *end = p + osz; bool is_dict = esc && *esc; - int ret; while (isz--) { unsigned char c = *src++; @@ -466,13 +463,7 @@ int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, escape_passthrough(c, &p, end); } - if (p > end) { - *dst = end; - return -ENOMEM; - } - ret = p - *dst; - *dst = p; - return ret; + return p - dst; } EXPORT_SYMBOL(string_escape_mem); diff --git a/lib/test-string_helpers.c b/lib/test-string_helpers.c index ab0d30e1e18f..5f95114a2f86 100644 --- a/lib/test-string_helpers.c +++ b/lib/test-string_helpers.c @@ -264,12 +264,12 @@ static __init void test_string_escape(const char *name, const struct test_string_2 *s2, unsigned int flags, const char *esc) { - int q_real = 512; - char *out_test = kmalloc(q_real, GFP_KERNEL); - char *out_real = kmalloc(q_real, GFP_KERNEL); + size_t out_size = 512; + char *out_test = kmalloc(out_size, GFP_KERNEL); + char *out_real = kmalloc(out_size, GFP_KERNEL); char *in = kmalloc(256, GFP_KERNEL); - char *buf = out_real; - int p = 0, q_test = 0; + size_t p = 0, q_test = 0; + size_t q_real; if (!out_test || !out_real || !in) goto out; @@ -301,29 +301,26 @@ static __init void test_string_escape(const char *name, q_test += len; } - q_real = string_escape_mem(in, p, &buf, q_real, flags, esc); + q_real = string_escape_mem(in, p, out_real, out_size, flags, esc); test_string_check_buf(name, flags, in, p, out_real, q_real, out_test, q_test); + + 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 %zu, got %zu\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); + out: kfree(in); kfree(out_real); kfree(out_test); } -static __init void test_string_escape_nomem(void) -{ - char *in = "\eb \\C\007\"\x90\r]"; - char out[64], *buf = out; - int rc = -ENOMEM, ret; - - ret = string_escape_str_any_np(in, &buf, strlen(in), NULL); - if (ret == rc) - return; - - pr_err("Test 'escape nomem' failed: got %d instead of %d\n", ret, rc); -} - static int __init test_string_helpers_init(void) { unsigned int i; @@ -342,8 +339,6 @@ static int __init test_string_helpers_init(void) for (i = 0; i < (ESCAPE_ANY_NP | ESCAPE_HEX) + 1; i++) test_string_escape("escape 1", escape1, i, TEST_STRING_2_DICT_1); - test_string_escape_nomem(); - return -EINVAL; } module_init(test_string_helpers_init); diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 3568e3906777..d02c394b5b58 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -1160,7 +1160,7 @@ char *escaped_string(char *buf, char *end, u8 *addr, struct printf_spec spec, len = spec.field_width < 0 ? 1 : spec.field_width; /* Ignore the error. We print as many characters as we can */ - string_escape_mem(addr, len, &buf, end - buf, flags, NULL); + buf += string_escape_mem(addr, len, buf, buf < end ? end - buf : 0, flags, NULL); return buf; } diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c index 33fb105d4352..22c4418057f4 100644 --- a/net/sunrpc/cache.c +++ b/net/sunrpc/cache.c @@ -1068,12 +1068,14 @@ void qword_add(char **bpp, int *lp, char *str) { char *bp = *bpp; int len = *lp; - int ret; + int ret, written; if (len < 0) return; - ret = string_escape_str(str, &bp, len, ESCAPE_OCTAL, "\\ \n\t"); - if (ret < 0 || ret == len) + ret = string_escape_str(str, bp, len, ESCAPE_OCTAL, "\\ \n\t"); + written = min(ret, len); + bp += written; + if (ret >= len) len = -1; else { len -= ret; -- 2.1.3 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH v2 3/3] lib/string_helpers.c: Change semantics of string_escape_mem 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 0 siblings, 1 reply; 52+ messages in thread From: Andy Shevchenko @ 2015-01-29 13:29 UTC (permalink / raw) To: Rasmus Villemoes Cc: Andrew Morton, Trond Myklebust, J. Bruce Fields, David S. Miller, linux-kernel, linux-nfs, netdev On Thu, 2015-01-29 at 11:03 +0100, Rasmus Villemoes wrote: Thanks for an update. Few comments below. > The current semantics of string_escape_mem are inadequate for one of > its two current users, vsnprintf(). If that is to honour its contract, > it must know how much space would be needed for the entire escaped > buffer, and string_escape_mem provides no way of obtaining that (short > of allocating a large enough buffer (~4 times input string) to let it > play with, and that's definitely a big no-no inside vsnprintf). > > So change the semantics for string_escape_mem to be more > snprintf-like: Return the size of the output that would be generated > if the destination buffer was big enough, but of course still only > write to the part of dst it is allowed to, and don't do > '\0'-termination. It is then up to the caller to detect whether output > was truncated and to append a '\0' if desired. > > This also fixes a bug in the escaped_string() helper function, which > used to unconditionally pass a length of "end-buf" to > string_escape_mem(); since the latter doesn't check osz for being > insanely large, it would happily write to dst. For example, > kasprintf(GFP_KERNEL, "something and then %pE", ...); is an easy way > to trigger an oops. > > In test-string_helpers.c, I removed the now meaningless -ENOMEM test, > and replaced it with testing for getting the expected return value > even if the buffer is too small. Also ensure that nothing is written > when osz==0. 'osz == 0' > > In net/sunrpc/cache.c, I think qword_add still has the same > semantics. Someone should definitely double-check this. > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> > --- > include/linux/string_helpers.h | 10 +++++----- > lib/string_helpers.c | 25 ++++++++----------------- > lib/test-string_helpers.c | 37 ++++++++++++++++--------------------- > lib/vsprintf.c | 2 +- > net/sunrpc/cache.c | 8 +++++--- > 5 files changed, 35 insertions(+), 47 deletions(-) > > diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h > index 6eb567ac56bc..7a082aa183a8 100644 > --- a/include/linux/string_helpers.h > +++ b/include/linux/string_helpers.h > @@ -47,22 +47,22 @@ static inline int string_unescape_any_inplace(char *buf) > #define ESCAPE_ANY_NP (ESCAPE_ANY | ESCAPE_NP) > #define ESCAPE_HEX 0x20 > > -int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, > +size_t string_escape_mem(const char *src, size_t isz, char *dst, size_t osz, > unsigned int flags, const char *esc); > > -static inline int string_escape_mem_any_np(const char *src, size_t isz, > - char **dst, size_t osz, const char *esc) > +static inline size_t string_escape_mem_any_np(const char *src, size_t isz, > + char *dst, size_t osz, const char *esc) > { > return string_escape_mem(src, isz, dst, osz, ESCAPE_ANY_NP, esc); > } > > -static inline int string_escape_str(const char *src, char **dst, size_t sz, > +static inline size_t string_escape_str(const char *src, char *dst, size_t sz, > unsigned int flags, const char *esc) > { > return string_escape_mem(src, strlen(src), dst, sz, flags, esc); > } > > -static inline int string_escape_str_any_np(const char *src, char **dst, > +static inline size_t string_escape_str_any_np(const char *src, char *dst, > size_t sz, const char *esc) > { > return string_escape_str(src, dst, sz, ESCAPE_ANY_NP, esc); > diff --git a/lib/string_helpers.c b/lib/string_helpers.c > index e14dd8555760..05d4a583d20e 100644 > --- a/lib/string_helpers.c > +++ b/lib/string_helpers.c > @@ -414,20 +414,17 @@ static bool escape_hex(unsigned char c, char **dst, char *end) > * it if needs. > * > * Return: > - * The amount of the characters processed to the destination buffer, or > - * %-ENOMEM if the size of buffer is not enough to put an escaped character is > - * returned. > - * > - * Even in the case of error @dst pointer will be updated to point to the byte > - * after the last processed character. > + * The total size of the escaped output that would be generated for > + * the given input and flags. To check whether the output was > + * truncated, compare the return value to osz. There is room left in > + * dst for a '\0' terminator if and only if ret < osz. > */ > -int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, > - unsigned int flags, const char *esc) > +size_t string_escape_mem(const char *src, size_t isz, char *dst, size_t osz, > + unsigned int flags, const char *esc) I prefer to leave the prototype the same. int for return is okay. dst should be updated accordingly. > { > - char *p = *dst; > + char *p = dst; > char *end = p + osz; > bool is_dict = esc && *esc; > - int ret; > > while (isz--) { > unsigned char c = *src++; > @@ -466,13 +463,7 @@ int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, > > escape_passthrough(c, &p, end); > } > - if (p > end) { > - *dst = end; > - return -ENOMEM; > - } > > - ret = p - *dst; > - *dst = p; > - return ret; > + return p - dst; > } > EXPORT_SYMBOL(string_escape_mem); > diff --git a/lib/test-string_helpers.c b/lib/test-string_helpers.c > index ab0d30e1e18f..5f95114a2f86 100644 > --- a/lib/test-string_helpers.c > +++ b/lib/test-string_helpers.c > @@ -264,12 +264,12 @@ static __init void test_string_escape(const char *name, > const struct test_string_2 *s2, > unsigned int flags, const char *esc) > { > - int q_real = 512; > - char *out_test = kmalloc(q_real, GFP_KERNEL); > - char *out_real = kmalloc(q_real, GFP_KERNEL); > + size_t out_size = 512; > + char *out_test = kmalloc(out_size, GFP_KERNEL); > + char *out_real = kmalloc(out_size, GFP_KERNEL); > char *in = kmalloc(256, GFP_KERNEL); > - char *buf = out_real; > - int p = 0, q_test = 0; > + size_t p = 0, q_test = 0; > + size_t q_real; > > if (!out_test || !out_real || !in) > goto out; > @@ -301,29 +301,26 @@ static __init void test_string_escape(const char *name, > q_test += len; > } > > - q_real = string_escape_mem(in, p, &buf, q_real, flags, esc); > + q_real = string_escape_mem(in, p, out_real, out_size, flags, esc); > > test_string_check_buf(name, flags, in, p, out_real, q_real, out_test, > q_test); > + > + 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 %zu, got %zu\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); Could it be a part of nomem test still? > + > out: > kfree(in); > kfree(out_real); > kfree(out_test); > } > > -static __init void test_string_escape_nomem(void) > -{ > - char *in = "\eb \\C\007\"\x90\r]"; > - char out[64], *buf = out; > - int rc = -ENOMEM, ret; > - > - ret = string_escape_str_any_np(in, &buf, strlen(in), NULL); > - if (ret == rc) > - return; > - > - pr_err("Test 'escape nomem' failed: got %d instead of %d\n", ret, rc); > -} > - > static int __init test_string_helpers_init(void) > { > unsigned int i; > @@ -342,8 +339,6 @@ static int __init test_string_helpers_init(void) > for (i = 0; i < (ESCAPE_ANY_NP | ESCAPE_HEX) + 1; i++) > test_string_escape("escape 1", escape1, i, TEST_STRING_2_DICT_1); > > - test_string_escape_nomem(); > - > return -EINVAL; > } > module_init(test_string_helpers_init); > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > index 3568e3906777..d02c394b5b58 100644 > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -1160,7 +1160,7 @@ char *escaped_string(char *buf, char *end, u8 *addr, struct printf_spec spec, > len = spec.field_width < 0 ? 1 : spec.field_width; > > /* Ignore the error. We print as many characters as we can */ > - string_escape_mem(addr, len, &buf, end - buf, flags, NULL); > + buf += string_escape_mem(addr, len, buf, buf < end ? end - buf : 0, flags, NULL); So, the problem is when we have end < buf, right? How about to move this check out of the call parameters? [Keep in might the original prototype] if (buf < end) string_escape_mem(addr, len, &buf, end - buf, flags, NULL); else string_escape_mem(addr, len, &buf, 0, flags, NULL); I don't know if it's a good idea to check this inside string_escape_mem(). Possible not. > > return buf; > } > diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c > index 33fb105d4352..22c4418057f4 100644 > --- a/net/sunrpc/cache.c > +++ b/net/sunrpc/cache.c > @@ -1068,12 +1068,14 @@ void qword_add(char **bpp, int *lp, char *str) > { > char *bp = *bpp; > int len = *lp; > - int ret; > + int ret, written; > > if (len < 0) return; > > - ret = string_escape_str(str, &bp, len, ESCAPE_OCTAL, "\\ \n\t"); > - if (ret < 0 || ret == len) > + ret = string_escape_str(str, bp, len, ESCAPE_OCTAL, "\\ \n\t"); > + written = min(ret, len); > + bp += written; > + if (ret >= len) > len = -1; > else { > len -= ret; For this part the comment from J. Bruce is needed. There is one more user, i.e. fs/proc/array.c::task_name(). In all of them we have to amend a prepend commentary. Like changing 'Ignore the error' to 'Ignore the overflow'. -- Andy Shevchenko <andriy.shevchenko@intel.com> Intel Finland Oy ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 3/3] lib/string_helpers.c: Change semantics of string_escape_mem 2015-01-29 13:29 ` Andy Shevchenko @ 2015-01-29 14:29 ` Rasmus Villemoes 2015-01-30 10:27 ` Andy Shevchenko 0 siblings, 1 reply; 52+ messages in thread From: Rasmus Villemoes @ 2015-01-29 14:29 UTC (permalink / raw) To: Andy Shevchenko Cc: Andrew Morton, Trond Myklebust, J. Bruce Fields, David S. Miller, linux-kernel, linux-nfs, netdev On Thu, Jan 29 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: >> * >> * Return: >> - * The amount of the characters processed to the destination buffer, or >> - * %-ENOMEM if the size of buffer is not enough to put an escaped character is >> - * returned. >> - * >> - * Even in the case of error @dst pointer will be updated to point to the byte >> - * after the last processed character. >> + * The total size of the escaped output that would be generated for >> + * the given input and flags. To check whether the output was >> + * truncated, compare the return value to osz. There is room left in >> + * dst for a '\0' terminator if and only if ret < osz. >> */ >> -int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, >> - unsigned int flags, const char *esc) >> +size_t string_escape_mem(const char *src, size_t isz, char *dst, size_t osz, >> + unsigned int flags, const char *esc) > > I prefer to leave the prototype the same. int for return is okay. dst > should be updated accordingly. Please explain exactly what you think the return value should be, and what *dst should be set to. >> { >> - char *p = *dst; >> + char *p = dst; >> char *end = p + osz; >> bool is_dict = esc && *esc; >> - int ret; >> >> while (isz--) { >> unsigned char c = *src++; >> @@ -466,13 +463,7 @@ int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, >> >> escape_passthrough(c, &p, end); >> } >> - if (p > end) { >> - *dst = end; >> - return -ENOMEM; >> - } >> >> - ret = p - *dst; >> - *dst = p; >> - return ret; >> + return p - dst; >> } >> EXPORT_SYMBOL(string_escape_mem); >> diff --git a/lib/test-string_helpers.c b/lib/test-string_helpers.c >> index ab0d30e1e18f..5f95114a2f86 100644 >> --- a/lib/test-string_helpers.c >> +++ b/lib/test-string_helpers.c >> @@ -264,12 +264,12 @@ static __init void test_string_escape(const char *name, >> const struct test_string_2 *s2, >> unsigned int flags, const char *esc) >> { >> - int q_real = 512; >> - char *out_test = kmalloc(q_real, GFP_KERNEL); >> - char *out_real = kmalloc(q_real, GFP_KERNEL); >> + size_t out_size = 512; >> + char *out_test = kmalloc(out_size, GFP_KERNEL); >> + char *out_real = kmalloc(out_size, GFP_KERNEL); >> char *in = kmalloc(256, GFP_KERNEL); >> - char *buf = out_real; >> - int p = 0, q_test = 0; >> + size_t p = 0, q_test = 0; >> + size_t q_real; >> >> if (!out_test || !out_real || !in) >> goto out; >> @@ -301,29 +301,26 @@ static __init void test_string_escape(const char *name, >> q_test += len; >> } >> >> - q_real = string_escape_mem(in, p, &buf, q_real, flags, esc); >> + q_real = string_escape_mem(in, p, out_real, out_size, flags, esc); >> >> test_string_check_buf(name, flags, in, p, out_real, q_real, out_test, >> q_test); >> + >> + 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 %zu, got %zu\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); > > Could it be a part of nomem test still? What nomem test? string_escape_mem with snprintf-like semantics cannot return an error; that has to be checked by the caller. >> + >> out: >> kfree(in); >> kfree(out_real); >> kfree(out_test); >> } >> >> -static __init void test_string_escape_nomem(void) >> -{ >> - char *in = "\eb \\C\007\"\x90\r]"; >> - char out[64], *buf = out; >> - int rc = -ENOMEM, ret; >> - >> - ret = string_escape_str_any_np(in, &buf, strlen(in), NULL); >> - if (ret == rc) >> - return; >> - >> - pr_err("Test 'escape nomem' failed: got %d instead of %d\n", ret, rc); >> -} >> - >> static int __init test_string_helpers_init(void) >> { >> unsigned int i; >> @@ -342,8 +339,6 @@ static int __init test_string_helpers_init(void) >> for (i = 0; i < (ESCAPE_ANY_NP | ESCAPE_HEX) + 1; i++) >> test_string_escape("escape 1", escape1, i, TEST_STRING_2_DICT_1); >> >> - test_string_escape_nomem(); >> - >> return -EINVAL; >> } >> module_init(test_string_helpers_init); >> diff --git a/lib/vsprintf.c b/lib/vsprintf.c >> index 3568e3906777..d02c394b5b58 100644 >> --- a/lib/vsprintf.c >> +++ b/lib/vsprintf.c >> @@ -1160,7 +1160,7 @@ char *escaped_string(char *buf, char *end, u8 *addr, struct printf_spec spec, >> len = spec.field_width < 0 ? 1 : spec.field_width; >> >> /* Ignore the error. We print as many characters as we can */ >> - string_escape_mem(addr, len, &buf, end - buf, flags, NULL); >> + buf += string_escape_mem(addr, len, buf, buf < end ? end - buf : 0, flags, NULL); > > So, the problem is when we have end < buf, right? > How about to move this check out of the call parameters? > > [Keep in might the original prototype] > > if (buf < end) > string_escape_mem(addr, len, &buf, end - buf, flags, NULL); > else > string_escape_mem(addr, len, &buf, 0, flags, NULL); In that case, I just did the same as is done for %pV, and prefer to keep it that way. >> diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c >> index 33fb105d4352..22c4418057f4 100644 >> --- a/net/sunrpc/cache.c >> +++ b/net/sunrpc/cache.c >> @@ -1068,12 +1068,14 @@ void qword_add(char **bpp, int *lp, char *str) >> { >> char *bp = *bpp; >> int len = *lp; >> - int ret; >> + int ret, written; >> >> if (len < 0) return; >> >> - ret = string_escape_str(str, &bp, len, ESCAPE_OCTAL, "\\ \n\t"); >> - if (ret < 0 || ret == len) >> + ret = string_escape_str(str, bp, len, ESCAPE_OCTAL, "\\ \n\t"); >> + written = min(ret, len); >> + bp += written; >> + if (ret >= len) >> len = -1; >> else { >> len -= ret; > > For this part the comment from J. Bruce is needed. > > There is one more user, i.e. fs/proc/array.c::task_name(). > > In all of them we have to amend a prepend commentary. Like changing > 'Ignore the error' to 'Ignore the overflow'. I hadn't looked for users in -next. I'll leave it to you to amend that patch before it hits mainline. Rasmus ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 3/3] lib/string_helpers.c: Change semantics of string_escape_mem 2015-01-29 14:29 ` Rasmus Villemoes @ 2015-01-30 10:27 ` Andy Shevchenko 2015-01-30 23:39 ` Rasmus Villemoes 0 siblings, 1 reply; 52+ messages in thread From: Andy Shevchenko @ 2015-01-30 10:27 UTC (permalink / raw) To: Rasmus Villemoes Cc: Andrew Morton, Trond Myklebust, J. Bruce Fields, David S. Miller, linux-kernel, linux-nfs, netdev On Thu, 2015-01-29 at 15:29 +0100, Rasmus Villemoes wrote: > On Thu, Jan 29 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > >> * > >> * Return: > >> - * The amount of the characters processed to the destination buffer, or > >> - * %-ENOMEM if the size of buffer is not enough to put an escaped character is > >> - * returned. > >> - * > >> - * Even in the case of error @dst pointer will be updated to point to the byte > >> - * after the last processed character. > >> + * The total size of the escaped output that would be generated for > >> + * the given input and flags. To check whether the output was > >> + * truncated, compare the return value to osz. There is room left in > >> + * dst for a '\0' terminator if and only if ret < osz. > >> */ > >> -int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, > >> - unsigned int flags, const char *esc) > >> +size_t string_escape_mem(const char *src, size_t isz, char *dst, size_t osz, > >> + unsigned int flags, const char *esc) > > > > I prefer to leave the prototype the same. int for return is okay. dst > > should be updated accordingly. > > Please explain exactly what you think the return value should be, and > what *dst should be set to. Return value like you proposed, *dst is incremented by it. > > >> { > >> - char *p = *dst; > >> + char *p = dst; > >> char *end = p + osz; > >> bool is_dict = esc && *esc; > >> - int ret; > >> > >> while (isz--) { > >> unsigned char c = *src++; > >> @@ -466,13 +463,7 @@ int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, > >> > >> escape_passthrough(c, &p, end); > >> } > >> - if (p > end) { > >> - *dst = end; > >> - return -ENOMEM; > >> - } > >> > >> - ret = p - *dst; > >> - *dst = p; > >> - return ret; > >> + return p - dst; > >> } > >> EXPORT_SYMBOL(string_escape_mem); > >> diff --git a/lib/test-string_helpers.c b/lib/test-string_helpers.c > >> index ab0d30e1e18f..5f95114a2f86 100644 > >> --- a/lib/test-string_helpers.c > >> +++ b/lib/test-string_helpers.c > >> @@ -264,12 +264,12 @@ static __init void test_string_escape(const char *name, > >> const struct test_string_2 *s2, > >> unsigned int flags, const char *esc) > >> { > >> - int q_real = 512; > >> - char *out_test = kmalloc(q_real, GFP_KERNEL); > >> - char *out_real = kmalloc(q_real, GFP_KERNEL); > >> + size_t out_size = 512; > >> + char *out_test = kmalloc(out_size, GFP_KERNEL); > >> + char *out_real = kmalloc(out_size, GFP_KERNEL); > >> char *in = kmalloc(256, GFP_KERNEL); > >> - char *buf = out_real; > >> - int p = 0, q_test = 0; > >> + size_t p = 0, q_test = 0; > >> + size_t q_real; > >> > >> if (!out_test || !out_real || !in) > >> goto out; > >> @@ -301,29 +301,26 @@ static __init void test_string_escape(const char *name, > >> q_test += len; > >> } > >> > >> - q_real = string_escape_mem(in, p, &buf, q_real, flags, esc); > >> + q_real = string_escape_mem(in, p, out_real, out_size, flags, esc); > >> > >> test_string_check_buf(name, flags, in, p, out_real, q_real, out_test, > >> q_test); > >> + > >> + 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 %zu, got %zu\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); > > > > Could it be a part of nomem test still? > > What nomem test? string_escape_mem with snprintf-like semantics cannot > return an error; that has to be checked by the caller. Make this code a separate test, which actually still nomem, since you have not enough memory in the destination buffer. What the problem to check for proper return value and the last couple of characters written to the destination buffer? > > >> + > >> out: > >> kfree(in); > >> kfree(out_real); > >> kfree(out_test); > >> } > >> > >> -static __init void test_string_escape_nomem(void) > >> -{ > >> - char *in = "\eb \\C\007\"\x90\r]"; > >> - char out[64], *buf = out; > >> - int rc = -ENOMEM, ret; > >> - > >> - ret = string_escape_str_any_np(in, &buf, strlen(in), NULL); > >> - if (ret == rc) > >> - return; > >> - > >> - pr_err("Test 'escape nomem' failed: got %d instead of %d\n", ret, rc); > >> -} > >> - > >> static int __init test_string_helpers_init(void) > >> { > >> unsigned int i; > >> @@ -342,8 +339,6 @@ static int __init test_string_helpers_init(void) > >> for (i = 0; i < (ESCAPE_ANY_NP | ESCAPE_HEX) + 1; i++) > >> test_string_escape("escape 1", escape1, i, TEST_STRING_2_DICT_1); > >> > >> - test_string_escape_nomem(); > >> - > >> return -EINVAL; > >> } > >> module_init(test_string_helpers_init); > >> diff --git a/lib/vsprintf.c b/lib/vsprintf.c > >> index 3568e3906777..d02c394b5b58 100644 > >> --- a/lib/vsprintf.c > >> +++ b/lib/vsprintf.c > >> @@ -1160,7 +1160,7 @@ char *escaped_string(char *buf, char *end, u8 *addr, struct printf_spec spec, > >> len = spec.field_width < 0 ? 1 : spec.field_width; > >> > >> /* Ignore the error. We print as many characters as we can */ > >> - string_escape_mem(addr, len, &buf, end - buf, flags, NULL); > >> + buf += string_escape_mem(addr, len, buf, buf < end ? end - buf : 0, flags, NULL); > > > > So, the problem is when we have end < buf, right? > > How about to move this check out of the call parameters? > > > > [Keep in might the original prototype] > > > > if (buf < end) > > string_escape_mem(addr, len, &buf, end - buf, flags, NULL); > > else > > string_escape_mem(addr, len, &buf, 0, flags, NULL); > > In that case, I just did the same as is done for %pV, and prefer to keep > it that way. I've checked the other case, we may keep same style. > >> diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c > >> index 33fb105d4352..22c4418057f4 100644 > >> --- a/net/sunrpc/cache.c > >> +++ b/net/sunrpc/cache.c > >> @@ -1068,12 +1068,14 @@ void qword_add(char **bpp, int *lp, char *str) > >> { > >> char *bp = *bpp; > >> int len = *lp; > >> - int ret; > >> + int ret, written; > >> > >> if (len < 0) return; > >> > >> - ret = string_escape_str(str, &bp, len, ESCAPE_OCTAL, "\\ \n\t"); > >> - if (ret < 0 || ret == len) > >> + ret = string_escape_str(str, bp, len, ESCAPE_OCTAL, "\\ \n\t"); > >> + written = min(ret, len); > >> + bp += written; > >> + if (ret >= len) > >> len = -1; > >> else { > >> len -= ret; > > > > For this part the comment from J. Bruce is needed. > > > > There is one more user, i.e. fs/proc/array.c::task_name(). > > > > In all of them we have to amend a prepend commentary. Like changing > > 'Ignore the error' to 'Ignore the overflow'. > > I hadn't looked for users in -next. I'll leave it to you to amend that > patch before it hits mainline. When your series will be ready (and actually I recommend to push first patch apart from the rest since it's not related) I may do the update for fs/proc/array.c. -- Andy Shevchenko <andriy.shevchenko@intel.com> Intel Finland Oy ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 3/3] lib/string_helpers.c: Change semantics of string_escape_mem @ 2015-01-30 23:39 ` Rasmus Villemoes 0 siblings, 0 replies; 52+ messages in thread From: Rasmus Villemoes @ 2015-01-30 23:39 UTC (permalink / raw) To: Andy Shevchenko Cc: Andrew Morton, Trond Myklebust, J. Bruce Fields, David S. Miller, linux-kernel, linux-nfs, netdev On Fri, Jan 30 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Thu, 2015-01-29 at 15:29 +0100, Rasmus Villemoes wrote: >> On Thu, Jan 29 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: >> >> >> * >> >> * Return: >> >> - * The amount of the characters processed to the destination buffer, or >> >> - * %-ENOMEM if the size of buffer is not enough to put an escaped character is >> >> - * returned. >> >> - * >> >> - * Even in the case of error @dst pointer will be updated to point to the byte >> >> - * after the last processed character. >> >> + * The total size of the escaped output that would be generated for >> >> + * the given input and flags. To check whether the output was >> >> + * truncated, compare the return value to osz. There is room left in >> >> + * dst for a '\0' terminator if and only if ret < osz. >> >> */ >> >> -int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, >> >> - unsigned int flags, const char *esc) >> >> +size_t string_escape_mem(const char *src, size_t isz, char *dst, size_t osz, >> >> + unsigned int flags, const char *esc) >> > >> > I prefer to leave the prototype the same. int for return is okay. dst >> > should be updated accordingly. >> >> Please explain exactly what you think the return value should be, and >> what *dst should be set to. > > Return value like you proposed, *dst is incremented by it. The more I think about it, the less I like having dst being char**. (1) It is unlike most other functions taking buffer+size arguments. (2) It is inconvenient. Callers doing char escaped[SOME_SIZE]; or char *escaped = kmalloc(likely_big_enough); can't just pass &escaped; they would need to use an extra dummy variable. (3) With the return value telling the size of the would-be generated output, it is redundant. In fact, I dislike it so much that I'm not going to sign off on a patch where dst is still char**. >> > Could it be a part of nomem test still? >> >> What nomem test? string_escape_mem with snprintf-like semantics cannot >> return an error; that has to be checked by the caller. > > Make this code a separate test, which actually still nomem, since you > have not enough memory in the destination buffer. What the problem to > check for proper return value and the last couple of characters written > to the destination buffer? Sure, it could go into a separate helper. Anyway, the semantics of string_escape_mem needs to be decided before it makes sense to update the tests. > When your series will be ready (and actually I recommend to push first > patch apart from the rest since it's not related) I agree on that part. Andrew, could you take 1/3 (http://thread.gmane.org/gmane.linux.kernel/1876841/focus=348404), and then we'll see when the %pE case gets sorted out. Rasmus ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 3/3] lib/string_helpers.c: Change semantics of string_escape_mem @ 2015-01-30 23:39 ` Rasmus Villemoes 0 siblings, 0 replies; 52+ messages in thread From: Rasmus Villemoes @ 2015-01-30 23:39 UTC (permalink / raw) To: Andy Shevchenko Cc: Andrew Morton, Trond Myklebust, J. Bruce Fields, David S. Miller, linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-nfs-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA On Fri, Jan 30 2015, Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote: > On Thu, 2015-01-29 at 15:29 +0100, Rasmus Villemoes wrote: >> On Thu, Jan 29 2015, Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote: >> >> >> * >> >> * Return: >> >> - * The amount of the characters processed to the destination buffer, or >> >> - * %-ENOMEM if the size of buffer is not enough to put an escaped character is >> >> - * returned. >> >> - * >> >> - * Even in the case of error @dst pointer will be updated to point to the byte >> >> - * after the last processed character. >> >> + * The total size of the escaped output that would be generated for >> >> + * the given input and flags. To check whether the output was >> >> + * truncated, compare the return value to osz. There is room left in >> >> + * dst for a '\0' terminator if and only if ret < osz. >> >> */ >> >> -int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, >> >> - unsigned int flags, const char *esc) >> >> +size_t string_escape_mem(const char *src, size_t isz, char *dst, size_t osz, >> >> + unsigned int flags, const char *esc) >> > >> > I prefer to leave the prototype the same. int for return is okay. dst >> > should be updated accordingly. >> >> Please explain exactly what you think the return value should be, and >> what *dst should be set to. > > Return value like you proposed, *dst is incremented by it. The more I think about it, the less I like having dst being char**. (1) It is unlike most other functions taking buffer+size arguments. (2) It is inconvenient. Callers doing char escaped[SOME_SIZE]; or char *escaped = kmalloc(likely_big_enough); can't just pass &escaped; they would need to use an extra dummy variable. (3) With the return value telling the size of the would-be generated output, it is redundant. In fact, I dislike it so much that I'm not going to sign off on a patch where dst is still char**. >> > Could it be a part of nomem test still? >> >> What nomem test? string_escape_mem with snprintf-like semantics cannot >> return an error; that has to be checked by the caller. > > Make this code a separate test, which actually still nomem, since you > have not enough memory in the destination buffer. What the problem to > check for proper return value and the last couple of characters written > to the destination buffer? Sure, it could go into a separate helper. Anyway, the semantics of string_escape_mem needs to be decided before it makes sense to update the tests. > When your series will be ready (and actually I recommend to push first > patch apart from the rest since it's not related) I agree on that part. Andrew, could you take 1/3 (http://thread.gmane.org/gmane.linux.kernel/1876841/focus=348404), and then we'll see when the %pE case gets sorted out. 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v2 3/3] lib/string_helpers.c: Change semantics of string_escape_mem 2015-01-30 23:39 ` Rasmus Villemoes (?) @ 2015-02-02 10:56 ` Andy Shevchenko -1 siblings, 0 replies; 52+ messages in thread From: Andy Shevchenko @ 2015-02-02 10:56 UTC (permalink / raw) To: Rasmus Villemoes Cc: Andrew Morton, Trond Myklebust, J. Bruce Fields, David S. Miller, linux-kernel, linux-nfs, netdev On Sat, 2015-01-31 at 00:39 +0100, Rasmus Villemoes wrote: > On Fri, Jan 30 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > >> >> * Return: > >> >> - * The amount of the characters processed to the destination buffer, or > >> >> - * %-ENOMEM if the size of buffer is not enough to put an escaped character is > >> >> - * returned. > >> >> - * > >> >> - * Even in the case of error @dst pointer will be updated to point to the byte > >> >> - * after the last processed character. > >> >> + * The total size of the escaped output that would be generated for > >> >> + * the given input and flags. To check whether the output was > >> >> + * truncated, compare the return value to osz. There is room left in > >> >> + * dst for a '\0' terminator if and only if ret < osz. > >> >> */ > >> >> -int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, > >> >> - unsigned int flags, const char *esc) > >> >> +size_t string_escape_mem(const char *src, size_t isz, char *dst, size_t osz, > >> >> + unsigned int flags, const char *esc) > >> > > >> > I prefer to leave the prototype the same. int for return is okay. dst > >> > should be updated accordingly. > >> > >> Please explain exactly what you think the return value should be, and > >> what *dst should be set to. > > > > Return value like you proposed, *dst is incremented by it. > > The more I think about it, the less I like having dst being char**. > > (1) It is unlike most other functions taking buffer+size arguments. > (2) It is inconvenient. Callers doing > > char escaped[SOME_SIZE]; > > or > > char *escaped = kmalloc(likely_big_enough); > > can't just pass &escaped; they would need to use an extra dummy variable. > > (3) With the return value telling the size of the would-be generated > output, it is redundant. > > In fact, I dislike it so much that I'm not going to sign off on a patch > where dst is still char**. Fair enough, though there usual case when temporary variable is already present (at least current users). Let's proceed with char *dst. -- Andy Shevchenko <andriy.shevchenko@intel.com> Intel Finland Oy ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v3 0/3] Two printf fixes @ 2015-02-09 23:44 ` Rasmus Villemoes 0 siblings, 0 replies; 52+ messages in thread From: Rasmus Villemoes @ 2015-02-09 23:44 UTC (permalink / raw) To: Andrew Morton, Andy Shevchenko, Trond Myklebust, J. Bruce Fields, David S. Miller Cc: Rasmus Villemoes, linux-kernel, linux-nfs, netdev Both %pE and %ph are unusable in kasprintf(), since the occurrence of either will trigger an oops during the first vsnprintf call where kasprintf tries to find the correct size to allocate. These oopses could be papered over with somewhat smaller patches than these, but then the return value from vsnprintf would still not reflect the actual size needed. For %pE, this requires a change of semantics of string_escape_mem and hence an annoyingly large diffstat. Not changed in v3: The test_string_escape_nomem helper is still gone, and the overflow test done in test_string_escape. I also kept the "if (out + 1 < end)" conditionals that way. Changed in v3: * Add Andy's ack to 1/3. * Ensure that string_escape_mem doesn't output partial escape sequences after 2/3, while still preparing for it to do exactly that in 3/3. * Leave the return value of string_escape_mem as int. v2: Suggestions from Andy Shevchenko: * Simpler fix of hex_string(). * The string_escape_mem change is split in two, 2/3 updating the internal helpers and 3/3 then changing the external interface. Rasmus Villemoes (3): lib/vsprintf.c: Fix potential NULL deref in hex_string lib/string_helpers.c: Refactor string_escape_mem lib/string_helpers.c: Change semantics of string_escape_mem include/linux/string_helpers.h | 8 +- lib/string_helpers.c | 189 ++++++++++++++++------------------------- lib/test-string_helpers.c | 35 ++++---- lib/vsprintf.c | 24 ++++-- net/sunrpc/cache.c | 8 +- 5 files changed, 113 insertions(+), 151 deletions(-) -- 2.1.3 ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v3 0/3] Two printf fixes @ 2015-02-09 23:44 ` Rasmus Villemoes 0 siblings, 0 replies; 52+ messages in thread From: Rasmus Villemoes @ 2015-02-09 23:44 UTC (permalink / raw) To: Andrew Morton, Andy Shevchenko, Trond Myklebust, J. Bruce Fields, David S. Miller Cc: Rasmus Villemoes, linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-nfs-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA Both %pE and %ph are unusable in kasprintf(), since the occurrence of either will trigger an oops during the first vsnprintf call where kasprintf tries to find the correct size to allocate. These oopses could be papered over with somewhat smaller patches than these, but then the return value from vsnprintf would still not reflect the actual size needed. For %pE, this requires a change of semantics of string_escape_mem and hence an annoyingly large diffstat. Not changed in v3: The test_string_escape_nomem helper is still gone, and the overflow test done in test_string_escape. I also kept the "if (out + 1 < end)" conditionals that way. Changed in v3: * Add Andy's ack to 1/3. * Ensure that string_escape_mem doesn't output partial escape sequences after 2/3, while still preparing for it to do exactly that in 3/3. * Leave the return value of string_escape_mem as int. v2: Suggestions from Andy Shevchenko: * Simpler fix of hex_string(). * The string_escape_mem change is split in two, 2/3 updating the internal helpers and 3/3 then changing the external interface. Rasmus Villemoes (3): lib/vsprintf.c: Fix potential NULL deref in hex_string lib/string_helpers.c: Refactor string_escape_mem lib/string_helpers.c: Change semantics of string_escape_mem include/linux/string_helpers.h | 8 +- lib/string_helpers.c | 189 ++++++++++++++++------------------------- lib/test-string_helpers.c | 35 ++++---- lib/vsprintf.c | 24 ++++-- net/sunrpc/cache.c | 8 +- 5 files changed, 113 insertions(+), 151 deletions(-) -- 2.1.3 -- 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v3 1/3] lib/vsprintf.c: Fix potential NULL deref in hex_string 2015-02-09 23:44 ` Rasmus Villemoes (?) @ 2015-02-09 23:44 ` Rasmus Villemoes -1 siblings, 0 replies; 52+ messages in thread From: Rasmus Villemoes @ 2015-02-09 23:44 UTC (permalink / raw) To: Andrew Morton, Andy Shevchenko, Jiri Kosina, Randy Dunlap, Masanari Iida Cc: Rasmus Villemoes, linux-kernel The helper hex_string() is broken in two ways. First, it doesn't increment buf regardless of whether there is room to print, so callers such as kasprintf() that try to probe the correct storage to allocate will get a too small return value. But even worse, kasprintf() (and likely anyone else trying to find the size of the result) pass NULL for buf and 0 for size, so we also have end == NULL. But this means that the end-1 in hex_string() is (char*)-1, so buf < end-1 is true and we get a NULL pointer deref. I double-checked this with a trivial kernel module that just did a kasprintf(GFP_KERNEL, "%14ph", "CrashBoomBang"). Nobody seems to be using %ph with kasprintf, but we might as well fix it before it hits someone. Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> --- lib/vsprintf.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/lib/vsprintf.c b/lib/vsprintf.c index ec337f64f52d..3568e3906777 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -782,11 +782,19 @@ char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec, if (spec.field_width > 0) len = min_t(int, spec.field_width, 64); - for (i = 0; i < len && buf < end - 1; i++) { - buf = hex_byte_pack(buf, addr[i]); + for (i = 0; i < len; ++i) { + if (buf < end) + *buf = hex_asc_hi(addr[i]); + ++buf; + if (buf < end) + *buf = hex_asc_lo(addr[i]); + ++buf; - if (buf < end && separator && i != len - 1) - *buf++ = separator; + if (separator && i != len - 1) { + if (buf < end) + *buf = separator; + ++buf; + } } return buf; -- 2.1.3 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH v3 2/3] lib/string_helpers.c: Refactor string_escape_mem 2015-02-09 23:44 ` Rasmus Villemoes (?) (?) @ 2015-02-09 23:44 ` Rasmus Villemoes 2015-02-10 12:16 ` Andy Shevchenko -1 siblings, 1 reply; 52+ messages in thread From: Rasmus Villemoes @ 2015-02-09 23:44 UTC (permalink / raw) To: Andrew Morton, Andy Shevchenko, Mathias Krause Cc: Rasmus Villemoes, linux-kernel When printf is given the format specifier %pE, it needs a way of obtaining the total output size that would be generated if the buffer was large enough, and string_escape_mem doesn't easily provide that. This is a refactorization of string_escape_mem in preparation of changing its external API to provide that information. The somewhat ugly 'goto skip;'s and subsequent seemingly redundant conditionals are to make the following patch touch as little as possible in string_helpers.c while still preserving the current behaviour of never outputting partial escape sequences. That behaviour must also change for %pE to work as one expects from every other printf specifier. Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> --- lib/string_helpers.c | 202 +++++++++++++++++++++++---------------------------- 1 file changed, 90 insertions(+), 112 deletions(-) diff --git a/lib/string_helpers.c b/lib/string_helpers.c index 58b78ba57439..7e2fef1eb40e 100644 --- a/lib/string_helpers.c +++ b/lib/string_helpers.c @@ -243,29 +243,21 @@ int string_unescape(char *src, char *dst, size_t size, unsigned int flags) } EXPORT_SYMBOL(string_unescape); -static int escape_passthrough(unsigned char c, char **dst, size_t *osz) +static bool escape_passthrough(unsigned char c, char **dst, char *end) { char *out = *dst; - if (*osz < 1) - return -ENOMEM; - - *out++ = c; - - *dst = out; - *osz -= 1; - - return 1; + if (out < end) + *out = c; + *dst = out + 1; + return true; } -static int escape_space(unsigned char c, char **dst, size_t *osz) +static bool escape_space(unsigned char c, char **dst, char *end) { char *out = *dst; unsigned char to; - if (*osz < 2) - return -ENOMEM; - switch (c) { case '\n': to = 'n'; @@ -283,26 +275,26 @@ static int escape_space(unsigned char c, char **dst, size_t *osz) to = 'f'; break; default: - return 0; + return false; } - *out++ = '\\'; - *out++ = to; - - *dst = out; - *osz -= 2; + if (out + 1 >= end) + goto skip; + if (out + 0 < end) + out[0] = '\\'; + if (out + 1 < end) + out[1] = to; - return 1; +skip: + *dst = out + 2; + return true; } -static int escape_special(unsigned char c, char **dst, size_t *osz) +static bool escape_special(unsigned char c, char **dst, char *end) { char *out = *dst; unsigned char to; - if (*osz < 2) - return -ENOMEM; - switch (c) { case '\\': to = '\\'; @@ -314,71 +306,78 @@ static int escape_special(unsigned char c, char **dst, size_t *osz) to = 'e'; break; default: - return 0; + return false; } - *out++ = '\\'; - *out++ = to; - - *dst = out; - *osz -= 2; + if (out + 1 >= end) + goto skip; + if (out + 0 < end) + out[0] = '\\'; + if (out + 1 < end) + out[1] = to; - return 1; +skip: + *dst = out + 2; + return true; } -static int escape_null(unsigned char c, char **dst, size_t *osz) +static bool escape_null(unsigned char c, char **dst, char *end) { char *out = *dst; - if (*osz < 2) - return -ENOMEM; - if (c) - return 0; - - *out++ = '\\'; - *out++ = '0'; + return false; - *dst = out; - *osz -= 2; + if (out + 1 >= end) + goto skip; + if (out + 0 < end) + out[0] = '\\'; + if (out + 1 < end) + out[1] = '0'; - return 1; +skip: + *dst = out + 2; + return true; } -static int escape_octal(unsigned char c, char **dst, size_t *osz) +static bool escape_octal(unsigned char c, char **dst, char *end) { char *out = *dst; - if (*osz < 4) - return -ENOMEM; - - *out++ = '\\'; - *out++ = ((c >> 6) & 0x07) + '0'; - *out++ = ((c >> 3) & 0x07) + '0'; - *out++ = ((c >> 0) & 0x07) + '0'; - - *dst = out; - *osz -= 4; - - return 1; + if (out + 3 >= end) + goto skip; + if (out + 0 < end) + out[0] = '\\'; + if (out + 1 < end) + out[1] = ((c >> 6) & 0x07) + '0'; + if (out + 2 < end) + out[2] = ((c >> 3) & 0x07) + '0'; + if (out + 3 < end) + out[3] = ((c >> 0) & 0x07) + '0'; + +skip: + *dst = out + 4; + return true; } -static int escape_hex(unsigned char c, char **dst, size_t *osz) +static bool escape_hex(unsigned char c, char **dst, char *end) { char *out = *dst; - if (*osz < 4) - return -ENOMEM; - - *out++ = '\\'; - *out++ = 'x'; - *out++ = hex_asc_hi(c); - *out++ = hex_asc_lo(c); - - *dst = out; - *osz -= 4; - - return 1; + if (out + 3 >= end) + goto skip; + if (out + 0 < end) + out[0] = '\\'; + if (out + 1 < end) + out[1] = 'x'; + if (out + 2 < end) + out[2] = hex_asc_hi(c); + if (out + 3 < end) + out[3] = hex_asc_lo(c); + +skip: + *dst = out + 4; + return true; } /** @@ -440,9 +439,10 @@ static int escape_hex(unsigned char c, char **dst, size_t *osz) int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, unsigned int flags, const char *esc) { - char *out = *dst, *p = out; + char *p = *dst; + char *end = p + osz; bool is_dict = esc && *esc; - int ret = 0; + int ret; while (isz--) { unsigned char c = *src++; @@ -462,55 +462,33 @@ int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, (is_dict && !strchr(esc, c))) { /* do nothing */ } else { - if (flags & ESCAPE_SPACE) { - ret = escape_space(c, &p, &osz); - if (ret < 0) - break; - if (ret > 0) - continue; - } - - if (flags & ESCAPE_SPECIAL) { - ret = escape_special(c, &p, &osz); - if (ret < 0) - break; - if (ret > 0) - continue; - } - - if (flags & ESCAPE_NULL) { - ret = escape_null(c, &p, &osz); - if (ret < 0) - break; - if (ret > 0) - continue; - } + if (flags & ESCAPE_SPACE && escape_space(c, &p, end)) + continue; + + if (flags & ESCAPE_SPECIAL && escape_special(c, &p, end)) + continue; + + if (flags & ESCAPE_NULL && escape_null(c, &p, end)) + continue; /* ESCAPE_OCTAL and ESCAPE_HEX always go last */ - if (flags & ESCAPE_OCTAL) { - ret = escape_octal(c, &p, &osz); - if (ret < 0) - break; + if (flags & ESCAPE_OCTAL && escape_octal(c, &p, end)) continue; - } - if (flags & ESCAPE_HEX) { - ret = escape_hex(c, &p, &osz); - if (ret < 0) - break; + + if (flags & ESCAPE_HEX && escape_hex(c, &p, end)) continue; - } } - ret = escape_passthrough(c, &p, &osz); - if (ret < 0) - break; + escape_passthrough(c, &p, end); } - *dst = p; - - if (ret < 0) - return ret; + if (p > end) { + *dst = end; + return -ENOMEM; + } - return p - out; + ret = p - *dst; + *dst = p; + return ret; } EXPORT_SYMBOL(string_escape_mem); -- 2.1.3 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH v3 2/3] lib/string_helpers.c: Refactor string_escape_mem 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 0 siblings, 0 replies; 52+ messages in thread From: Andy Shevchenko @ 2015-02-10 12:16 UTC (permalink / raw) To: Rasmus Villemoes; +Cc: Andrew Morton, Mathias Krause, linux-kernel On Tue, 2015-02-10 at 00:44 +0100, Rasmus Villemoes wrote: > When printf is given the format specifier %pE, it needs a way of > obtaining the total output size that would be generated if the buffer > was large enough, and string_escape_mem doesn't easily provide > that. This is a refactorization of string_escape_mem in preparation of > changing its external API to provide that information. > > The somewhat ugly 'goto skip;'s and subsequent seemingly redundant > conditionals are to make the following patch touch as little as > possible in string_helpers.c while still preserving the current > behaviour of never outputting partial escape sequences. That behaviour > must also change for %pE to work as one expects from every other > printf specifier. > Thanks for an update. My comment below. > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> > --- > lib/string_helpers.c | 202 +++++++++++++++++++++++---------------------------- > 1 file changed, 90 insertions(+), 112 deletions(-) > > diff --git a/lib/string_helpers.c b/lib/string_helpers.c > index 58b78ba57439..7e2fef1eb40e 100644 > --- a/lib/string_helpers.c > +++ b/lib/string_helpers.c > @@ -243,29 +243,21 @@ int string_unescape(char *src, char *dst, size_t size, unsigned int flags) > } > EXPORT_SYMBOL(string_unescape); > > -static int escape_passthrough(unsigned char c, char **dst, size_t *osz) > +static bool escape_passthrough(unsigned char c, char **dst, char *end) > { > char *out = *dst; > > - if (*osz < 1) > - return -ENOMEM; > - > - *out++ = c; > - > - *dst = out; > - *osz -= 1; > - > - return 1; > + if (out < end) > + *out = c; > + *dst = out + 1; > + return true; > } > > -static int escape_space(unsigned char c, char **dst, size_t *osz) > +static bool escape_space(unsigned char c, char **dst, char *end) > { > char *out = *dst; > unsigned char to; > > - if (*osz < 2) > - return -ENOMEM; > - > switch (c) { > case '\n': > to = 'n'; > @@ -283,26 +275,26 @@ static int escape_space(unsigned char c, char **dst, size_t *osz) > to = 'f'; > break; > default: > - return 0; > + return false; > } > > - *out++ = '\\'; > - *out++ = to; > - > - *dst = out; > - *osz -= 2; > + if (out + 1 >= end) > + goto skip; > + if (out + 0 < end) > + out[0] = '\\'; > + if (out + 1 < end) > + out[1] = to; > > - return 1; > +skip: > + *dst = out + 2; > + return true; Why couldn't we use 3 line idiom? You are going to remove skip stuff in the next patch anyway, but assignment will be left. So, what about if (out + 2 > end) { *dst = out + 2; return true; } if (out < end) *out = '\\'; ++out; if (out < end) *out = to; ++out; *dst = out; return true; > } > > -static int escape_special(unsigned char c, char **dst, size_t *osz) > +static bool escape_special(unsigned char c, char **dst, char *end) > { > char *out = *dst; > unsigned char to; > > - if (*osz < 2) > - return -ENOMEM; > - > switch (c) { > case '\\': > to = '\\'; > @@ -314,71 +306,78 @@ static int escape_special(unsigned char c, char **dst, size_t *osz) > to = 'e'; > break; > default: > - return 0; > + return false; > } > > - *out++ = '\\'; > - *out++ = to; > - > - *dst = out; > - *osz -= 2; > + if (out + 1 >= end) > + goto skip; > + if (out + 0 < end) > + out[0] = '\\'; > + if (out + 1 < end) > + out[1] = to; > > - return 1; > +skip: > + *dst = out + 2; > + return true; > } > > -static int escape_null(unsigned char c, char **dst, size_t *osz) > +static bool escape_null(unsigned char c, char **dst, char *end) > { > char *out = *dst; > > - if (*osz < 2) > - return -ENOMEM; > - > if (c) > - return 0; > - > - *out++ = '\\'; > - *out++ = '0'; > + return false; > > - *dst = out; > - *osz -= 2; > + if (out + 1 >= end) > + goto skip; > + if (out + 0 < end) > + out[0] = '\\'; > + if (out + 1 < end) > + out[1] = '0'; > > - return 1; > +skip: > + *dst = out + 2; > + return true; > } > > -static int escape_octal(unsigned char c, char **dst, size_t *osz) > +static bool escape_octal(unsigned char c, char **dst, char *end) > { > char *out = *dst; > > - if (*osz < 4) > - return -ENOMEM; > - > - *out++ = '\\'; > - *out++ = ((c >> 6) & 0x07) + '0'; > - *out++ = ((c >> 3) & 0x07) + '0'; > - *out++ = ((c >> 0) & 0x07) + '0'; > - > - *dst = out; > - *osz -= 4; > - > - return 1; > + if (out + 3 >= end) > + goto skip; > + if (out + 0 < end) > + out[0] = '\\'; > + if (out + 1 < end) > + out[1] = ((c >> 6) & 0x07) + '0'; > + if (out + 2 < end) > + out[2] = ((c >> 3) & 0x07) + '0'; > + if (out + 3 < end) > + out[3] = ((c >> 0) & 0x07) + '0'; > + > +skip: > + *dst = out + 4; > + return true; > } > > -static int escape_hex(unsigned char c, char **dst, size_t *osz) > +static bool escape_hex(unsigned char c, char **dst, char *end) > { > char *out = *dst; > > - if (*osz < 4) > - return -ENOMEM; > - > - *out++ = '\\'; > - *out++ = 'x'; > - *out++ = hex_asc_hi(c); > - *out++ = hex_asc_lo(c); > - > - *dst = out; > - *osz -= 4; > - > - return 1; > + if (out + 3 >= end) > + goto skip; > + if (out + 0 < end) > + out[0] = '\\'; > + if (out + 1 < end) > + out[1] = 'x'; > + if (out + 2 < end) > + out[2] = hex_asc_hi(c); > + if (out + 3 < end) > + out[3] = hex_asc_lo(c); > + > +skip: > + *dst = out + 4; > + return true; > } > > /** > @@ -440,9 +439,10 @@ static int escape_hex(unsigned char c, char **dst, size_t *osz) > int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, > unsigned int flags, const char *esc) > { > - char *out = *dst, *p = out; > + char *p = *dst; > + char *end = p + osz; > bool is_dict = esc && *esc; > - int ret = 0; > + int ret; > > while (isz--) { > unsigned char c = *src++; > @@ -462,55 +462,33 @@ int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, > (is_dict && !strchr(esc, c))) { > /* do nothing */ > } else { > - if (flags & ESCAPE_SPACE) { > - ret = escape_space(c, &p, &osz); > - if (ret < 0) > - break; > - if (ret > 0) > - continue; > - } > - > - if (flags & ESCAPE_SPECIAL) { > - ret = escape_special(c, &p, &osz); > - if (ret < 0) > - break; > - if (ret > 0) > - continue; > - } > - > - if (flags & ESCAPE_NULL) { > - ret = escape_null(c, &p, &osz); > - if (ret < 0) > - break; > - if (ret > 0) > - continue; > - } > + if (flags & ESCAPE_SPACE && escape_space(c, &p, end)) > + continue; > + > + if (flags & ESCAPE_SPECIAL && escape_special(c, &p, end)) > + continue; > + > + if (flags & ESCAPE_NULL && escape_null(c, &p, end)) > + continue; > > /* ESCAPE_OCTAL and ESCAPE_HEX always go last */ > - if (flags & ESCAPE_OCTAL) { > - ret = escape_octal(c, &p, &osz); > - if (ret < 0) > - break; > + if (flags & ESCAPE_OCTAL && escape_octal(c, &p, end)) > continue; > - } > - if (flags & ESCAPE_HEX) { > - ret = escape_hex(c, &p, &osz); > - if (ret < 0) > - break; > + > + if (flags & ESCAPE_HEX && escape_hex(c, &p, end)) > continue; > - } > } > > - ret = escape_passthrough(c, &p, &osz); > - if (ret < 0) > - break; > + escape_passthrough(c, &p, end); > } > > - *dst = p; > - > - if (ret < 0) > - return ret; > + if (p > end) { > + *dst = end; > + return -ENOMEM; > + } > > - return p - out; > + ret = p - *dst; > + *dst = p; > + return ret; > } > EXPORT_SYMBOL(string_escape_mem); -- Andy Shevchenko <andriy.shevchenko@intel.com> Intel Finland Oy ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v3 3/3] lib/string_helpers.c: Change semantics of string_escape_mem @ 2015-02-09 23:44 ` Rasmus Villemoes 0 siblings, 0 replies; 52+ messages in thread From: Rasmus Villemoes @ 2015-02-09 23:44 UTC (permalink / raw) To: Andrew Morton, Andy Shevchenko, Trond Myklebust, J. Bruce Fields, David S. Miller Cc: Rasmus Villemoes, linux-kernel, linux-nfs, netdev The current semantics of string_escape_mem are inadequate for one of its current users, vsnprintf(). If that is to honour its contract, it must know how much space would be needed for the entire escaped buffer, and string_escape_mem provides no way of obtaining that (short of allocating a large enough buffer (~4 times input string) to let it play with, and that's definitely a big no-no inside vsnprintf). So change the semantics for string_escape_mem to be more snprintf-like: Return the size of the output that would be generated if the destination buffer was big enough, but of course still only write to the part of dst it is allowed to, and don't do '\0'-termination. It is then up to the caller to detect whether output was truncated and to append a '\0' if desired. Also, we must output partial escape sequences, otherwise a call such as snprintf(buf, 3, "%1pE", "\123") would cause printf to write a \0 to buf[2] but leaving buf[0] and buf[1] with whatever they previously contained. This also fixes a bug in the escaped_string() helper function, which used to unconditionally pass a length of "end-buf" to string_escape_mem(); since the latter doesn't check osz for being insanely large, it would happily write to dst. For example, kasprintf(GFP_KERNEL, "something and then %pE", ...); is an easy way to trigger an oops. In test-string_helpers.c, I removed the now meaningless -ENOMEM test, and replaced it with testing for getting the expected return value even if the buffer is too small. Also ensure that nothing is written when osz == 0. In net/sunrpc/cache.c, I think qword_add still has the same semantics. Someone should definitely double-check this. Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> --- include/linux/string_helpers.h | 8 ++++---- lib/string_helpers.c | 39 +++++++-------------------------------- lib/test-string_helpers.c | 35 +++++++++++++++-------------------- lib/vsprintf.c | 8 ++++++-- net/sunrpc/cache.c | 8 +++++--- 5 files changed, 37 insertions(+), 61 deletions(-) diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h index 6eb567ac56bc..38a2a6f1fc76 100644 --- a/include/linux/string_helpers.h +++ b/include/linux/string_helpers.h @@ -47,22 +47,22 @@ static inline int string_unescape_any_inplace(char *buf) #define ESCAPE_ANY_NP (ESCAPE_ANY | ESCAPE_NP) #define ESCAPE_HEX 0x20 -int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, +int string_escape_mem(const char *src, size_t isz, char *dst, size_t osz, unsigned int flags, const char *esc); static inline int string_escape_mem_any_np(const char *src, size_t isz, - char **dst, size_t osz, const char *esc) + char *dst, size_t osz, const char *esc) { return string_escape_mem(src, isz, dst, osz, ESCAPE_ANY_NP, esc); } -static inline int string_escape_str(const char *src, char **dst, size_t sz, +static inline int string_escape_str(const char *src, char *dst, size_t sz, unsigned int flags, const char *esc) { return string_escape_mem(src, strlen(src), dst, sz, flags, esc); } -static inline int string_escape_str_any_np(const char *src, char **dst, +static inline int string_escape_str_any_np(const char *src, char *dst, size_t sz, const char *esc) { return string_escape_str(src, dst, sz, ESCAPE_ANY_NP, esc); diff --git a/lib/string_helpers.c b/lib/string_helpers.c index 7e2fef1eb40e..df7fda90b333 100644 --- a/lib/string_helpers.c +++ b/lib/string_helpers.c @@ -278,14 +278,11 @@ static bool escape_space(unsigned char c, char **dst, char *end) return false; } - if (out + 1 >= end) - goto skip; if (out + 0 < end) out[0] = '\\'; if (out + 1 < end) out[1] = to; -skip: *dst = out + 2; return true; } @@ -309,14 +306,11 @@ static bool escape_special(unsigned char c, char **dst, char *end) return false; } - if (out + 1 >= end) - goto skip; if (out + 0 < end) out[0] = '\\'; if (out + 1 < end) out[1] = to; -skip: *dst = out + 2; return true; } @@ -328,14 +322,11 @@ static bool escape_null(unsigned char c, char **dst, char *end) if (c) return false; - if (out + 1 >= end) - goto skip; if (out + 0 < end) out[0] = '\\'; if (out + 1 < end) out[1] = '0'; -skip: *dst = out + 2; return true; } @@ -344,8 +335,6 @@ static bool escape_octal(unsigned char c, char **dst, char *end) { char *out = *dst; - if (out + 3 >= end) - goto skip; if (out + 0 < end) out[0] = '\\'; if (out + 1 < end) @@ -355,7 +344,6 @@ static bool escape_octal(unsigned char c, char **dst, char *end) if (out + 3 < end) out[3] = ((c >> 0) & 0x07) + '0'; -skip: *dst = out + 4; return true; } @@ -364,8 +352,6 @@ static bool escape_hex(unsigned char c, char **dst, char *end) { char *out = *dst; - if (out + 3 >= end) - goto skip; if (out + 0 < end) out[0] = '\\'; if (out + 1 < end) @@ -375,7 +361,6 @@ static bool escape_hex(unsigned char c, char **dst, char *end) if (out + 3 < end) out[3] = hex_asc_lo(c); -skip: *dst = out + 4; return true; } @@ -429,20 +414,17 @@ skip: * it if needs. * * Return: - * The amount of the characters processed to the destination buffer, or - * %-ENOMEM if the size of buffer is not enough to put an escaped character is - * returned. - * - * Even in the case of error @dst pointer will be updated to point to the byte - * after the last processed character. + * The total size of the escaped output that would be generated for + * the given input and flags. To check whether the output was + * truncated, compare the return value to osz. There is room left in + * dst for a '\0' terminator if and only if ret < osz. */ -int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, +int string_escape_mem(const char *src, size_t isz, char *dst, size_t osz, unsigned int flags, const char *esc) { - char *p = *dst; + char *p = dst; char *end = p + osz; bool is_dict = esc && *esc; - int ret; while (isz--) { unsigned char c = *src++; @@ -482,13 +464,6 @@ int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, escape_passthrough(c, &p, end); } - if (p > end) { - *dst = end; - return -ENOMEM; - } - - ret = p - *dst; - *dst = p; - return ret; + return p - dst; } EXPORT_SYMBOL(string_escape_mem); diff --git a/lib/test-string_helpers.c b/lib/test-string_helpers.c index ab0d30e1e18f..5f759c3c2f60 100644 --- a/lib/test-string_helpers.c +++ b/lib/test-string_helpers.c @@ -264,12 +264,12 @@ static __init void test_string_escape(const char *name, const struct test_string_2 *s2, unsigned int flags, const char *esc) { - int q_real = 512; - char *out_test = kmalloc(q_real, GFP_KERNEL); - char *out_real = kmalloc(q_real, GFP_KERNEL); + size_t out_size = 512; + char *out_test = kmalloc(out_size, GFP_KERNEL); + char *out_real = kmalloc(out_size, GFP_KERNEL); char *in = kmalloc(256, GFP_KERNEL); - char *buf = out_real; int p = 0, q_test = 0; + int q_real; if (!out_test || !out_real || !in) goto out; @@ -301,29 +301,26 @@ static __init void test_string_escape(const char *name, q_test += len; } - q_real = string_escape_mem(in, p, &buf, q_real, flags, esc); + q_real = string_escape_mem(in, p, out_real, out_size, flags, esc); test_string_check_buf(name, flags, in, p, out_real, q_real, out_test, q_test); + + 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); + out: kfree(in); kfree(out_real); kfree(out_test); } -static __init void test_string_escape_nomem(void) -{ - char *in = "\eb \\C\007\"\x90\r]"; - char out[64], *buf = out; - int rc = -ENOMEM, ret; - - ret = string_escape_str_any_np(in, &buf, strlen(in), NULL); - if (ret == rc) - return; - - pr_err("Test 'escape nomem' failed: got %d instead of %d\n", ret, rc); -} - static int __init test_string_helpers_init(void) { unsigned int i; @@ -342,8 +339,6 @@ static int __init test_string_helpers_init(void) for (i = 0; i < (ESCAPE_ANY_NP | ESCAPE_HEX) + 1; i++) test_string_escape("escape 1", escape1, i, TEST_STRING_2_DICT_1); - test_string_escape_nomem(); - return -EINVAL; } module_init(test_string_helpers_init); diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 3568e3906777..58e1193eaa4b 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -1159,8 +1159,12 @@ char *escaped_string(char *buf, char *end, u8 *addr, struct printf_spec spec, len = spec.field_width < 0 ? 1 : spec.field_width; - /* Ignore the error. We print as many characters as we can */ - string_escape_mem(addr, len, &buf, end - buf, flags, NULL); + /* + * string_escape_mem writes as many characters as it can to + * the given buffer, and returns the total size of the output + * had the buffer been big enough. + */ + buf += string_escape_mem(addr, len, buf, buf < end ? end - buf : 0, flags, NULL); return buf; } diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c index 33fb105d4352..22c4418057f4 100644 --- a/net/sunrpc/cache.c +++ b/net/sunrpc/cache.c @@ -1068,12 +1068,14 @@ void qword_add(char **bpp, int *lp, char *str) { char *bp = *bpp; int len = *lp; - int ret; + int ret, written; if (len < 0) return; - ret = string_escape_str(str, &bp, len, ESCAPE_OCTAL, "\\ \n\t"); - if (ret < 0 || ret == len) + ret = string_escape_str(str, bp, len, ESCAPE_OCTAL, "\\ \n\t"); + written = min(ret, len); + bp += written; + if (ret >= len) len = -1; else { len -= ret; -- 2.1.3 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH v3 3/3] lib/string_helpers.c: Change semantics of string_escape_mem @ 2015-02-09 23:44 ` Rasmus Villemoes 0 siblings, 0 replies; 52+ messages in thread From: Rasmus Villemoes @ 2015-02-09 23:44 UTC (permalink / raw) To: Andrew Morton, Andy Shevchenko, Trond Myklebust, J. Bruce Fields, David S. Miller Cc: Rasmus Villemoes, linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-nfs-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA The current semantics of string_escape_mem are inadequate for one of its current users, vsnprintf(). If that is to honour its contract, it must know how much space would be needed for the entire escaped buffer, and string_escape_mem provides no way of obtaining that (short of allocating a large enough buffer (~4 times input string) to let it play with, and that's definitely a big no-no inside vsnprintf). So change the semantics for string_escape_mem to be more snprintf-like: Return the size of the output that would be generated if the destination buffer was big enough, but of course still only write to the part of dst it is allowed to, and don't do '\0'-termination. It is then up to the caller to detect whether output was truncated and to append a '\0' if desired. Also, we must output partial escape sequences, otherwise a call such as snprintf(buf, 3, "%1pE", "\123") would cause printf to write a \0 to buf[2] but leaving buf[0] and buf[1] with whatever they previously contained. This also fixes a bug in the escaped_string() helper function, which used to unconditionally pass a length of "end-buf" to string_escape_mem(); since the latter doesn't check osz for being insanely large, it would happily write to dst. For example, kasprintf(GFP_KERNEL, "something and then %pE", ...); is an easy way to trigger an oops. In test-string_helpers.c, I removed the now meaningless -ENOMEM test, and replaced it with testing for getting the expected return value even if the buffer is too small. Also ensure that nothing is written when osz == 0. In net/sunrpc/cache.c, I think qword_add still has the same semantics. Someone should definitely double-check this. Signed-off-by: Rasmus Villemoes <linux-qQsb+v5E8BnlAoU/VqSP6n9LOBIZ5rWg@public.gmane.org> --- include/linux/string_helpers.h | 8 ++++---- lib/string_helpers.c | 39 +++++++-------------------------------- lib/test-string_helpers.c | 35 +++++++++++++++-------------------- lib/vsprintf.c | 8 ++++++-- net/sunrpc/cache.c | 8 +++++--- 5 files changed, 37 insertions(+), 61 deletions(-) diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h index 6eb567ac56bc..38a2a6f1fc76 100644 --- a/include/linux/string_helpers.h +++ b/include/linux/string_helpers.h @@ -47,22 +47,22 @@ static inline int string_unescape_any_inplace(char *buf) #define ESCAPE_ANY_NP (ESCAPE_ANY | ESCAPE_NP) #define ESCAPE_HEX 0x20 -int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, +int string_escape_mem(const char *src, size_t isz, char *dst, size_t osz, unsigned int flags, const char *esc); static inline int string_escape_mem_any_np(const char *src, size_t isz, - char **dst, size_t osz, const char *esc) + char *dst, size_t osz, const char *esc) { return string_escape_mem(src, isz, dst, osz, ESCAPE_ANY_NP, esc); } -static inline int string_escape_str(const char *src, char **dst, size_t sz, +static inline int string_escape_str(const char *src, char *dst, size_t sz, unsigned int flags, const char *esc) { return string_escape_mem(src, strlen(src), dst, sz, flags, esc); } -static inline int string_escape_str_any_np(const char *src, char **dst, +static inline int string_escape_str_any_np(const char *src, char *dst, size_t sz, const char *esc) { return string_escape_str(src, dst, sz, ESCAPE_ANY_NP, esc); diff --git a/lib/string_helpers.c b/lib/string_helpers.c index 7e2fef1eb40e..df7fda90b333 100644 --- a/lib/string_helpers.c +++ b/lib/string_helpers.c @@ -278,14 +278,11 @@ static bool escape_space(unsigned char c, char **dst, char *end) return false; } - if (out + 1 >= end) - goto skip; if (out + 0 < end) out[0] = '\\'; if (out + 1 < end) out[1] = to; -skip: *dst = out + 2; return true; } @@ -309,14 +306,11 @@ static bool escape_special(unsigned char c, char **dst, char *end) return false; } - if (out + 1 >= end) - goto skip; if (out + 0 < end) out[0] = '\\'; if (out + 1 < end) out[1] = to; -skip: *dst = out + 2; return true; } @@ -328,14 +322,11 @@ static bool escape_null(unsigned char c, char **dst, char *end) if (c) return false; - if (out + 1 >= end) - goto skip; if (out + 0 < end) out[0] = '\\'; if (out + 1 < end) out[1] = '0'; -skip: *dst = out + 2; return true; } @@ -344,8 +335,6 @@ static bool escape_octal(unsigned char c, char **dst, char *end) { char *out = *dst; - if (out + 3 >= end) - goto skip; if (out + 0 < end) out[0] = '\\'; if (out + 1 < end) @@ -355,7 +344,6 @@ static bool escape_octal(unsigned char c, char **dst, char *end) if (out + 3 < end) out[3] = ((c >> 0) & 0x07) + '0'; -skip: *dst = out + 4; return true; } @@ -364,8 +352,6 @@ static bool escape_hex(unsigned char c, char **dst, char *end) { char *out = *dst; - if (out + 3 >= end) - goto skip; if (out + 0 < end) out[0] = '\\'; if (out + 1 < end) @@ -375,7 +361,6 @@ static bool escape_hex(unsigned char c, char **dst, char *end) if (out + 3 < end) out[3] = hex_asc_lo(c); -skip: *dst = out + 4; return true; } @@ -429,20 +414,17 @@ skip: * it if needs. * * Return: - * The amount of the characters processed to the destination buffer, or - * %-ENOMEM if the size of buffer is not enough to put an escaped character is - * returned. - * - * Even in the case of error @dst pointer will be updated to point to the byte - * after the last processed character. + * The total size of the escaped output that would be generated for + * the given input and flags. To check whether the output was + * truncated, compare the return value to osz. There is room left in + * dst for a '\0' terminator if and only if ret < osz. */ -int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, +int string_escape_mem(const char *src, size_t isz, char *dst, size_t osz, unsigned int flags, const char *esc) { - char *p = *dst; + char *p = dst; char *end = p + osz; bool is_dict = esc && *esc; - int ret; while (isz--) { unsigned char c = *src++; @@ -482,13 +464,6 @@ int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, escape_passthrough(c, &p, end); } - if (p > end) { - *dst = end; - return -ENOMEM; - } - - ret = p - *dst; - *dst = p; - return ret; + return p - dst; } EXPORT_SYMBOL(string_escape_mem); diff --git a/lib/test-string_helpers.c b/lib/test-string_helpers.c index ab0d30e1e18f..5f759c3c2f60 100644 --- a/lib/test-string_helpers.c +++ b/lib/test-string_helpers.c @@ -264,12 +264,12 @@ static __init void test_string_escape(const char *name, const struct test_string_2 *s2, unsigned int flags, const char *esc) { - int q_real = 512; - char *out_test = kmalloc(q_real, GFP_KERNEL); - char *out_real = kmalloc(q_real, GFP_KERNEL); + size_t out_size = 512; + char *out_test = kmalloc(out_size, GFP_KERNEL); + char *out_real = kmalloc(out_size, GFP_KERNEL); char *in = kmalloc(256, GFP_KERNEL); - char *buf = out_real; int p = 0, q_test = 0; + int q_real; if (!out_test || !out_real || !in) goto out; @@ -301,29 +301,26 @@ static __init void test_string_escape(const char *name, q_test += len; } - q_real = string_escape_mem(in, p, &buf, q_real, flags, esc); + q_real = string_escape_mem(in, p, out_real, out_size, flags, esc); test_string_check_buf(name, flags, in, p, out_real, q_real, out_test, q_test); + + 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); + out: kfree(in); kfree(out_real); kfree(out_test); } -static __init void test_string_escape_nomem(void) -{ - char *in = "\eb \\C\007\"\x90\r]"; - char out[64], *buf = out; - int rc = -ENOMEM, ret; - - ret = string_escape_str_any_np(in, &buf, strlen(in), NULL); - if (ret == rc) - return; - - pr_err("Test 'escape nomem' failed: got %d instead of %d\n", ret, rc); -} - static int __init test_string_helpers_init(void) { unsigned int i; @@ -342,8 +339,6 @@ static int __init test_string_helpers_init(void) for (i = 0; i < (ESCAPE_ANY_NP | ESCAPE_HEX) + 1; i++) test_string_escape("escape 1", escape1, i, TEST_STRING_2_DICT_1); - test_string_escape_nomem(); - return -EINVAL; } module_init(test_string_helpers_init); diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 3568e3906777..58e1193eaa4b 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -1159,8 +1159,12 @@ char *escaped_string(char *buf, char *end, u8 *addr, struct printf_spec spec, len = spec.field_width < 0 ? 1 : spec.field_width; - /* Ignore the error. We print as many characters as we can */ - string_escape_mem(addr, len, &buf, end - buf, flags, NULL); + /* + * string_escape_mem writes as many characters as it can to + * the given buffer, and returns the total size of the output + * had the buffer been big enough. + */ + buf += string_escape_mem(addr, len, buf, buf < end ? end - buf : 0, flags, NULL); return buf; } diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c index 33fb105d4352..22c4418057f4 100644 --- a/net/sunrpc/cache.c +++ b/net/sunrpc/cache.c @@ -1068,12 +1068,14 @@ void qword_add(char **bpp, int *lp, char *str) { char *bp = *bpp; int len = *lp; - int ret; + int ret, written; if (len < 0) return; - ret = string_escape_str(str, &bp, len, ESCAPE_OCTAL, "\\ \n\t"); - if (ret < 0 || ret == len) + ret = string_escape_str(str, bp, len, ESCAPE_OCTAL, "\\ \n\t"); + written = min(ret, len); + bp += written; + if (ret >= len) len = -1; else { len -= ret; -- 2.1.3 -- 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 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH v3 3/3] lib/string_helpers.c: Change semantics of string_escape_mem @ 2015-02-10 12:32 ` Andy Shevchenko 0 siblings, 0 replies; 52+ messages in thread From: Andy Shevchenko @ 2015-02-10 12:32 UTC (permalink / raw) To: Rasmus Villemoes Cc: Andrew Morton, Trond Myklebust, J. Bruce Fields, David S. Miller, linux-kernel, linux-nfs, netdev On Tue, 2015-02-10 at 00:44 +0100, Rasmus Villemoes wrote: > The current semantics of string_escape_mem are inadequate for one of > its current users, vsnprintf(). If that is to honour its contract, it > must know how much space would be needed for the entire escaped > buffer, and string_escape_mem provides no way of obtaining that (short > of allocating a large enough buffer (~4 times input string) to let it > play with, and that's definitely a big no-no inside vsnprintf). > > So change the semantics for string_escape_mem to be more > snprintf-like: Return the size of the output that would be generated > if the destination buffer was big enough, but of course still only > write to the part of dst it is allowed to, and don't do > '\0'-termination. It is then up to the caller to detect whether output > was truncated and to append a '\0' if desired. Also, we must output > partial escape sequences, otherwise a call such as snprintf(buf, 3, > "%1pE", "\123") would cause printf to write a \0 to buf[2] but leaving > buf[0] and buf[1] with whatever they previously contained. > > This also fixes a bug in the escaped_string() helper function, which > used to unconditionally pass a length of "end-buf" to > string_escape_mem(); since the latter doesn't check osz for being > insanely large, it would happily write to dst. For example, > kasprintf(GFP_KERNEL, "something and then %pE", ...); is an easy way > to trigger an oops. > > In test-string_helpers.c, I removed the now meaningless -ENOMEM test, > and replaced it with testing for getting the expected return value > even if the buffer is too small. Also ensure that nothing is written > when osz == 0. > > In net/sunrpc/cache.c, I think qword_add still has the same > semantics. Someone should definitely double-check this. Thanks for an update. My comments below. After addressing 'em, wrt changes to patch 2/3, take my Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> for all parts except net/sunrpc/cache.c. > > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> > --- > include/linux/string_helpers.h | 8 ++++---- > lib/string_helpers.c | 39 +++++++-------------------------------- > lib/test-string_helpers.c | 35 +++++++++++++++-------------------- > lib/vsprintf.c | 8 ++++++-- > net/sunrpc/cache.c | 8 +++++--- > 5 files changed, 37 insertions(+), 61 deletions(-) > > diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h > index 6eb567ac56bc..38a2a6f1fc76 100644 > --- a/include/linux/string_helpers.h > +++ b/include/linux/string_helpers.h > @@ -47,22 +47,22 @@ static inline int string_unescape_any_inplace(char *buf) > #define ESCAPE_ANY_NP (ESCAPE_ANY | ESCAPE_NP) > #define ESCAPE_HEX 0x20 > > -int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, > +int string_escape_mem(const char *src, size_t isz, char *dst, size_t osz, > unsigned int flags, const char *esc); > > static inline int string_escape_mem_any_np(const char *src, size_t isz, > - char **dst, size_t osz, const char *esc) > + char *dst, size_t osz, const char *esc) > { > return string_escape_mem(src, isz, dst, osz, ESCAPE_ANY_NP, esc); > } > > -static inline int string_escape_str(const char *src, char **dst, size_t sz, > +static inline int string_escape_str(const char *src, char *dst, size_t sz, > unsigned int flags, const char *esc) > { > return string_escape_mem(src, strlen(src), dst, sz, flags, esc); > } > > -static inline int string_escape_str_any_np(const char *src, char **dst, > +static inline int string_escape_str_any_np(const char *src, char *dst, > size_t sz, const char *esc) > { > return string_escape_str(src, dst, sz, ESCAPE_ANY_NP, esc); > diff --git a/lib/string_helpers.c b/lib/string_helpers.c > index 7e2fef1eb40e..df7fda90b333 100644 > --- a/lib/string_helpers.c > +++ b/lib/string_helpers.c > @@ -278,14 +278,11 @@ static bool escape_space(unsigned char c, char **dst, char *end) > return false; > } > > - if (out + 1 >= end) > - goto skip; > if (out + 0 < end) > out[0] = '\\'; > if (out + 1 < end) > out[1] = to; > > -skip: > *dst = out + 2; > return true; > } > @@ -309,14 +306,11 @@ static bool escape_special(unsigned char c, char **dst, char *end) > return false; > } > > - if (out + 1 >= end) > - goto skip; > if (out + 0 < end) > out[0] = '\\'; > if (out + 1 < end) > out[1] = to; > > -skip: > *dst = out + 2; > return true; > } > @@ -328,14 +322,11 @@ static bool escape_null(unsigned char c, char **dst, char *end) > if (c) > return false; > > - if (out + 1 >= end) > - goto skip; > if (out + 0 < end) > out[0] = '\\'; > if (out + 1 < end) > out[1] = '0'; > > -skip: > *dst = out + 2; > return true; > } > @@ -344,8 +335,6 @@ static bool escape_octal(unsigned char c, char **dst, char *end) > { > char *out = *dst; > > - if (out + 3 >= end) > - goto skip; > if (out + 0 < end) > out[0] = '\\'; > if (out + 1 < end) > @@ -355,7 +344,6 @@ static bool escape_octal(unsigned char c, char **dst, char *end) > if (out + 3 < end) > out[3] = ((c >> 0) & 0x07) + '0'; > > -skip: > *dst = out + 4; > return true; > } > @@ -364,8 +352,6 @@ static bool escape_hex(unsigned char c, char **dst, char *end) > { > char *out = *dst; > > - if (out + 3 >= end) > - goto skip; > if (out + 0 < end) > out[0] = '\\'; > if (out + 1 < end) > @@ -375,7 +361,6 @@ static bool escape_hex(unsigned char c, char **dst, char *end) > if (out + 3 < end) > out[3] = hex_asc_lo(c); > > -skip: > *dst = out + 4; > return true; > } > @@ -429,20 +414,17 @@ skip: > * it if needs. > * > * Return: > - * The amount of the characters processed to the destination buffer, or > - * %-ENOMEM if the size of buffer is not enough to put an escaped character is > - * returned. > - * > - * Even in the case of error @dst pointer will be updated to point to the byte > - * after the last processed character. > + * The total size of the escaped output that would be generated for > + * the given input and flags. To check whether the output was > + * truncated, compare the return value to osz. There is room left in > + * dst for a '\0' terminator if and only if ret < osz. > */ > -int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, > +int string_escape_mem(const char *src, size_t isz, char *dst, size_t osz, > unsigned int flags, const char *esc) > { > - char *p = *dst; > + char *p = dst; > char *end = p + osz; > bool is_dict = esc && *esc; > - int ret; > > while (isz--) { > unsigned char c = *src++; > @@ -482,13 +464,6 @@ int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, > escape_passthrough(c, &p, end); > } > > - if (p > end) { > - *dst = end; > - return -ENOMEM; > - } > - > - ret = p - *dst; > - *dst = p; > - return ret; > + return p - dst; > } > EXPORT_SYMBOL(string_escape_mem); > diff --git a/lib/test-string_helpers.c b/lib/test-string_helpers.c > index ab0d30e1e18f..5f759c3c2f60 100644 > --- a/lib/test-string_helpers.c > +++ b/lib/test-string_helpers.c > @@ -264,12 +264,12 @@ static __init void test_string_escape(const char *name, > const struct test_string_2 *s2, > unsigned int flags, const char *esc) > { > - int q_real = 512; > - char *out_test = kmalloc(q_real, GFP_KERNEL); > - char *out_real = kmalloc(q_real, GFP_KERNEL); > + size_t out_size = 512; > + char *out_test = kmalloc(out_size, GFP_KERNEL); > + char *out_real = kmalloc(out_size, GFP_KERNEL); > char *in = kmalloc(256, GFP_KERNEL); > - char *buf = out_real; > int p = 0, q_test = 0; > + int q_real; > > if (!out_test || !out_real || !in) > goto out; > @@ -301,29 +301,26 @@ static __init void test_string_escape(const char *name, > q_test += len; > } > > - q_real = string_escape_mem(in, p, &buf, q_real, flags, esc); > + q_real = string_escape_mem(in, p, out_real, out_size, flags, esc); > > test_string_check_buf(name, flags, in, p, out_real, q_real, out_test, > q_test); > + > + 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); > + So, why couldn't we split this to separate test case? It seems I already pointed this out. > out: > kfree(in); > kfree(out_real); > kfree(out_test); > } > > -static __init void test_string_escape_nomem(void) > -{ > - char *in = "\eb \\C\007\"\x90\r]"; > - char out[64], *buf = out; > - int rc = -ENOMEM, ret; > - > - ret = string_escape_str_any_np(in, &buf, strlen(in), NULL); > - if (ret == rc) > - return; > - > - pr_err("Test 'escape nomem' failed: got %d instead of %d\n", ret, rc); > -} > - > static int __init test_string_helpers_init(void) > { > unsigned int i; > @@ -342,8 +339,6 @@ static int __init test_string_helpers_init(void) > for (i = 0; i < (ESCAPE_ANY_NP | ESCAPE_HEX) + 1; i++) > test_string_escape("escape 1", escape1, i, TEST_STRING_2_DICT_1); > > - test_string_escape_nomem(); > - > return -EINVAL; > } > module_init(test_string_helpers_init); > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > index 3568e3906777..58e1193eaa4b 100644 > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -1159,8 +1159,12 @@ char *escaped_string(char *buf, char *end, u8 *addr, struct printf_spec spec, > > len = spec.field_width < 0 ? 1 : spec.field_width; > > - /* Ignore the error. We print as many characters as we can */ > - string_escape_mem(addr, len, &buf, end - buf, flags, NULL); > + /* > + * string_escape_mem writes as many characters as it can to 'string_escape_mem() writes…' > + * the given buffer, and returns the total size of the output > + * had the buffer been big enough. > + */ > + buf += string_escape_mem(addr, len, buf, buf < end ? end - buf : 0, flags, NULL); > > return buf; > } > diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c > index 33fb105d4352..22c4418057f4 100644 > --- a/net/sunrpc/cache.c > +++ b/net/sunrpc/cache.c > @@ -1068,12 +1068,14 @@ void qword_add(char **bpp, int *lp, char *str) > { > char *bp = *bpp; > int len = *lp; > - int ret; > + int ret, written; > > if (len < 0) return; > > - ret = string_escape_str(str, &bp, len, ESCAPE_OCTAL, "\\ \n\t"); > - if (ret < 0 || ret == len) > + ret = string_escape_str(str, bp, len, ESCAPE_OCTAL, "\\ \n\t"); > + written = min(ret, len); > + bp += written; > + if (ret >= len) > len = -1; > else { > len -= ret; -- Andy Shevchenko <andriy.shevchenko@intel.com> Intel Finland Oy ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v3 3/3] lib/string_helpers.c: Change semantics of string_escape_mem @ 2015-02-10 12:32 ` Andy Shevchenko 0 siblings, 0 replies; 52+ messages in thread From: Andy Shevchenko @ 2015-02-10 12:32 UTC (permalink / raw) To: Rasmus Villemoes Cc: Andrew Morton, Trond Myklebust, J. Bruce Fields, David S. Miller, linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-nfs-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA On Tue, 2015-02-10 at 00:44 +0100, Rasmus Villemoes wrote: > The current semantics of string_escape_mem are inadequate for one of > its current users, vsnprintf(). If that is to honour its contract, it > must know how much space would be needed for the entire escaped > buffer, and string_escape_mem provides no way of obtaining that (short > of allocating a large enough buffer (~4 times input string) to let it > play with, and that's definitely a big no-no inside vsnprintf). > > So change the semantics for string_escape_mem to be more > snprintf-like: Return the size of the output that would be generated > if the destination buffer was big enough, but of course still only > write to the part of dst it is allowed to, and don't do > '\0'-termination. It is then up to the caller to detect whether output > was truncated and to append a '\0' if desired. Also, we must output > partial escape sequences, otherwise a call such as snprintf(buf, 3, > "%1pE", "\123") would cause printf to write a \0 to buf[2] but leaving > buf[0] and buf[1] with whatever they previously contained. > > This also fixes a bug in the escaped_string() helper function, which > used to unconditionally pass a length of "end-buf" to > string_escape_mem(); since the latter doesn't check osz for being > insanely large, it would happily write to dst. For example, > kasprintf(GFP_KERNEL, "something and then %pE", ...); is an easy way > to trigger an oops. > > In test-string_helpers.c, I removed the now meaningless -ENOMEM test, > and replaced it with testing for getting the expected return value > even if the buffer is too small. Also ensure that nothing is written > when osz == 0. > > In net/sunrpc/cache.c, I think qword_add still has the same > semantics. Someone should definitely double-check this. Thanks for an update. My comments below. After addressing 'em, wrt changes to patch 2/3, take my Acked-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> for all parts except net/sunrpc/cache.c. > > Signed-off-by: Rasmus Villemoes <linux-qQsb+v5E8BnlAoU/VqSP6n9LOBIZ5rWg@public.gmane.org> > --- > include/linux/string_helpers.h | 8 ++++---- > lib/string_helpers.c | 39 +++++++-------------------------------- > lib/test-string_helpers.c | 35 +++++++++++++++-------------------- > lib/vsprintf.c | 8 ++++++-- > net/sunrpc/cache.c | 8 +++++--- > 5 files changed, 37 insertions(+), 61 deletions(-) > > diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h > index 6eb567ac56bc..38a2a6f1fc76 100644 > --- a/include/linux/string_helpers.h > +++ b/include/linux/string_helpers.h > @@ -47,22 +47,22 @@ static inline int string_unescape_any_inplace(char *buf) > #define ESCAPE_ANY_NP (ESCAPE_ANY | ESCAPE_NP) > #define ESCAPE_HEX 0x20 > > -int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, > +int string_escape_mem(const char *src, size_t isz, char *dst, size_t osz, > unsigned int flags, const char *esc); > > static inline int string_escape_mem_any_np(const char *src, size_t isz, > - char **dst, size_t osz, const char *esc) > + char *dst, size_t osz, const char *esc) > { > return string_escape_mem(src, isz, dst, osz, ESCAPE_ANY_NP, esc); > } > > -static inline int string_escape_str(const char *src, char **dst, size_t sz, > +static inline int string_escape_str(const char *src, char *dst, size_t sz, > unsigned int flags, const char *esc) > { > return string_escape_mem(src, strlen(src), dst, sz, flags, esc); > } > > -static inline int string_escape_str_any_np(const char *src, char **dst, > +static inline int string_escape_str_any_np(const char *src, char *dst, > size_t sz, const char *esc) > { > return string_escape_str(src, dst, sz, ESCAPE_ANY_NP, esc); > diff --git a/lib/string_helpers.c b/lib/string_helpers.c > index 7e2fef1eb40e..df7fda90b333 100644 > --- a/lib/string_helpers.c > +++ b/lib/string_helpers.c > @@ -278,14 +278,11 @@ static bool escape_space(unsigned char c, char **dst, char *end) > return false; > } > > - if (out + 1 >= end) > - goto skip; > if (out + 0 < end) > out[0] = '\\'; > if (out + 1 < end) > out[1] = to; > > -skip: > *dst = out + 2; > return true; > } > @@ -309,14 +306,11 @@ static bool escape_special(unsigned char c, char **dst, char *end) > return false; > } > > - if (out + 1 >= end) > - goto skip; > if (out + 0 < end) > out[0] = '\\'; > if (out + 1 < end) > out[1] = to; > > -skip: > *dst = out + 2; > return true; > } > @@ -328,14 +322,11 @@ static bool escape_null(unsigned char c, char **dst, char *end) > if (c) > return false; > > - if (out + 1 >= end) > - goto skip; > if (out + 0 < end) > out[0] = '\\'; > if (out + 1 < end) > out[1] = '0'; > > -skip: > *dst = out + 2; > return true; > } > @@ -344,8 +335,6 @@ static bool escape_octal(unsigned char c, char **dst, char *end) > { > char *out = *dst; > > - if (out + 3 >= end) > - goto skip; > if (out + 0 < end) > out[0] = '\\'; > if (out + 1 < end) > @@ -355,7 +344,6 @@ static bool escape_octal(unsigned char c, char **dst, char *end) > if (out + 3 < end) > out[3] = ((c >> 0) & 0x07) + '0'; > > -skip: > *dst = out + 4; > return true; > } > @@ -364,8 +352,6 @@ static bool escape_hex(unsigned char c, char **dst, char *end) > { > char *out = *dst; > > - if (out + 3 >= end) > - goto skip; > if (out + 0 < end) > out[0] = '\\'; > if (out + 1 < end) > @@ -375,7 +361,6 @@ static bool escape_hex(unsigned char c, char **dst, char *end) > if (out + 3 < end) > out[3] = hex_asc_lo(c); > > -skip: > *dst = out + 4; > return true; > } > @@ -429,20 +414,17 @@ skip: > * it if needs. > * > * Return: > - * The amount of the characters processed to the destination buffer, or > - * %-ENOMEM if the size of buffer is not enough to put an escaped character is > - * returned. > - * > - * Even in the case of error @dst pointer will be updated to point to the byte > - * after the last processed character. > + * The total size of the escaped output that would be generated for > + * the given input and flags. To check whether the output was > + * truncated, compare the return value to osz. There is room left in > + * dst for a '\0' terminator if and only if ret < osz. > */ > -int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, > +int string_escape_mem(const char *src, size_t isz, char *dst, size_t osz, > unsigned int flags, const char *esc) > { > - char *p = *dst; > + char *p = dst; > char *end = p + osz; > bool is_dict = esc && *esc; > - int ret; > > while (isz--) { > unsigned char c = *src++; > @@ -482,13 +464,6 @@ int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, > escape_passthrough(c, &p, end); > } > > - if (p > end) { > - *dst = end; > - return -ENOMEM; > - } > - > - ret = p - *dst; > - *dst = p; > - return ret; > + return p - dst; > } > EXPORT_SYMBOL(string_escape_mem); > diff --git a/lib/test-string_helpers.c b/lib/test-string_helpers.c > index ab0d30e1e18f..5f759c3c2f60 100644 > --- a/lib/test-string_helpers.c > +++ b/lib/test-string_helpers.c > @@ -264,12 +264,12 @@ static __init void test_string_escape(const char *name, > const struct test_string_2 *s2, > unsigned int flags, const char *esc) > { > - int q_real = 512; > - char *out_test = kmalloc(q_real, GFP_KERNEL); > - char *out_real = kmalloc(q_real, GFP_KERNEL); > + size_t out_size = 512; > + char *out_test = kmalloc(out_size, GFP_KERNEL); > + char *out_real = kmalloc(out_size, GFP_KERNEL); > char *in = kmalloc(256, GFP_KERNEL); > - char *buf = out_real; > int p = 0, q_test = 0; > + int q_real; > > if (!out_test || !out_real || !in) > goto out; > @@ -301,29 +301,26 @@ static __init void test_string_escape(const char *name, > q_test += len; > } > > - q_real = string_escape_mem(in, p, &buf, q_real, flags, esc); > + q_real = string_escape_mem(in, p, out_real, out_size, flags, esc); > > test_string_check_buf(name, flags, in, p, out_real, q_real, out_test, > q_test); > + > + 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); > + So, why couldn't we split this to separate test case? It seems I already pointed this out. > out: > kfree(in); > kfree(out_real); > kfree(out_test); > } > > -static __init void test_string_escape_nomem(void) > -{ > - char *in = "\eb \\C\007\"\x90\r]"; > - char out[64], *buf = out; > - int rc = -ENOMEM, ret; > - > - ret = string_escape_str_any_np(in, &buf, strlen(in), NULL); > - if (ret == rc) > - return; > - > - pr_err("Test 'escape nomem' failed: got %d instead of %d\n", ret, rc); > -} > - > static int __init test_string_helpers_init(void) > { > unsigned int i; > @@ -342,8 +339,6 @@ static int __init test_string_helpers_init(void) > for (i = 0; i < (ESCAPE_ANY_NP | ESCAPE_HEX) + 1; i++) > test_string_escape("escape 1", escape1, i, TEST_STRING_2_DICT_1); > > - test_string_escape_nomem(); > - > return -EINVAL; > } > module_init(test_string_helpers_init); > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > index 3568e3906777..58e1193eaa4b 100644 > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -1159,8 +1159,12 @@ char *escaped_string(char *buf, char *end, u8 *addr, struct printf_spec spec, > > len = spec.field_width < 0 ? 1 : spec.field_width; > > - /* Ignore the error. We print as many characters as we can */ > - string_escape_mem(addr, len, &buf, end - buf, flags, NULL); > + /* > + * string_escape_mem writes as many characters as it can to 'string_escape_mem() writes…' > + * the given buffer, and returns the total size of the output > + * had the buffer been big enough. > + */ > + buf += string_escape_mem(addr, len, buf, buf < end ? end - buf : 0, flags, NULL); > > return buf; > } > diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c > index 33fb105d4352..22c4418057f4 100644 > --- a/net/sunrpc/cache.c > +++ b/net/sunrpc/cache.c > @@ -1068,12 +1068,14 @@ void qword_add(char **bpp, int *lp, char *str) > { > char *bp = *bpp; > int len = *lp; > - int ret; > + int ret, written; > > if (len < 0) return; > > - ret = string_escape_str(str, &bp, len, ESCAPE_OCTAL, "\\ \n\t"); > - if (ret < 0 || ret == len) > + ret = string_escape_str(str, bp, len, ESCAPE_OCTAL, "\\ \n\t"); > + written = min(ret, len); > + bp += written; > + if (ret >= len) > len = -1; > else { > len -= ret; -- 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v3 3/3] lib/string_helpers.c: Change semantics of string_escape_mem 2015-02-10 12:32 ` Andy Shevchenko (?) @ 2015-02-10 13:02 ` Rasmus Villemoes 2015-02-10 14:22 ` Andy Shevchenko -1 siblings, 1 reply; 52+ messages in thread From: Rasmus Villemoes @ 2015-02-10 13:02 UTC (permalink / raw) To: Andy Shevchenko Cc: Andrew Morton, Trond Myklebust, J. Bruce Fields, David S. Miller, linux-kernel, linux-nfs, netdev On Tue, Feb 10 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Tue, 2015-02-10 at 00:44 +0100, Rasmus Villemoes wrote: >> The current semantics of string_escape_mem are inadequate for one of >> its current users, vsnprintf(). If that is to honour its contract, it >> must know how much space would be needed for the entire escaped >> buffer, and string_escape_mem provides no way of obtaining that (short >> of allocating a large enough buffer (~4 times input string) to let it >> play with, and that's definitely a big no-no inside vsnprintf). >> >> So change the semantics for string_escape_mem to be more >> snprintf-like: Return the size of the output that would be generated >> if the destination buffer was big enough, but of course still only >> write to the part of dst it is allowed to, and don't do >> '\0'-termination. It is then up to the caller to detect whether output >> was truncated and to append a '\0' if desired. Also, we must output >> partial escape sequences, otherwise a call such as snprintf(buf, 3, >> "%1pE", "\123") would cause printf to write a \0 to buf[2] but leaving >> buf[0] and buf[1] with whatever they previously contained. >> >> This also fixes a bug in the escaped_string() helper function, which >> used to unconditionally pass a length of "end-buf" to >> string_escape_mem(); since the latter doesn't check osz for being >> insanely large, it would happily write to dst. For example, >> kasprintf(GFP_KERNEL, "something and then %pE", ...); is an easy way >> to trigger an oops. >> >> In test-string_helpers.c, I removed the now meaningless -ENOMEM test, >> and replaced it with testing for getting the expected return value >> even if the buffer is too small. Also ensure that nothing is written >> when osz == 0. >> >> In net/sunrpc/cache.c, I think qword_add still has the same >> semantics. Someone should definitely double-check this. > > Thanks for an update. My comments below. > After addressing 'em, wrt changes to patch 2/3, take my > Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > for all parts except net/sunrpc/cache.c. > >> >> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> >> --- >> index ab0d30e1e18f..5f759c3c2f60 100644 >> --- a/lib/test-string_helpers.c >> +++ b/lib/test-string_helpers.c >> @@ -264,12 +264,12 @@ static __init void test_string_escape(const char *name, >> const struct test_string_2 *s2, >> unsigned int flags, const char *esc) >> { >> - int q_real = 512; >> - char *out_test = kmalloc(q_real, GFP_KERNEL); >> - char *out_real = kmalloc(q_real, GFP_KERNEL); >> + size_t out_size = 512; >> + char *out_test = kmalloc(out_size, GFP_KERNEL); >> + char *out_real = kmalloc(out_size, GFP_KERNEL); >> char *in = kmalloc(256, GFP_KERNEL); >> - char *buf = out_real; >> int p = 0, q_test = 0; >> + int q_real; >> >> if (!out_test || !out_real || !in) >> goto out; >> @@ -301,29 +301,26 @@ static __init void test_string_escape(const char *name, >> q_test += len; >> } >> >> - q_real = string_escape_mem(in, p, &buf, q_real, flags, esc); >> + q_real = string_escape_mem(in, p, out_real, out_size, flags, esc); >> >> test_string_check_buf(name, flags, in, p, out_real, q_real, out_test, >> q_test); >> + >> + 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); >> + > > So, why couldn't we split this to separate test case? It seems I already > pointed this out. > This actually provides better coverage since we do this for all the "positive" test cases, instead of just the single ad hoc case done previously. Of course the added lines could be factored into a separate helper, but there's quite a lot of state to pass, so I thought this would actually be simpler - note how the two string_escape_mem calls are easily seen to be identical except for the outsize argument. It may already be too late for the merge window, but I didn't want to spend too much time on these mostly cosmetic details (that also goes for the 3- versus 2-line issue). Rasmus ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v3 3/3] lib/string_helpers.c: Change semantics of string_escape_mem @ 2015-02-10 14:22 ` Andy Shevchenko 0 siblings, 0 replies; 52+ messages in thread From: Andy Shevchenko @ 2015-02-10 14:22 UTC (permalink / raw) To: Rasmus Villemoes Cc: Andrew Morton, Trond Myklebust, J. Bruce Fields, David S. Miller, linux-kernel, linux-nfs, netdev On Tue, 2015-02-10 at 14:02 +0100, Rasmus Villemoes wrote: > On Tue, Feb 10 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > On Tue, 2015-02-10 at 00:44 +0100, Rasmus Villemoes wrote: > >> The current semantics of string_escape_mem are inadequate for one of > >> its current users, vsnprintf(). If that is to honour its contract, it > >> must know how much space would be needed for the entire escaped > >> buffer, and string_escape_mem provides no way of obtaining that (short > >> of allocating a large enough buffer (~4 times input string) to let it > >> play with, and that's definitely a big no-no inside vsnprintf). > >> > >> So change the semantics for string_escape_mem to be more > >> snprintf-like: Return the size of the output that would be generated > >> if the destination buffer was big enough, but of course still only > >> write to the part of dst it is allowed to, and don't do > >> '\0'-termination. It is then up to the caller to detect whether output > >> was truncated and to append a '\0' if desired. Also, we must output > >> partial escape sequences, otherwise a call such as snprintf(buf, 3, > >> "%1pE", "\123") would cause printf to write a \0 to buf[2] but leaving > >> buf[0] and buf[1] with whatever they previously contained. > >> > >> This also fixes a bug in the escaped_string() helper function, which > >> used to unconditionally pass a length of "end-buf" to > >> string_escape_mem(); since the latter doesn't check osz for being > >> insanely large, it would happily write to dst. For example, > >> kasprintf(GFP_KERNEL, "something and then %pE", ...); is an easy way > >> to trigger an oops. > >> > >> In test-string_helpers.c, I removed the now meaningless -ENOMEM test, > >> and replaced it with testing for getting the expected return value > >> even if the buffer is too small. Also ensure that nothing is written > >> when osz == 0. > >> > >> In net/sunrpc/cache.c, I think qword_add still has the same > >> semantics. Someone should definitely double-check this. > > > > Thanks for an update. My comments below. > > After addressing 'em, wrt changes to patch 2/3, take my > > Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > for all parts except net/sunrpc/cache.c. > > > >> > >> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> > >> --- > >> index ab0d30e1e18f..5f759c3c2f60 100644 > >> --- a/lib/test-string_helpers.c > >> +++ b/lib/test-string_helpers.c > >> @@ -264,12 +264,12 @@ static __init void test_string_escape(const char *name, > >> const struct test_string_2 *s2, > >> unsigned int flags, const char *esc) > >> { > >> - int q_real = 512; > >> - char *out_test = kmalloc(q_real, GFP_KERNEL); > >> - char *out_real = kmalloc(q_real, GFP_KERNEL); > >> + size_t out_size = 512; > >> + char *out_test = kmalloc(out_size, GFP_KERNEL); > >> + char *out_real = kmalloc(out_size, GFP_KERNEL); > >> char *in = kmalloc(256, GFP_KERNEL); > >> - char *buf = out_real; > >> int p = 0, q_test = 0; > >> + int q_real; > >> > >> if (!out_test || !out_real || !in) > >> goto out; > >> @@ -301,29 +301,26 @@ static __init void test_string_escape(const char *name, > >> q_test += len; > >> } > >> > >> - q_real = string_escape_mem(in, p, &buf, q_real, flags, esc); > >> + q_real = string_escape_mem(in, p, out_real, out_size, flags, esc); > >> > >> test_string_check_buf(name, flags, in, p, out_real, q_real, out_test, > >> q_test); > >> + > >> + 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); > >> + > > > > 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. > since we do this for all the > "positive" test cases, instead of just the single ad hoc case done previously. Of > course the added lines could be factored into a separate helper, but > there's quite a lot of state to pass, so I thought this would actually > be simpler - note how the two string_escape_mem calls are easily seen to > be identical except for the outsize argument. > > It may already be too late for the merge window, but I didn't want to > spend too much time on these mostly cosmetic details (that also goes for > the 3- versus 2-line issue). Yes, too late, thus it's enough time to address my comments :-) On the other hand I actually don't know if it's a good idea to push this series to stable. I guess you may just put Fixes: tags in the patches 1/3, 3/3 w/o Cc'ing to stable since we have no issues with current users. -- Andy Shevchenko <andriy.shevchenko@intel.com> Intel Finland Oy ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v3 3/3] lib/string_helpers.c: Change semantics of string_escape_mem @ 2015-02-10 14:22 ` Andy Shevchenko 0 siblings, 0 replies; 52+ messages in thread From: Andy Shevchenko @ 2015-02-10 14:22 UTC (permalink / raw) To: Rasmus Villemoes Cc: Andrew Morton, Trond Myklebust, J. Bruce Fields, David S. Miller, linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-nfs-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA On Tue, 2015-02-10 at 14:02 +0100, Rasmus Villemoes wrote: > On Tue, Feb 10 2015, Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote: > > > On Tue, 2015-02-10 at 00:44 +0100, Rasmus Villemoes wrote: > >> The current semantics of string_escape_mem are inadequate for one of > >> its current users, vsnprintf(). If that is to honour its contract, it > >> must know how much space would be needed for the entire escaped > >> buffer, and string_escape_mem provides no way of obtaining that (short > >> of allocating a large enough buffer (~4 times input string) to let it > >> play with, and that's definitely a big no-no inside vsnprintf). > >> > >> So change the semantics for string_escape_mem to be more > >> snprintf-like: Return the size of the output that would be generated > >> if the destination buffer was big enough, but of course still only > >> write to the part of dst it is allowed to, and don't do > >> '\0'-termination. It is then up to the caller to detect whether output > >> was truncated and to append a '\0' if desired. Also, we must output > >> partial escape sequences, otherwise a call such as snprintf(buf, 3, > >> "%1pE", "\123") would cause printf to write a \0 to buf[2] but leaving > >> buf[0] and buf[1] with whatever they previously contained. > >> > >> This also fixes a bug in the escaped_string() helper function, which > >> used to unconditionally pass a length of "end-buf" to > >> string_escape_mem(); since the latter doesn't check osz for being > >> insanely large, it would happily write to dst. For example, > >> kasprintf(GFP_KERNEL, "something and then %pE", ...); is an easy way > >> to trigger an oops. > >> > >> In test-string_helpers.c, I removed the now meaningless -ENOMEM test, > >> and replaced it with testing for getting the expected return value > >> even if the buffer is too small. Also ensure that nothing is written > >> when osz == 0. > >> > >> In net/sunrpc/cache.c, I think qword_add still has the same > >> semantics. Someone should definitely double-check this. > > > > Thanks for an update. My comments below. > > After addressing 'em, wrt changes to patch 2/3, take my > > Acked-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> > > > > for all parts except net/sunrpc/cache.c. > > > >> > >> Signed-off-by: Rasmus Villemoes <linux-qQsb+v5E8BnlAoU/VqSP6n9LOBIZ5rWg@public.gmane.org> > >> --- > >> index ab0d30e1e18f..5f759c3c2f60 100644 > >> --- a/lib/test-string_helpers.c > >> +++ b/lib/test-string_helpers.c > >> @@ -264,12 +264,12 @@ static __init void test_string_escape(const char *name, > >> const struct test_string_2 *s2, > >> unsigned int flags, const char *esc) > >> { > >> - int q_real = 512; > >> - char *out_test = kmalloc(q_real, GFP_KERNEL); > >> - char *out_real = kmalloc(q_real, GFP_KERNEL); > >> + size_t out_size = 512; > >> + char *out_test = kmalloc(out_size, GFP_KERNEL); > >> + char *out_real = kmalloc(out_size, GFP_KERNEL); > >> char *in = kmalloc(256, GFP_KERNEL); > >> - char *buf = out_real; > >> int p = 0, q_test = 0; > >> + int q_real; > >> > >> if (!out_test || !out_real || !in) > >> goto out; > >> @@ -301,29 +301,26 @@ static __init void test_string_escape(const char *name, > >> q_test += len; > >> } > >> > >> - q_real = string_escape_mem(in, p, &buf, q_real, flags, esc); > >> + q_real = string_escape_mem(in, p, out_real, out_size, flags, esc); > >> > >> test_string_check_buf(name, flags, in, p, out_real, q_real, out_test, > >> q_test); > >> + > >> + 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); > >> + > > > > 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. > since we do this for all the > "positive" test cases, instead of just the single ad hoc case done previously. Of > course the added lines could be factored into a separate helper, but > there's quite a lot of state to pass, so I thought this would actually > be simpler - note how the two string_escape_mem calls are easily seen to > be identical except for the outsize argument. > > It may already be too late for the merge window, but I didn't want to > spend too much time on these mostly cosmetic details (that also goes for > the 3- versus 2-line issue). Yes, too late, thus it's enough time to address my comments :-) On the other hand I actually don't know if it's a good idea to push this series to stable. I guess you may just put Fixes: tags in the patches 1/3, 3/3 w/o Cc'ing to stable since we have no issues with current users. -- 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v3 3/3] lib/string_helpers.c: Change semantics of string_escape_mem 2015-02-10 14:22 ` Andy Shevchenko (?) @ 2015-02-21 1:35 ` Rasmus Villemoes 2015-02-23 12:50 ` Andy Shevchenko -1 siblings, 1 reply; 52+ messages in thread From: Rasmus Villemoes @ 2015-02-21 1:35 UTC (permalink / raw) To: Andy Shevchenko Cc: Andrew Morton, Trond Myklebust, J. Bruce Fields, David S. Miller, linux-kernel, linux-nfs, netdev On Tue, Feb 10 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: >> >> --- >> >> index ab0d30e1e18f..5f759c3c2f60 100644 >> >> --- a/lib/test-string_helpers.c >> >> +++ b/lib/test-string_helpers.c >> >> @@ -264,12 +264,12 @@ static __init void test_string_escape(const char *name, >> >> const struct test_string_2 *s2, >> >> unsigned int flags, const char *esc) >> >> { >> >> - int q_real = 512; >> >> - char *out_test = kmalloc(q_real, GFP_KERNEL); >> >> - char *out_real = kmalloc(q_real, GFP_KERNEL); >> >> + size_t out_size = 512; >> >> + char *out_test = kmalloc(out_size, GFP_KERNEL); >> >> + char *out_real = kmalloc(out_size, GFP_KERNEL); >> >> char *in = kmalloc(256, GFP_KERNEL); >> >> - char *buf = out_real; >> >> int p = 0, q_test = 0; >> >> + int q_real; >> >> >> >> if (!out_test || !out_real || !in) >> >> goto out; >> >> @@ -301,29 +301,26 @@ static __init void test_string_escape(const char *name, >> >> q_test += len; >> >> } >> >> >> >> - q_real = string_escape_mem(in, p, &buf, q_real, flags, esc); >> >> + q_real = string_escape_mem(in, p, out_real, out_size, flags, esc); >> >> >> >> test_string_check_buf(name, flags, in, p, out_real, q_real, out_test, >> >> q_test); >> >> + >> >> + 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); >> >> + >> > >> > 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. 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. I've updated 2/3 with the early returns you suggested, but I'll wait a little before sending out a v4. Rasmus ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v3 3/3] lib/string_helpers.c: Change semantics of string_escape_mem @ 2015-02-23 12:50 ` Andy Shevchenko 0 siblings, 0 replies; 52+ messages in thread From: Andy Shevchenko @ 2015-02-23 12:50 UTC (permalink / raw) To: Rasmus Villemoes Cc: Andrew Morton, Trond Myklebust, J. Bruce Fields, David S. Miller, linux-kernel, linux-nfs, netdev On Sat, 2015-02-21 at 02:35 +0100, Rasmus Villemoes wrote: > On Tue, Feb 10 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > >> >> @@ -301,29 +301,26 @@ static __init void test_string_escape(const char *name, > >> >> q_test += len; > >> >> } > >> >> > >> >> - q_real = string_escape_mem(in, p, &buf, q_real, flags, esc); > >> >> + q_real = string_escape_mem(in, p, out_real, out_size, flags, esc); > >> >> > >> >> test_string_check_buf(name, flags, in, p, out_real, q_real, out_test, > >> >> q_test); > >> >> + > >> >> + 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); > >> >> + > >> > > >> > 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? > I've updated 2/3 with the early returns you suggested, but I'll wait a > little before sending out a v4. -- Andy Shevchenko <andriy.shevchenko@intel.com> Intel Finland Oy ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v3 3/3] lib/string_helpers.c: Change semantics of string_escape_mem @ 2015-02-23 12:50 ` Andy Shevchenko 0 siblings, 0 replies; 52+ messages in thread From: Andy Shevchenko @ 2015-02-23 12:50 UTC (permalink / raw) To: Rasmus Villemoes Cc: Andrew Morton, Trond Myklebust, J. Bruce Fields, David S. Miller, linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-nfs-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA On Sat, 2015-02-21 at 02:35 +0100, Rasmus Villemoes wrote: > On Tue, Feb 10 2015, Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote: > >> >> @@ -301,29 +301,26 @@ static __init void test_string_escape(const char *name, > >> >> q_test += len; > >> >> } > >> >> > >> >> - q_real = string_escape_mem(in, p, &buf, q_real, flags, esc); > >> >> + q_real = string_escape_mem(in, p, out_real, out_size, flags, esc); > >> >> > >> >> test_string_check_buf(name, flags, in, p, out_real, q_real, out_test, > >> >> q_test); > >> >> + > >> >> + 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); > >> >> + > >> > > >> > 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? > I've updated 2/3 with the early returns you suggested, but I'll wait a > little before sending out a v4. -- 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v3 3/3] lib/string_helpers.c: Change semantics of string_escape_mem @ 2015-02-23 22:55 ` Rasmus Villemoes 0 siblings, 0 replies; 52+ messages in thread From: Rasmus Villemoes @ 2015-02-23 22:55 UTC (permalink / raw) To: Andy Shevchenko Cc: Andrew Morton, Trond Myklebust, J. Bruce Fields, David S. Miller, linux-kernel, linux-nfs, netdev 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 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH v3 3/3] lib/string_helpers.c: Change semantics of string_escape_mem @ 2015-02-23 22:55 ` Rasmus Villemoes 0 siblings, 0 replies; 52+ messages in thread From: Rasmus Villemoes @ 2015-02-23 22:55 UTC (permalink / raw) To: Andy Shevchenko Cc: Andrew Morton, Trond Myklebust, J. Bruce Fields, David S. Miller, linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-nfs-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA 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 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH v3 3/3] lib/string_helpers.c: Change semantics of string_escape_mem @ 2015-03-02 12:37 ` Andy Shevchenko 0 siblings, 0 replies; 52+ messages in thread From: Andy Shevchenko @ 2015-03-02 12:37 UTC (permalink / raw) To: Rasmus Villemoes Cc: Andrew Morton, Trond Myklebust, J. Bruce Fields, David S. Miller, linux-kernel, linux-nfs, netdev 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v3 3/3] lib/string_helpers.c: Change semantics of string_escape_mem @ 2015-03-02 12:37 ` Andy Shevchenko 0 siblings, 0 replies; 52+ messages in thread From: Andy Shevchenko @ 2015-03-02 12:37 UTC (permalink / raw) To: Rasmus Villemoes Cc: Andrew Morton, Trond Myklebust, J. Bruce Fields, David S. Miller, linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-nfs-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v3 3/3] lib/string_helpers.c: Change semantics of string_escape_mem 2015-03-02 12:37 ` Andy Shevchenko (?) @ 2015-03-02 23:03 ` Rasmus Villemoes 2015-03-03 10:26 ` Andy Shevchenko -1 siblings, 1 reply; 52+ messages in thread From: Rasmus Villemoes @ 2015-03-02 23:03 UTC (permalink / raw) To: Andy Shevchenko Cc: Andrew Morton, Trond Myklebust, J. Bruce Fields, David S. Miller, linux-kernel, linux-nfs, netdev On Mon, Mar 02 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > 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: > >> > 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. Or maybe not at all: We could pass NULL, 0, which is what has to work anyway for the kasprintf case - failure will then be detected through an oops, but I think that should be ok. That would also remove the memset and memchr_inv calls above. I don't like the idea of just defining a small stack buffer (say buf[16]) and passing that (still with a size of 0): It's better to either detect writes directly (by passing a large enough buffer with known contents), or indirectly through an oops, as opposed to having to figure it out from random stack corruption. And kmalloc'ing+error handling+kfree'ing a buffer inside the overflow check would just be plain silly, when we have a large enough buffer already. >> 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 = … Yes, that will make the patch even smaller, just touching that single line. Done. > 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. I don't think we need to care. AFAICT, task_name is only used for /proc/*/status, and it is the first thing to fill anything into the seq_file, so overflow is impossible. [Testing for pre-existing overflow also wouldn't be enough; one should check whether there's actually room for ~32 bytes.] As I said, I do think that longer-term one shouldn't have to poke around in the seq_file internals, but for now I'd like to make the patch minimal. >> 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()). Well, %pT is a completely new addition to vsprintf.c. Also, I don't think that would be a very good match - not every user of %pT might want escaping, so at the very least this would require implementing some extra flags for %pT. But if task_name would be the only user of those flags, I think the escaping logic is better kept there. Anyway, this is outside this series' scope. Rasmus ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v3 3/3] lib/string_helpers.c: Change semantics of string_escape_mem @ 2015-03-03 10:26 ` Andy Shevchenko 0 siblings, 0 replies; 52+ messages in thread From: Andy Shevchenko @ 2015-03-03 10:26 UTC (permalink / raw) To: Rasmus Villemoes Cc: Andrew Morton, Trond Myklebust, J. Bruce Fields, David S. Miller, linux-kernel, linux-nfs, netdev On Tue, 2015-03-03 at 00:03 +0100, Rasmus Villemoes wrote: > On Mon, Mar 02 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > 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: > > > >> > 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. > > Or maybe not at all: We could pass NULL, 0, which is what has to work > anyway for the kasprintf case - failure will then be detected through an > oops, but I think that should be ok. That would also remove the memset and > memchr_inv calls above. > > I don't like the idea of just defining a small stack buffer (say > buf[16]) and passing that (still with a size of 0): It's better to > either detect writes directly (by passing a large enough buffer with > known contents), or indirectly through an oops, as opposed to having to > figure it out from random stack corruption. And kmalloc'ing+error > handling+kfree'ing a buffer inside the overflow check would just be > plain silly, when we have a large enough buffer already. Come with v4, I think I have no big objections to the approach. > As I said, I do think that longer-term one shouldn't have to poke around > in the seq_file internals, but for now I'd like to make the patch minimal. Ok. > >> 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()). > > Well, %pT is a completely new addition to vsprintf.c. Also, I don't > think that would be a very good match - not every user of %pT might want > escaping, so at the very least this would require implementing some > extra flags for %pT. Something like %pTe (for 'sanely Escaped' with flags you proposed earlier) ? > But if task_name would be the only user of those > flags, I think the escaping logic is better kept there. Anyway, this is > outside this series' scope. Yes. -- Andy Shevchenko <andriy.shevchenko@intel.com> Intel Finland Oy ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v3 3/3] lib/string_helpers.c: Change semantics of string_escape_mem @ 2015-03-03 10:26 ` Andy Shevchenko 0 siblings, 0 replies; 52+ messages in thread From: Andy Shevchenko @ 2015-03-03 10:26 UTC (permalink / raw) To: Rasmus Villemoes Cc: Andrew Morton, Trond Myklebust, J. Bruce Fields, David S. Miller, linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-nfs-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA On Tue, 2015-03-03 at 00:03 +0100, Rasmus Villemoes wrote: > On Mon, Mar 02 2015, Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote: > > On Mon, 2015-02-23 at 23:55 +0100, Rasmus Villemoes wrote: > >> On Mon, Feb 23 2015, Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote: > > > >> > 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. > > Or maybe not at all: We could pass NULL, 0, which is what has to work > anyway for the kasprintf case - failure will then be detected through an > oops, but I think that should be ok. That would also remove the memset and > memchr_inv calls above. > > I don't like the idea of just defining a small stack buffer (say > buf[16]) and passing that (still with a size of 0): It's better to > either detect writes directly (by passing a large enough buffer with > known contents), or indirectly through an oops, as opposed to having to > figure it out from random stack corruption. And kmalloc'ing+error > handling+kfree'ing a buffer inside the overflow check would just be > plain silly, when we have a large enough buffer already. Come with v4, I think I have no big objections to the approach. > As I said, I do think that longer-term one shouldn't have to poke around > in the seq_file internals, but for now I'd like to make the patch minimal. Ok. > >> 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()). > > Well, %pT is a completely new addition to vsprintf.c. Also, I don't > think that would be a very good match - not every user of %pT might want > escaping, so at the very least this would require implementing some > extra flags for %pT. Something like %pTe (for 'sanely Escaped' with flags you proposed earlier) ? > But if task_name would be the only user of those > flags, I think the escaping logic is better kept there. Anyway, this is > outside this series' scope. Yes. -- 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v4 0/3] Two printf fixes 2015-02-09 23:44 ` Rasmus Villemoes ` (3 preceding siblings ...) (?) @ 2015-03-03 23:20 ` Rasmus Villemoes 2015-03-03 23:20 ` [PATCH v4 1/3] lib/vsprintf.c: Fix potential NULL deref in hex_string Rasmus Villemoes ` (2 more replies) -1 siblings, 3 replies; 52+ messages in thread From: Rasmus Villemoes @ 2015-03-03 23:20 UTC (permalink / raw) To: Andrew Morton, Andy Shevchenko, J. Bruce Fields, Trond Myklebust, Anna Schumaker, David S. Miller Cc: Rasmus Villemoes, linux-kernel, linux-nfs, netdev Both %pE and %ph are unusable in kasprintf(), since the occurrence of either will trigger an oops during the first vsnprintf call where kasprintf tries to find the correct size to allocate. These oopses could be papered over with somewhat smaller patches than these, but then the return value from vsnprintf would still not reflect the actual size needed. For %pE, this requires a change of semantics of string_escape_mem and hence an annoyingly large diffstat. v4: Also update fs/proc/array.c, factor overflow test into separate helper, minor stylistic changes. Changed in v3: * Add Andy's ack to 1/3. * Ensure that string_escape_mem doesn't output partial escape sequences after 2/3, while still preparing for it to do exactly that in 3/3. * Leave the return value of string_escape_mem as int. v2: Suggestions from Andy Shevchenko: * Simpler fix of hex_string(). * The string_escape_mem change is split in two, 2/3 updating the internal helpers and 3/3 then changing the external interface. Rasmus Villemoes (3): lib/vsprintf.c: Fix potential NULL deref in hex_string lib/string_helpers.c: Refactor string_escape_mem lib/string_helpers.c: Change semantics of string_escape_mem fs/proc/array.c | 4 +- include/linux/string_helpers.h | 8 +- lib/string_helpers.c | 193 +++++++++++++++++------------------------ lib/test-string_helpers.c | 38 ++++---- lib/vsprintf.c | 24 +++-- net/sunrpc/cache.c | 8 +- 6 files changed, 127 insertions(+), 148 deletions(-) -- 2.1.3 ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v4 1/3] lib/vsprintf.c: Fix potential NULL deref in hex_string 2015-03-03 23:20 ` [PATCH v4 0/3] Two printf fixes Rasmus Villemoes @ 2015-03-03 23:20 ` Rasmus Villemoes 2015-03-03 23:20 ` [PATCH v4 2/3] lib/string_helpers.c: Refactor string_escape_mem Rasmus Villemoes 2015-03-03 23:20 ` [PATCH v4 3/3] lib/string_helpers.c: Change semantics of string_escape_mem Rasmus Villemoes 2 siblings, 0 replies; 52+ messages in thread From: Rasmus Villemoes @ 2015-03-03 23:20 UTC (permalink / raw) To: Andrew Morton, Andy Shevchenko, Rasmus Villemoes, Peter Zijlstra (Intel), Tejun Heo Cc: linux-kernel The helper hex_string() is broken in two ways. First, it doesn't increment buf regardless of whether there is room to print, so callers such as kasprintf() that try to probe the correct storage to allocate will get a too small return value. But even worse, kasprintf() (and likely anyone else trying to find the size of the result) pass NULL for buf and 0 for size, so we also have end == NULL. But this means that the end-1 in hex_string() is (char*)-1, so buf < end-1 is true and we get a NULL pointer deref. I double-checked this with a trivial kernel module that just did a kasprintf(GFP_KERNEL, "%14ph", "CrashBoomBang"). Nobody seems to be using %ph with kasprintf, but we might as well fix it before it hits someone. Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> --- lib/vsprintf.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/lib/vsprintf.c b/lib/vsprintf.c index b235c96167d3..a1b6487c6710 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -783,11 +783,19 @@ char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec, if (spec.field_width > 0) len = min_t(int, spec.field_width, 64); - for (i = 0; i < len && buf < end - 1; i++) { - buf = hex_byte_pack(buf, addr[i]); + for (i = 0; i < len; ++i) { + if (buf < end) + *buf = hex_asc_hi(addr[i]); + ++buf; + if (buf < end) + *buf = hex_asc_lo(addr[i]); + ++buf; - if (buf < end && separator && i != len - 1) - *buf++ = separator; + if (separator && i != len - 1) { + if (buf < end) + *buf = separator; + ++buf; + } } return buf; -- 2.1.3 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH v4 2/3] lib/string_helpers.c: Refactor string_escape_mem 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 ` 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 2 siblings, 1 reply; 52+ messages in thread From: Rasmus Villemoes @ 2015-03-03 23:20 UTC (permalink / raw) To: Andrew Morton, Andy Shevchenko, Rasmus Villemoes; +Cc: linux-kernel When printf is given the format specifier %pE, it needs a way of obtaining the total output size that would be generated if the buffer was large enough, and string_escape_mem doesn't easily provide that. This is a refactorization of string_escape_mem in preparation of changing its external API to provide that information. The somewhat ugly early returns and subsequent seemingly redundant conditionals are to make the following patch touch as little as possible in string_helpers.c while still preserving the current behaviour of never outputting partial escape sequences. That behaviour must also change for %pE to work as one expects from every other printf specifier. Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> --- lib/string_helpers.c | 208 ++++++++++++++++++++++++++------------------------- 1 file changed, 105 insertions(+), 103 deletions(-) diff --git a/lib/string_helpers.c b/lib/string_helpers.c index 8f8c4417f228..9c48ddad0f0d 100644 --- a/lib/string_helpers.c +++ b/lib/string_helpers.c @@ -239,29 +239,21 @@ int string_unescape(char *src, char *dst, size_t size, unsigned int flags) } EXPORT_SYMBOL(string_unescape); -static int escape_passthrough(unsigned char c, char **dst, size_t *osz) +static bool escape_passthrough(unsigned char c, char **dst, char *end) { char *out = *dst; - if (*osz < 1) - return -ENOMEM; - - *out++ = c; - - *dst = out; - *osz -= 1; - - return 1; + if (out < end) + *out = c; + *dst = out + 1; + return true; } -static int escape_space(unsigned char c, char **dst, size_t *osz) +static bool escape_space(unsigned char c, char **dst, char *end) { char *out = *dst; unsigned char to; - if (*osz < 2) - return -ENOMEM; - switch (c) { case '\n': to = 'n'; @@ -279,26 +271,30 @@ static int escape_space(unsigned char c, char **dst, size_t *osz) to = 'f'; break; default: - return 0; + return false; } - *out++ = '\\'; - *out++ = to; + if (out + 2 > end) { + *dst = out + 2; + return true; + } - *dst = out; - *osz -= 2; + if (out < end) + *out = '\\'; + ++out; + if (out < end) + *out = to; + ++out; - return 1; + *dst = out; + return true; } -static int escape_special(unsigned char c, char **dst, size_t *osz) +static bool escape_special(unsigned char c, char **dst, char *end) { char *out = *dst; unsigned char to; - if (*osz < 2) - return -ENOMEM; - switch (c) { case '\\': to = '\\'; @@ -310,71 +306,98 @@ static int escape_special(unsigned char c, char **dst, size_t *osz) to = 'e'; break; default: - return 0; + return false; } - *out++ = '\\'; - *out++ = to; + if (out + 2 > end) { + *dst = out + 2; + return true; + } - *dst = out; - *osz -= 2; + if (out < end) + *out = '\\'; + ++out; + if (out < end) + *out = to; + ++out; - return 1; + *dst = out; + return true; } -static int escape_null(unsigned char c, char **dst, size_t *osz) +static bool escape_null(unsigned char c, char **dst, char *end) { char *out = *dst; - if (*osz < 2) - return -ENOMEM; - if (c) - return 0; + return false; - *out++ = '\\'; - *out++ = '0'; + if (out + 2 > end) { + *dst = out + 2; + return true; + } - *dst = out; - *osz -= 2; + if (out < end) + *out = '\\'; + ++out; + if (out < end) + *out = '0'; + ++out; - return 1; + *dst = out; + return true; } -static int escape_octal(unsigned char c, char **dst, size_t *osz) +static bool escape_octal(unsigned char c, char **dst, char *end) { char *out = *dst; - if (*osz < 4) - return -ENOMEM; + if (out + 4 > end) { + *dst = out + 4; + return true; + } - *out++ = '\\'; - *out++ = ((c >> 6) & 0x07) + '0'; - *out++ = ((c >> 3) & 0x07) + '0'; - *out++ = ((c >> 0) & 0x07) + '0'; + if (out < end) + *out = '\\'; + ++out; + if (out < end) + *out = ((c >> 6) & 0x07) + '0'; + ++out; + if (out < end) + *out = ((c >> 3) & 0x07) + '0'; + ++out; + if (out < end) + *out = ((c >> 0) & 0x07) + '0'; + ++out; *dst = out; - *osz -= 4; - - return 1; + return true; } -static int escape_hex(unsigned char c, char **dst, size_t *osz) +static bool escape_hex(unsigned char c, char **dst, char *end) { char *out = *dst; - if (*osz < 4) - return -ENOMEM; + if (out + 4 > end) { + *dst = out + 4; + return true; + } - *out++ = '\\'; - *out++ = 'x'; - *out++ = hex_asc_hi(c); - *out++ = hex_asc_lo(c); + if (out < end) + *out = '\\'; + ++out; + if (out < end) + *out = 'x'; + ++out; + if (out < end) + *out = hex_asc_hi(c); + ++out; + if (out < end) + *out = hex_asc_lo(c); + ++out; *dst = out; - *osz -= 4; - - return 1; + return true; } /** @@ -436,9 +459,10 @@ static int escape_hex(unsigned char c, char **dst, size_t *osz) int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, unsigned int flags, const char *esc) { - char *out = *dst, *p = out; + char *p = *dst; + char *end = p + osz; bool is_dict = esc && *esc; - int ret = 0; + int ret; while (isz--) { unsigned char c = *src++; @@ -458,55 +482,33 @@ int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, (is_dict && !strchr(esc, c))) { /* do nothing */ } else { - if (flags & ESCAPE_SPACE) { - ret = escape_space(c, &p, &osz); - if (ret < 0) - break; - if (ret > 0) - continue; - } - - if (flags & ESCAPE_SPECIAL) { - ret = escape_special(c, &p, &osz); - if (ret < 0) - break; - if (ret > 0) - continue; - } - - if (flags & ESCAPE_NULL) { - ret = escape_null(c, &p, &osz); - if (ret < 0) - break; - if (ret > 0) - continue; - } + if (flags & ESCAPE_SPACE && escape_space(c, &p, end)) + continue; + + if (flags & ESCAPE_SPECIAL && escape_special(c, &p, end)) + continue; + + if (flags & ESCAPE_NULL && escape_null(c, &p, end)) + continue; /* ESCAPE_OCTAL and ESCAPE_HEX always go last */ - if (flags & ESCAPE_OCTAL) { - ret = escape_octal(c, &p, &osz); - if (ret < 0) - break; + if (flags & ESCAPE_OCTAL && escape_octal(c, &p, end)) continue; - } - if (flags & ESCAPE_HEX) { - ret = escape_hex(c, &p, &osz); - if (ret < 0) - break; + + if (flags & ESCAPE_HEX && escape_hex(c, &p, end)) continue; - } } - ret = escape_passthrough(c, &p, &osz); - if (ret < 0) - break; + escape_passthrough(c, &p, end); } - *dst = p; - - if (ret < 0) - return ret; + if (p > end) { + *dst = end; + return -ENOMEM; + } - return p - out; + ret = p - *dst; + *dst = p; + return ret; } EXPORT_SYMBOL(string_escape_mem); -- 2.1.3 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH v4 2/3] lib/string_helpers.c: Refactor string_escape_mem 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 0 siblings, 0 replies; 52+ messages in thread From: Andy Shevchenko @ 2015-03-04 10:51 UTC (permalink / raw) To: Rasmus Villemoes; +Cc: Andrew Morton, linux-kernel On Wed, 2015-03-04 at 00:20 +0100, Rasmus Villemoes wrote: > When printf is given the format specifier %pE, it needs a way of > obtaining the total output size that would be generated if the buffer > was large enough, and string_escape_mem doesn't easily provide > that. This is a refactorization of string_escape_mem in preparation of > changing its external API to provide that information. > > The somewhat ugly early returns and subsequent seemingly redundant > conditionals are to make the following patch touch as little as > possible in string_helpers.c while still preserving the current > behaviour of never outputting partial escape sequences. That behaviour > must also change for %pE to work as one expects from every other > printf specifier. > > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> Thanks for an update! Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > lib/string_helpers.c | 208 ++++++++++++++++++++++++++------------------------- > 1 file changed, 105 insertions(+), 103 deletions(-) > > diff --git a/lib/string_helpers.c b/lib/string_helpers.c > index 8f8c4417f228..9c48ddad0f0d 100644 > --- a/lib/string_helpers.c > +++ b/lib/string_helpers.c > @@ -239,29 +239,21 @@ int string_unescape(char *src, char *dst, size_t size, unsigned int flags) > } > EXPORT_SYMBOL(string_unescape); > > -static int escape_passthrough(unsigned char c, char **dst, size_t *osz) > +static bool escape_passthrough(unsigned char c, char **dst, char *end) > { > char *out = *dst; > > - if (*osz < 1) > - return -ENOMEM; > - > - *out++ = c; > - > - *dst = out; > - *osz -= 1; > - > - return 1; > + if (out < end) > + *out = c; > + *dst = out + 1; > + return true; > } > > -static int escape_space(unsigned char c, char **dst, size_t *osz) > +static bool escape_space(unsigned char c, char **dst, char *end) > { > char *out = *dst; > unsigned char to; > > - if (*osz < 2) > - return -ENOMEM; > - > switch (c) { > case '\n': > to = 'n'; > @@ -279,26 +271,30 @@ static int escape_space(unsigned char c, char **dst, size_t *osz) > to = 'f'; > break; > default: > - return 0; > + return false; > } > > - *out++ = '\\'; > - *out++ = to; > + if (out + 2 > end) { > + *dst = out + 2; > + return true; > + } > > - *dst = out; > - *osz -= 2; > + if (out < end) > + *out = '\\'; > + ++out; > + if (out < end) > + *out = to; > + ++out; > > - return 1; > + *dst = out; > + return true; > } > > -static int escape_special(unsigned char c, char **dst, size_t *osz) > +static bool escape_special(unsigned char c, char **dst, char *end) > { > char *out = *dst; > unsigned char to; > > - if (*osz < 2) > - return -ENOMEM; > - > switch (c) { > case '\\': > to = '\\'; > @@ -310,71 +306,98 @@ static int escape_special(unsigned char c, char **dst, size_t *osz) > to = 'e'; > break; > default: > - return 0; > + return false; > } > > - *out++ = '\\'; > - *out++ = to; > + if (out + 2 > end) { > + *dst = out + 2; > + return true; > + } > > - *dst = out; > - *osz -= 2; > + if (out < end) > + *out = '\\'; > + ++out; > + if (out < end) > + *out = to; > + ++out; > > - return 1; > + *dst = out; > + return true; > } > > -static int escape_null(unsigned char c, char **dst, size_t *osz) > +static bool escape_null(unsigned char c, char **dst, char *end) > { > char *out = *dst; > > - if (*osz < 2) > - return -ENOMEM; > - > if (c) > - return 0; > + return false; > > - *out++ = '\\'; > - *out++ = '0'; > + if (out + 2 > end) { > + *dst = out + 2; > + return true; > + } > > - *dst = out; > - *osz -= 2; > + if (out < end) > + *out = '\\'; > + ++out; > + if (out < end) > + *out = '0'; > + ++out; > > - return 1; > + *dst = out; > + return true; > } > > -static int escape_octal(unsigned char c, char **dst, size_t *osz) > +static bool escape_octal(unsigned char c, char **dst, char *end) > { > char *out = *dst; > > - if (*osz < 4) > - return -ENOMEM; > + if (out + 4 > end) { > + *dst = out + 4; > + return true; > + } > > - *out++ = '\\'; > - *out++ = ((c >> 6) & 0x07) + '0'; > - *out++ = ((c >> 3) & 0x07) + '0'; > - *out++ = ((c >> 0) & 0x07) + '0'; > + if (out < end) > + *out = '\\'; > + ++out; > + if (out < end) > + *out = ((c >> 6) & 0x07) + '0'; > + ++out; > + if (out < end) > + *out = ((c >> 3) & 0x07) + '0'; > + ++out; > + if (out < end) > + *out = ((c >> 0) & 0x07) + '0'; > + ++out; > > *dst = out; > - *osz -= 4; > - > - return 1; > + return true; > } > > -static int escape_hex(unsigned char c, char **dst, size_t *osz) > +static bool escape_hex(unsigned char c, char **dst, char *end) > { > char *out = *dst; > > - if (*osz < 4) > - return -ENOMEM; > + if (out + 4 > end) { > + *dst = out + 4; > + return true; > + } > > - *out++ = '\\'; > - *out++ = 'x'; > - *out++ = hex_asc_hi(c); > - *out++ = hex_asc_lo(c); > + if (out < end) > + *out = '\\'; > + ++out; > + if (out < end) > + *out = 'x'; > + ++out; > + if (out < end) > + *out = hex_asc_hi(c); > + ++out; > + if (out < end) > + *out = hex_asc_lo(c); > + ++out; > > *dst = out; > - *osz -= 4; > - > - return 1; > + return true; > } > > /** > @@ -436,9 +459,10 @@ static int escape_hex(unsigned char c, char **dst, size_t *osz) > int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, > unsigned int flags, const char *esc) > { > - char *out = *dst, *p = out; > + char *p = *dst; > + char *end = p + osz; > bool is_dict = esc && *esc; > - int ret = 0; > + int ret; > > while (isz--) { > unsigned char c = *src++; > @@ -458,55 +482,33 @@ int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, > (is_dict && !strchr(esc, c))) { > /* do nothing */ > } else { > - if (flags & ESCAPE_SPACE) { > - ret = escape_space(c, &p, &osz); > - if (ret < 0) > - break; > - if (ret > 0) > - continue; > - } > - > - if (flags & ESCAPE_SPECIAL) { > - ret = escape_special(c, &p, &osz); > - if (ret < 0) > - break; > - if (ret > 0) > - continue; > - } > - > - if (flags & ESCAPE_NULL) { > - ret = escape_null(c, &p, &osz); > - if (ret < 0) > - break; > - if (ret > 0) > - continue; > - } > + if (flags & ESCAPE_SPACE && escape_space(c, &p, end)) > + continue; > + > + if (flags & ESCAPE_SPECIAL && escape_special(c, &p, end)) > + continue; > + > + if (flags & ESCAPE_NULL && escape_null(c, &p, end)) > + continue; > > /* ESCAPE_OCTAL and ESCAPE_HEX always go last */ > - if (flags & ESCAPE_OCTAL) { > - ret = escape_octal(c, &p, &osz); > - if (ret < 0) > - break; > + if (flags & ESCAPE_OCTAL && escape_octal(c, &p, end)) > continue; > - } > - if (flags & ESCAPE_HEX) { > - ret = escape_hex(c, &p, &osz); > - if (ret < 0) > - break; > + > + if (flags & ESCAPE_HEX && escape_hex(c, &p, end)) > continue; > - } > } > > - ret = escape_passthrough(c, &p, &osz); > - if (ret < 0) > - break; > + escape_passthrough(c, &p, end); > } > > - *dst = p; > - > - if (ret < 0) > - return ret; > + if (p > end) { > + *dst = end; > + return -ENOMEM; > + } > > - return p - out; > + ret = p - *dst; > + *dst = p; > + return ret; > } > EXPORT_SYMBOL(string_escape_mem); -- Andy Shevchenko <andriy.shevchenko@intel.com> Intel Finland Oy ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v4 3/3] lib/string_helpers.c: Change semantics of string_escape_mem 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-03 23:20 ` Rasmus Villemoes 2015-03-04 11:49 ` Andy Shevchenko 2 siblings, 1 reply; 52+ messages in thread From: Rasmus Villemoes @ 2015-03-03 23:20 UTC (permalink / raw) To: Andrew Morton, Andy Shevchenko, J. Bruce Fields, Trond Myklebust, Anna Schumaker, David S. Miller Cc: Rasmus Villemoes, linux-kernel, linux-nfs, netdev The current semantics of string_escape_mem are inadequate for one of its current users, vsnprintf(). If that is to honour its contract, it must know how much space would be needed for the entire escaped buffer, and string_escape_mem provides no way of obtaining that (short of allocating a large enough buffer (~4 times input string) to let it play with, and that's definitely a big no-no inside vsnprintf). So change the semantics for string_escape_mem to be more snprintf-like: Return the size of the output that would be generated if the destination buffer was big enough, but of course still only write to the part of dst it is allowed to, and (contrary to snprintf) don't do '\0'-termination. It is then up to the caller to detect whether output was truncated and to append a '\0' if desired. Also, we must output partial escape sequences, otherwise a call such as snprintf(buf, 3, "%1pE", "\123") would cause printf to write a \0 to buf[2] but leaving buf[0] and buf[1] with whatever they previously contained. This also fixes a bug in the escaped_string() helper function, which used to unconditionally pass a length of "end-buf" to string_escape_mem(); since the latter doesn't check osz for being insanely large, it would happily write to dst. For example, kasprintf(GFP_KERNEL, "something and then %pE", ...); is an easy way to trigger an oops. In test-string_helpers.c, the -ENOMEM test is replaced with testing for getting the expected return value even if the buffer is too small. We also ensure that nothing is written (by relying on a NULL pointer deref) if the output size is 0 by passing NULL - this has to work for kasprintf("%pE") to work. In net/sunrpc/cache.c, I think qword_add still has the same semantics. Someone should definitely double-check this. In fs/proc/array.c, I made the minimum possible change, but longer-term it should stop poking around in seq_file internals. Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> --- fs/proc/array.c | 4 ++-- include/linux/string_helpers.h | 8 +++---- lib/string_helpers.c | 49 ++++++------------------------------------ lib/test-string_helpers.c | 40 +++++++++++++++++----------------- lib/vsprintf.c | 8 +++++-- net/sunrpc/cache.c | 8 ++++--- 6 files changed, 44 insertions(+), 73 deletions(-) diff --git a/fs/proc/array.c b/fs/proc/array.c index 1295a00ca316..b5405889a780 100644 --- a/fs/proc/array.c +++ b/fs/proc/array.c @@ -99,8 +99,8 @@ 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\\"); + buf += string_escape_str(tcomm, buf, m->size - m->count, + ESCAPE_SPACE | ESCAPE_SPECIAL, "\n\\"); m->count = buf - m->buf; seq_putc(m, '\n'); diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h index 657571817260..0991913f4953 100644 --- a/include/linux/string_helpers.h +++ b/include/linux/string_helpers.h @@ -47,22 +47,22 @@ static inline int string_unescape_any_inplace(char *buf) #define ESCAPE_ANY_NP (ESCAPE_ANY | ESCAPE_NP) #define ESCAPE_HEX 0x20 -int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, +int string_escape_mem(const char *src, size_t isz, char *dst, size_t osz, unsigned int flags, const char *esc); static inline int string_escape_mem_any_np(const char *src, size_t isz, - char **dst, size_t osz, const char *esc) + char *dst, size_t osz, const char *esc) { return string_escape_mem(src, isz, dst, osz, ESCAPE_ANY_NP, esc); } -static inline int string_escape_str(const char *src, char **dst, size_t sz, +static inline int string_escape_str(const char *src, char *dst, size_t sz, unsigned int flags, const char *esc) { return string_escape_mem(src, strlen(src), dst, sz, flags, esc); } -static inline int string_escape_str_any_np(const char *src, char **dst, +static inline int string_escape_str_any_np(const char *src, char *dst, size_t sz, const char *esc) { return string_escape_str(src, dst, sz, ESCAPE_ANY_NP, esc); diff --git a/lib/string_helpers.c b/lib/string_helpers.c index 9c48ddad0f0d..1826c7407258 100644 --- a/lib/string_helpers.c +++ b/lib/string_helpers.c @@ -274,11 +274,6 @@ static bool escape_space(unsigned char c, char **dst, char *end) return false; } - if (out + 2 > end) { - *dst = out + 2; - return true; - } - if (out < end) *out = '\\'; ++out; @@ -309,11 +304,6 @@ static bool escape_special(unsigned char c, char **dst, char *end) return false; } - if (out + 2 > end) { - *dst = out + 2; - return true; - } - if (out < end) *out = '\\'; ++out; @@ -332,11 +322,6 @@ static bool escape_null(unsigned char c, char **dst, char *end) if (c) return false; - if (out + 2 > end) { - *dst = out + 2; - return true; - } - if (out < end) *out = '\\'; ++out; @@ -352,11 +337,6 @@ static bool escape_octal(unsigned char c, char **dst, char *end) { char *out = *dst; - if (out + 4 > end) { - *dst = out + 4; - return true; - } - if (out < end) *out = '\\'; ++out; @@ -378,11 +358,6 @@ static bool escape_hex(unsigned char c, char **dst, char *end) { char *out = *dst; - if (out + 4 > end) { - *dst = out + 4; - return true; - } - if (out < end) *out = '\\'; ++out; @@ -449,20 +424,17 @@ static bool escape_hex(unsigned char c, char **dst, char *end) * it if needs. * * Return: - * The amount of the characters processed to the destination buffer, or - * %-ENOMEM if the size of buffer is not enough to put an escaped character is - * returned. - * - * Even in the case of error @dst pointer will be updated to point to the byte - * after the last processed character. + * The total size of the escaped output that would be generated for + * the given input and flags. To check whether the output was + * truncated, compare the return value to osz. There is room left in + * dst for a '\0' terminator if and only if ret < osz. */ -int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, +int string_escape_mem(const char *src, size_t isz, char *dst, size_t osz, unsigned int flags, const char *esc) { - char *p = *dst; + char *p = dst; char *end = p + osz; bool is_dict = esc && *esc; - int ret; while (isz--) { unsigned char c = *src++; @@ -502,13 +474,6 @@ int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, escape_passthrough(c, &p, end); } - if (p > end) { - *dst = end; - return -ENOMEM; - } - - ret = p - *dst; - *dst = p; - return ret; + return p - dst; } EXPORT_SYMBOL(string_escape_mem); diff --git a/lib/test-string_helpers.c b/lib/test-string_helpers.c index ab0d30e1e18f..8e376efd88a4 100644 --- a/lib/test-string_helpers.c +++ b/lib/test-string_helpers.c @@ -260,16 +260,28 @@ static __init const char *test_string_find_match(const struct test_string_2 *s2, return NULL; } +static __init void +test_string_escape_overflow(const char *in, int p, unsigned int flags, const char *esc, + int q_test, const char *name) +{ + int q_real; + + q_real = string_escape_mem(in, p, NULL, 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); +} + static __init void test_string_escape(const char *name, const struct test_string_2 *s2, unsigned int flags, const char *esc) { - int q_real = 512; - char *out_test = kmalloc(q_real, GFP_KERNEL); - char *out_real = kmalloc(q_real, GFP_KERNEL); + size_t out_size = 512; + char *out_test = kmalloc(out_size, GFP_KERNEL); + char *out_real = kmalloc(out_size, GFP_KERNEL); char *in = kmalloc(256, GFP_KERNEL); - char *buf = out_real; int p = 0, q_test = 0; + int q_real; if (!out_test || !out_real || !in) goto out; @@ -301,29 +313,19 @@ static __init void test_string_escape(const char *name, q_test += len; } - q_real = string_escape_mem(in, p, &buf, q_real, flags, esc); + q_real = string_escape_mem(in, p, out_real, out_size, flags, esc); test_string_check_buf(name, flags, in, p, out_real, q_real, out_test, q_test); + + test_string_escape_overflow(in, p, flags, esc, q_test, name); + out: kfree(in); kfree(out_real); kfree(out_test); } -static __init void test_string_escape_nomem(void) -{ - char *in = "\eb \\C\007\"\x90\r]"; - char out[64], *buf = out; - int rc = -ENOMEM, ret; - - ret = string_escape_str_any_np(in, &buf, strlen(in), NULL); - if (ret == rc) - return; - - pr_err("Test 'escape nomem' failed: got %d instead of %d\n", ret, rc); -} - static int __init test_string_helpers_init(void) { unsigned int i; @@ -342,8 +344,6 @@ static int __init test_string_helpers_init(void) for (i = 0; i < (ESCAPE_ANY_NP | ESCAPE_HEX) + 1; i++) test_string_escape("escape 1", escape1, i, TEST_STRING_2_DICT_1); - test_string_escape_nomem(); - return -EINVAL; } module_init(test_string_helpers_init); diff --git a/lib/vsprintf.c b/lib/vsprintf.c index a1b6487c6710..9651686f3b42 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -1241,8 +1241,12 @@ char *escaped_string(char *buf, char *end, u8 *addr, struct printf_spec spec, len = spec.field_width < 0 ? 1 : spec.field_width; - /* Ignore the error. We print as many characters as we can */ - string_escape_mem(addr, len, &buf, end - buf, flags, NULL); + /* + * string_escape_mem() writes as many characters as it can to + * the given buffer, and returns the total size of the output + * had the buffer been big enough. + */ + buf += string_escape_mem(addr, len, buf, buf < end ? end - buf : 0, flags, NULL); return buf; } diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c index 33fb105d4352..22c4418057f4 100644 --- a/net/sunrpc/cache.c +++ b/net/sunrpc/cache.c @@ -1068,12 +1068,14 @@ void qword_add(char **bpp, int *lp, char *str) { char *bp = *bpp; int len = *lp; - int ret; + int ret, written; if (len < 0) return; - ret = string_escape_str(str, &bp, len, ESCAPE_OCTAL, "\\ \n\t"); - if (ret < 0 || ret == len) + ret = string_escape_str(str, bp, len, ESCAPE_OCTAL, "\\ \n\t"); + written = min(ret, len); + bp += written; + if (ret >= len) len = -1; else { len -= ret; -- 2.1.3 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH v4 3/3] lib/string_helpers.c: Change semantics of string_escape_mem @ 2015-03-04 11:49 ` Andy Shevchenko 0 siblings, 0 replies; 52+ messages in thread From: Andy Shevchenko @ 2015-03-04 11:49 UTC (permalink / raw) To: Rasmus Villemoes Cc: Andrew Morton, J. Bruce Fields, Trond Myklebust, Anna Schumaker, David S. Miller, linux-kernel, linux-nfs, netdev On Wed, 2015-03-04 at 00:20 +0100, Rasmus Villemoes wrote: > The current semantics of string_escape_mem are inadequate for one of > its current users, vsnprintf(). If that is to honour its contract, it > must know how much space would be needed for the entire escaped > buffer, and string_escape_mem provides no way of obtaining that (short > of allocating a large enough buffer (~4 times input string) to let it > play with, and that's definitely a big no-no inside vsnprintf). > > So change the semantics for string_escape_mem to be more > snprintf-like: Return the size of the output that would be generated > if the destination buffer was big enough, but of course still only > write to the part of dst it is allowed to, and (contrary to snprintf) > don't do '\0'-termination. It is then up to the caller to detect > whether output was truncated and to append a '\0' if desired. Also, we > must output partial escape sequences, otherwise a call such as > snprintf(buf, 3, "%1pE", "\123") would cause printf to write a \0 to > buf[2] but leaving buf[0] and buf[1] with whatever they previously > contained. > > This also fixes a bug in the escaped_string() helper function, which > used to unconditionally pass a length of "end-buf" to > string_escape_mem(); since the latter doesn't check osz for being > insanely large, it would happily write to dst. For example, > kasprintf(GFP_KERNEL, "something and then %pE", ...); is an easy way > to trigger an oops. > > In test-string_helpers.c, the -ENOMEM test is replaced with testing > for getting the expected return value even if the buffer is too > small. We also ensure that nothing is written (by relying on a NULL > pointer deref) if the output size is 0 by passing NULL - this has to > work for kasprintf("%pE") to work. > > In net/sunrpc/cache.c, I think qword_add still has the same > semantics. Someone should definitely double-check this. > > In fs/proc/array.c, I made the minimum possible change, but > longer-term it should stop poking around in seq_file internals. > > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> Thanks! One nitpick below, but anyway Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> for everything except net/sunrpc/cache.c > --- > fs/proc/array.c | 4 ++-- > include/linux/string_helpers.h | 8 +++---- > lib/string_helpers.c | 49 ++++++------------------------------------ > lib/test-string_helpers.c | 40 +++++++++++++++++----------------- > lib/vsprintf.c | 8 +++++-- > net/sunrpc/cache.c | 8 ++++--- > 6 files changed, 44 insertions(+), 73 deletions(-) > > diff --git a/fs/proc/array.c b/fs/proc/array.c > index 1295a00ca316..b5405889a780 100644 > --- a/fs/proc/array.c > +++ b/fs/proc/array.c > @@ -99,8 +99,8 @@ 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\\"); > + buf += string_escape_str(tcomm, buf, m->size - m->count, > + ESCAPE_SPACE | ESCAPE_SPECIAL, "\n\\"); > > m->count = buf - m->buf; > seq_putc(m, '\n'); > diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h > index 657571817260..0991913f4953 100644 > --- a/include/linux/string_helpers.h > +++ b/include/linux/string_helpers.h > @@ -47,22 +47,22 @@ static inline int string_unescape_any_inplace(char *buf) > #define ESCAPE_ANY_NP (ESCAPE_ANY | ESCAPE_NP) > #define ESCAPE_HEX 0x20 > > -int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, > +int string_escape_mem(const char *src, size_t isz, char *dst, size_t osz, > unsigned int flags, const char *esc); > > static inline int string_escape_mem_any_np(const char *src, size_t isz, > - char **dst, size_t osz, const char *esc) > + char *dst, size_t osz, const char *esc) > { > return string_escape_mem(src, isz, dst, osz, ESCAPE_ANY_NP, esc); > } > > -static inline int string_escape_str(const char *src, char **dst, size_t sz, > +static inline int string_escape_str(const char *src, char *dst, size_t sz, > unsigned int flags, const char *esc) > { > return string_escape_mem(src, strlen(src), dst, sz, flags, esc); > } > > -static inline int string_escape_str_any_np(const char *src, char **dst, > +static inline int string_escape_str_any_np(const char *src, char *dst, > size_t sz, const char *esc) > { > return string_escape_str(src, dst, sz, ESCAPE_ANY_NP, esc); > diff --git a/lib/string_helpers.c b/lib/string_helpers.c > index 9c48ddad0f0d..1826c7407258 100644 > --- a/lib/string_helpers.c > +++ b/lib/string_helpers.c > @@ -274,11 +274,6 @@ static bool escape_space(unsigned char c, char **dst, char *end) > return false; > } > > - if (out + 2 > end) { > - *dst = out + 2; > - return true; > - } > - > if (out < end) > *out = '\\'; > ++out; > @@ -309,11 +304,6 @@ static bool escape_special(unsigned char c, char **dst, char *end) > return false; > } > > - if (out + 2 > end) { > - *dst = out + 2; > - return true; > - } > - > if (out < end) > *out = '\\'; > ++out; > @@ -332,11 +322,6 @@ static bool escape_null(unsigned char c, char **dst, char *end) > if (c) > return false; > > - if (out + 2 > end) { > - *dst = out + 2; > - return true; > - } > - > if (out < end) > *out = '\\'; > ++out; > @@ -352,11 +337,6 @@ static bool escape_octal(unsigned char c, char **dst, char *end) > { > char *out = *dst; > > - if (out + 4 > end) { > - *dst = out + 4; > - return true; > - } > - > if (out < end) > *out = '\\'; > ++out; > @@ -378,11 +358,6 @@ static bool escape_hex(unsigned char c, char **dst, char *end) > { > char *out = *dst; > > - if (out + 4 > end) { > - *dst = out + 4; > - return true; > - } > - > if (out < end) > *out = '\\'; > ++out; > @@ -449,20 +424,17 @@ static bool escape_hex(unsigned char c, char **dst, char *end) > * it if needs. > * > * Return: > - * The amount of the characters processed to the destination buffer, or > - * %-ENOMEM if the size of buffer is not enough to put an escaped character is > - * returned. > - * > - * Even in the case of error @dst pointer will be updated to point to the byte > - * after the last processed character. > + * The total size of the escaped output that would be generated for > + * the given input and flags. To check whether the output was > + * truncated, compare the return value to osz. There is room left in > + * dst for a '\0' terminator if and only if ret < osz. > */ > -int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, > +int string_escape_mem(const char *src, size_t isz, char *dst, size_t osz, > unsigned int flags, const char *esc) > { > - char *p = *dst; > + char *p = dst; > char *end = p + osz; > bool is_dict = esc && *esc; > - int ret; > > while (isz--) { > unsigned char c = *src++; > @@ -502,13 +474,6 @@ int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, > escape_passthrough(c, &p, end); > } > > - if (p > end) { > - *dst = end; > - return -ENOMEM; > - } > - > - ret = p - *dst; > - *dst = p; > - return ret; > + return p - dst; > } > EXPORT_SYMBOL(string_escape_mem); > diff --git a/lib/test-string_helpers.c b/lib/test-string_helpers.c > index ab0d30e1e18f..8e376efd88a4 100644 > --- a/lib/test-string_helpers.c > +++ b/lib/test-string_helpers.c > @@ -260,16 +260,28 @@ static __init const char *test_string_find_match(const struct test_string_2 *s2, > return NULL; > } > > +static __init void > +test_string_escape_overflow(const char *in, int p, unsigned int flags, const char *esc, > + int q_test, const char *name) Just a nitpick. Can we reformat this to have return type and name on one line? > +{ > + int q_real; > + > + q_real = string_escape_mem(in, p, NULL, 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); > +} > + > static __init void test_string_escape(const char *name, > const struct test_string_2 *s2, > unsigned int flags, const char *esc) > { > - int q_real = 512; > - char *out_test = kmalloc(q_real, GFP_KERNEL); > - char *out_real = kmalloc(q_real, GFP_KERNEL); > + size_t out_size = 512; > + char *out_test = kmalloc(out_size, GFP_KERNEL); > + char *out_real = kmalloc(out_size, GFP_KERNEL); > char *in = kmalloc(256, GFP_KERNEL); > - char *buf = out_real; > int p = 0, q_test = 0; > + int q_real; > > if (!out_test || !out_real || !in) > goto out; > @@ -301,29 +313,19 @@ static __init void test_string_escape(const char *name, > q_test += len; > } > > - q_real = string_escape_mem(in, p, &buf, q_real, flags, esc); > + q_real = string_escape_mem(in, p, out_real, out_size, flags, esc); > > test_string_check_buf(name, flags, in, p, out_real, q_real, out_test, > q_test); > + > + test_string_escape_overflow(in, p, flags, esc, q_test, name); > + > out: > kfree(in); > kfree(out_real); > kfree(out_test); > } > > -static __init void test_string_escape_nomem(void) > -{ > - char *in = "\eb \\C\007\"\x90\r]"; > - char out[64], *buf = out; > - int rc = -ENOMEM, ret; > - > - ret = string_escape_str_any_np(in, &buf, strlen(in), NULL); > - if (ret == rc) > - return; > - > - pr_err("Test 'escape nomem' failed: got %d instead of %d\n", ret, rc); > -} > - > static int __init test_string_helpers_init(void) > { > unsigned int i; > @@ -342,8 +344,6 @@ static int __init test_string_helpers_init(void) > for (i = 0; i < (ESCAPE_ANY_NP | ESCAPE_HEX) + 1; i++) > test_string_escape("escape 1", escape1, i, TEST_STRING_2_DICT_1); > > - test_string_escape_nomem(); > - > return -EINVAL; > } > module_init(test_string_helpers_init); > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > index a1b6487c6710..9651686f3b42 100644 > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -1241,8 +1241,12 @@ char *escaped_string(char *buf, char *end, u8 *addr, struct printf_spec spec, > > len = spec.field_width < 0 ? 1 : spec.field_width; > > - /* Ignore the error. We print as many characters as we can */ > - string_escape_mem(addr, len, &buf, end - buf, flags, NULL); > + /* > + * string_escape_mem() writes as many characters as it can to > + * the given buffer, and returns the total size of the output > + * had the buffer been big enough. > + */ > + buf += string_escape_mem(addr, len, buf, buf < end ? end - buf : 0, flags, NULL); > > return buf; > } > diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c > index 33fb105d4352..22c4418057f4 100644 > --- a/net/sunrpc/cache.c > +++ b/net/sunrpc/cache.c > @@ -1068,12 +1068,14 @@ void qword_add(char **bpp, int *lp, char *str) > { > char *bp = *bpp; > int len = *lp; > - int ret; > + int ret, written; > > if (len < 0) return; > > - ret = string_escape_str(str, &bp, len, ESCAPE_OCTAL, "\\ \n\t"); > - if (ret < 0 || ret == len) > + ret = string_escape_str(str, bp, len, ESCAPE_OCTAL, "\\ \n\t"); > + written = min(ret, len); > + bp += written; Do we need extra variable? > + if (ret >= len) Something already done in the min. > len = -1; > else { > len -= ret; Would we simplify this? ret = string_escape_str(str, bp, len, ESCAPE_OCTAL, "\\ \n\t"); if (ret >= len) { bp += len; len = -1; } else { bp += ret; … -- Andy Shevchenko <andriy.shevchenko@intel.com> Intel Finland Oy ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v4 3/3] lib/string_helpers.c: Change semantics of string_escape_mem @ 2015-03-04 11:49 ` Andy Shevchenko 0 siblings, 0 replies; 52+ messages in thread From: Andy Shevchenko @ 2015-03-04 11:49 UTC (permalink / raw) To: Rasmus Villemoes Cc: Andrew Morton, J. Bruce Fields, Trond Myklebust, Anna Schumaker, David S. Miller, linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-nfs-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA On Wed, 2015-03-04 at 00:20 +0100, Rasmus Villemoes wrote: > The current semantics of string_escape_mem are inadequate for one of > its current users, vsnprintf(). If that is to honour its contract, it > must know how much space would be needed for the entire escaped > buffer, and string_escape_mem provides no way of obtaining that (short > of allocating a large enough buffer (~4 times input string) to let it > play with, and that's definitely a big no-no inside vsnprintf). > > So change the semantics for string_escape_mem to be more > snprintf-like: Return the size of the output that would be generated > if the destination buffer was big enough, but of course still only > write to the part of dst it is allowed to, and (contrary to snprintf) > don't do '\0'-termination. It is then up to the caller to detect > whether output was truncated and to append a '\0' if desired. Also, we > must output partial escape sequences, otherwise a call such as > snprintf(buf, 3, "%1pE", "\123") would cause printf to write a \0 to > buf[2] but leaving buf[0] and buf[1] with whatever they previously > contained. > > This also fixes a bug in the escaped_string() helper function, which > used to unconditionally pass a length of "end-buf" to > string_escape_mem(); since the latter doesn't check osz for being > insanely large, it would happily write to dst. For example, > kasprintf(GFP_KERNEL, "something and then %pE", ...); is an easy way > to trigger an oops. > > In test-string_helpers.c, the -ENOMEM test is replaced with testing > for getting the expected return value even if the buffer is too > small. We also ensure that nothing is written (by relying on a NULL > pointer deref) if the output size is 0 by passing NULL - this has to > work for kasprintf("%pE") to work. > > In net/sunrpc/cache.c, I think qword_add still has the same > semantics. Someone should definitely double-check this. > > In fs/proc/array.c, I made the minimum possible change, but > longer-term it should stop poking around in seq_file internals. > > Signed-off-by: Rasmus Villemoes <linux-qQsb+v5E8BnlAoU/VqSP6n9LOBIZ5rWg@public.gmane.org> Thanks! One nitpick below, but anyway Acked-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> for everything except net/sunrpc/cache.c > --- > fs/proc/array.c | 4 ++-- > include/linux/string_helpers.h | 8 +++---- > lib/string_helpers.c | 49 ++++++------------------------------------ > lib/test-string_helpers.c | 40 +++++++++++++++++----------------- > lib/vsprintf.c | 8 +++++-- > net/sunrpc/cache.c | 8 ++++--- > 6 files changed, 44 insertions(+), 73 deletions(-) > > diff --git a/fs/proc/array.c b/fs/proc/array.c > index 1295a00ca316..b5405889a780 100644 > --- a/fs/proc/array.c > +++ b/fs/proc/array.c > @@ -99,8 +99,8 @@ 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\\"); > + buf += string_escape_str(tcomm, buf, m->size - m->count, > + ESCAPE_SPACE | ESCAPE_SPECIAL, "\n\\"); > > m->count = buf - m->buf; > seq_putc(m, '\n'); > diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h > index 657571817260..0991913f4953 100644 > --- a/include/linux/string_helpers.h > +++ b/include/linux/string_helpers.h > @@ -47,22 +47,22 @@ static inline int string_unescape_any_inplace(char *buf) > #define ESCAPE_ANY_NP (ESCAPE_ANY | ESCAPE_NP) > #define ESCAPE_HEX 0x20 > > -int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, > +int string_escape_mem(const char *src, size_t isz, char *dst, size_t osz, > unsigned int flags, const char *esc); > > static inline int string_escape_mem_any_np(const char *src, size_t isz, > - char **dst, size_t osz, const char *esc) > + char *dst, size_t osz, const char *esc) > { > return string_escape_mem(src, isz, dst, osz, ESCAPE_ANY_NP, esc); > } > > -static inline int string_escape_str(const char *src, char **dst, size_t sz, > +static inline int string_escape_str(const char *src, char *dst, size_t sz, > unsigned int flags, const char *esc) > { > return string_escape_mem(src, strlen(src), dst, sz, flags, esc); > } > > -static inline int string_escape_str_any_np(const char *src, char **dst, > +static inline int string_escape_str_any_np(const char *src, char *dst, > size_t sz, const char *esc) > { > return string_escape_str(src, dst, sz, ESCAPE_ANY_NP, esc); > diff --git a/lib/string_helpers.c b/lib/string_helpers.c > index 9c48ddad0f0d..1826c7407258 100644 > --- a/lib/string_helpers.c > +++ b/lib/string_helpers.c > @@ -274,11 +274,6 @@ static bool escape_space(unsigned char c, char **dst, char *end) > return false; > } > > - if (out + 2 > end) { > - *dst = out + 2; > - return true; > - } > - > if (out < end) > *out = '\\'; > ++out; > @@ -309,11 +304,6 @@ static bool escape_special(unsigned char c, char **dst, char *end) > return false; > } > > - if (out + 2 > end) { > - *dst = out + 2; > - return true; > - } > - > if (out < end) > *out = '\\'; > ++out; > @@ -332,11 +322,6 @@ static bool escape_null(unsigned char c, char **dst, char *end) > if (c) > return false; > > - if (out + 2 > end) { > - *dst = out + 2; > - return true; > - } > - > if (out < end) > *out = '\\'; > ++out; > @@ -352,11 +337,6 @@ static bool escape_octal(unsigned char c, char **dst, char *end) > { > char *out = *dst; > > - if (out + 4 > end) { > - *dst = out + 4; > - return true; > - } > - > if (out < end) > *out = '\\'; > ++out; > @@ -378,11 +358,6 @@ static bool escape_hex(unsigned char c, char **dst, char *end) > { > char *out = *dst; > > - if (out + 4 > end) { > - *dst = out + 4; > - return true; > - } > - > if (out < end) > *out = '\\'; > ++out; > @@ -449,20 +424,17 @@ static bool escape_hex(unsigned char c, char **dst, char *end) > * it if needs. > * > * Return: > - * The amount of the characters processed to the destination buffer, or > - * %-ENOMEM if the size of buffer is not enough to put an escaped character is > - * returned. > - * > - * Even in the case of error @dst pointer will be updated to point to the byte > - * after the last processed character. > + * The total size of the escaped output that would be generated for > + * the given input and flags. To check whether the output was > + * truncated, compare the return value to osz. There is room left in > + * dst for a '\0' terminator if and only if ret < osz. > */ > -int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, > +int string_escape_mem(const char *src, size_t isz, char *dst, size_t osz, > unsigned int flags, const char *esc) > { > - char *p = *dst; > + char *p = dst; > char *end = p + osz; > bool is_dict = esc && *esc; > - int ret; > > while (isz--) { > unsigned char c = *src++; > @@ -502,13 +474,6 @@ int string_escape_mem(const char *src, size_t isz, char **dst, size_t osz, > escape_passthrough(c, &p, end); > } > > - if (p > end) { > - *dst = end; > - return -ENOMEM; > - } > - > - ret = p - *dst; > - *dst = p; > - return ret; > + return p - dst; > } > EXPORT_SYMBOL(string_escape_mem); > diff --git a/lib/test-string_helpers.c b/lib/test-string_helpers.c > index ab0d30e1e18f..8e376efd88a4 100644 > --- a/lib/test-string_helpers.c > +++ b/lib/test-string_helpers.c > @@ -260,16 +260,28 @@ static __init const char *test_string_find_match(const struct test_string_2 *s2, > return NULL; > } > > +static __init void > +test_string_escape_overflow(const char *in, int p, unsigned int flags, const char *esc, > + int q_test, const char *name) Just a nitpick. Can we reformat this to have return type and name on one line? > +{ > + int q_real; > + > + q_real = string_escape_mem(in, p, NULL, 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); > +} > + > static __init void test_string_escape(const char *name, > const struct test_string_2 *s2, > unsigned int flags, const char *esc) > { > - int q_real = 512; > - char *out_test = kmalloc(q_real, GFP_KERNEL); > - char *out_real = kmalloc(q_real, GFP_KERNEL); > + size_t out_size = 512; > + char *out_test = kmalloc(out_size, GFP_KERNEL); > + char *out_real = kmalloc(out_size, GFP_KERNEL); > char *in = kmalloc(256, GFP_KERNEL); > - char *buf = out_real; > int p = 0, q_test = 0; > + int q_real; > > if (!out_test || !out_real || !in) > goto out; > @@ -301,29 +313,19 @@ static __init void test_string_escape(const char *name, > q_test += len; > } > > - q_real = string_escape_mem(in, p, &buf, q_real, flags, esc); > + q_real = string_escape_mem(in, p, out_real, out_size, flags, esc); > > test_string_check_buf(name, flags, in, p, out_real, q_real, out_test, > q_test); > + > + test_string_escape_overflow(in, p, flags, esc, q_test, name); > + > out: > kfree(in); > kfree(out_real); > kfree(out_test); > } > > -static __init void test_string_escape_nomem(void) > -{ > - char *in = "\eb \\C\007\"\x90\r]"; > - char out[64], *buf = out; > - int rc = -ENOMEM, ret; > - > - ret = string_escape_str_any_np(in, &buf, strlen(in), NULL); > - if (ret == rc) > - return; > - > - pr_err("Test 'escape nomem' failed: got %d instead of %d\n", ret, rc); > -} > - > static int __init test_string_helpers_init(void) > { > unsigned int i; > @@ -342,8 +344,6 @@ static int __init test_string_helpers_init(void) > for (i = 0; i < (ESCAPE_ANY_NP | ESCAPE_HEX) + 1; i++) > test_string_escape("escape 1", escape1, i, TEST_STRING_2_DICT_1); > > - test_string_escape_nomem(); > - > return -EINVAL; > } > module_init(test_string_helpers_init); > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > index a1b6487c6710..9651686f3b42 100644 > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -1241,8 +1241,12 @@ char *escaped_string(char *buf, char *end, u8 *addr, struct printf_spec spec, > > len = spec.field_width < 0 ? 1 : spec.field_width; > > - /* Ignore the error. We print as many characters as we can */ > - string_escape_mem(addr, len, &buf, end - buf, flags, NULL); > + /* > + * string_escape_mem() writes as many characters as it can to > + * the given buffer, and returns the total size of the output > + * had the buffer been big enough. > + */ > + buf += string_escape_mem(addr, len, buf, buf < end ? end - buf : 0, flags, NULL); > > return buf; > } > diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c > index 33fb105d4352..22c4418057f4 100644 > --- a/net/sunrpc/cache.c > +++ b/net/sunrpc/cache.c > @@ -1068,12 +1068,14 @@ void qword_add(char **bpp, int *lp, char *str) > { > char *bp = *bpp; > int len = *lp; > - int ret; > + int ret, written; > > if (len < 0) return; > > - ret = string_escape_str(str, &bp, len, ESCAPE_OCTAL, "\\ \n\t"); > - if (ret < 0 || ret == len) > + ret = string_escape_str(str, bp, len, ESCAPE_OCTAL, "\\ \n\t"); > + written = min(ret, len); > + bp += written; Do we need extra variable? > + if (ret >= len) Something already done in the min. > len = -1; > else { > len -= ret; Would we simplify this? ret = string_escape_str(str, bp, len, ESCAPE_OCTAL, "\\ \n\t"); if (ret >= len) { bp += len; len = -1; } else { bp += ret; … -- 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
end of thread, other threads:[~2015-03-04 11:49 UTC | newest] Thread overview: 52+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 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
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.