| Message ID | 20190109154022.23551-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] seq_file: convert mangle_path() to use string_escape_str() | expand |
On Wed, 9 Jan 2019 17:40:22 +0200 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > Since string_escape_str() does not support overlapping buffer first we check if > there is enough room in the buffer and then update a path. The side effect of > this change is in case of failure the buffer is left unchanged. > > ... > > --- a/fs/seq_file.c > +++ b/fs/seq_file.c > @@ -421,21 +421,13 @@ EXPORT_SYMBOL(seq_printf); > */ > char *mangle_path(char *s, const char *p, const char *esc) > { > - while (s <= p) { > - char c = *p++; > - if (!c) { > - return s; > - } else if (!strchr(esc, c)) { > - *s++ = c; > - } else if (s + 4 > p) { > - break; > - } else { > - *s++ = '\\'; > - *s++ = '0' + ((c & 0300) >> 6); > - *s++ = '0' + ((c & 070) >> 3); > - *s++ = '0' + (c & 07); > - } > - } > + size_t len = p + strlen(p) - s; > + int ret; > + > + ret = string_escape_str(p, NULL, 0, ESCAPE_OCTAL, esc); > + if (ret < len) > + return s + string_escape_str(p, s, len, ESCAPE_OCTAL, esc); > + > return NULL; > } > EXPORT_SYMBOL(mangle_path); Confusing. I think the objective of the patch is to use an existing library function rather than open-coding, but the library function doesn't support in-place operation on the string. So the old mangle_path() was OK with in-place conversion, but the new mangle_path() is not. Is that correct? Do we know that all existing mangle_path() callers are OK with this? Please make all this clear in the changelog. Also, the identifier `ret' is widely understood to mean "the value which this function will return", but that is not the case here. Please use a more appropriate identifier.
On Wed, Jan 9, 2019 at 8:21 PM Andrew Morton <akpm@linux-foundation.org> wrote: > On Wed, 9 Jan 2019 17:40:22 +0200 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > Since string_escape_str() does not support overlapping buffer first we check if > > there is enough room in the buffer and then update a path. The side effect of > > this change is in case of failure the buffer is left unchanged. > > char *mangle_path(char *s, const char *p, const char *esc) > > { > > + size_t len = p + strlen(p) - s; > > + int ret; > > + > > + ret = string_escape_str(p, NULL, 0, ESCAPE_OCTAL, esc); > > + if (ret < len) > > + return s + string_escape_str(p, s, len, ESCAPE_OCTAL, esc); > > + > > return NULL; > > } > Confusing. > > I think the objective of the patch is to use an existing library > function rather than open-coding, but the library function doesn't > support in-place operation on the string. So the old mangle_path() was > OK with in-place conversion, but the new mangle_path() is not. Is that > correct? As long as source buffer wouldn't be changed during conversion. Here the same technique is used as in kasprintf(), i.e. first iteration we would like to check if the buffer has enough size for the output, on second do actual conversion. Probably I need to add some test cases. > Do we know that all existing mangle_path() callers are OK > with this? Please make all this clear in the changelog. > > Also, the identifier `ret' is widely understood to mean "the value > which this function will return", but that is not the case here. > Please use a more appropriate identifier. The meaning of it is a destination length. I will rename it.
diff --git a/fs/seq_file.c b/fs/seq_file.c index 1dea7a8a5255..b818b23070e6 100644 --- a/fs/seq_file.c +++ b/fs/seq_file.c @@ -421,21 +421,13 @@ EXPORT_SYMBOL(seq_printf); */ char *mangle_path(char *s, const char *p, const char *esc) { - while (s <= p) { - char c = *p++; - if (!c) { - return s; - } else if (!strchr(esc, c)) { - *s++ = c; - } else if (s + 4 > p) { - break; - } else { - *s++ = '\\'; - *s++ = '0' + ((c & 0300) >> 6); - *s++ = '0' + ((c & 070) >> 3); - *s++ = '0' + (c & 07); - } - } + size_t len = p + strlen(p) - s; + int ret; + + ret = string_escape_str(p, NULL, 0, ESCAPE_OCTAL, esc); + if (ret < len) + return s + string_escape_str(p, s, len, ESCAPE_OCTAL, esc); + return NULL; } EXPORT_SYMBOL(mangle_path);
Since string_escape_str() does not support overlapping buffer first we check if there is enough room in the buffer and then update a path. The side effect of this change is in case of failure the buffer is left unchanged. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- fs/seq_file.c | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-)