[PATCH v1] seq_file: convert mangle_path() to use string_escape_str()

[PATCH v1] seq_file: convert mangle_path() to use string_escape_str()

[Andy Shevchenko]

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(-)

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);
-- 
2.20.1

Re: [PATCH v1] seq_file: convert mangle_path() to use string_escape_str()

[Andrew Morton]

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.

Re: [PATCH v1] seq_file: convert mangle_path() to use string_escape_str()

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko