linux-kernel - Re: [PATCH v2 2/3] lib/string_helpers.c: Refactor string_escape

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87sietbulp.fsf@rasmusvillemoes.dk>
Date:	Thu, 29 Jan 2015 14:10:42 +0100
From:	Rasmus Villemoes <linux@...musvillemoes.dk>
To:	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Mathias Krause <minipli@...glemail.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/3] lib/string_helpers.c: Refactor string_escape_mem

On Thu, Jan 29 2015, Andy Shevchenko <andriy.shevchenko@...ux.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/