openssh unix dev - Apr 2016 - ssh-keygen -R is case-sensitive, but should not be

Here is a better patch. Somehow I pasted an older version of my edits:

-------------------------------------------------------
% diff ./match.c /home/millerig/osrc/openssh-7.2p2/match.c
121a122
> 	char *low_string = 0;
156,159c157,168
< 		if (match_pattern(string, sub)) {
< 			if (negated)
< 				return -1;		/* Negative */
< 			else
---
> 		if (dolower) {
> 			u_int j;
> 			if (low_string) free(low_string);
> 			low_string = malloc(strlen(string) + 1);
> 			for (j = 0; j < strlen(string); ++j) low_string[j] =
tolower(string[j]);
> 			low_string[j] = 0;
> 		}
> 		if (match_pattern((dolower ? low_string : string), sub)) {
> 			if (negated) {
> 				got_positive = -1;		/* Negative */
> 				break;
> 			} else
165,166c174,175
< 	 * Return success if got a positive match.  If there was a negative
< 	 * match, we have already returned -1 and never get here.
---
> 	 * Return success if there was a positive match;
> 	 * return -1 if there was a negative match.
167a177
> 	if (low_string) free(low_string);
-------------------------------------------------------

On 16/04/16 00:32, Griff Miller II wrote:
> Here is a better patch. Somehow I pasted an older version of my edits:
Hello Griff

Thanks for your patch. That will surely help the maintainers.
Here are a few comments from my part:
> -------------------------------------------------------
> % diff ./match.c /home/millerig/osrc/openssh-7.2p2/match.c
> 121a122
I'd have prefered an unified diff :)

>> 	char *low_string = 0;
I know that the C standard guarantees that it has the exact
same semantic, but please, if you are initializing a pointer, use NULL.

(PS: there's exactly an instance of that in openssh code at 
server_input_hostkeys_prove, that should be changed, too)

There's a memory leak on line 147 when a subpattern is too long.

> 156,159c157,168
> <  		if (match_pattern(string, sub)) {
> <  			if (negated)
> <  				return -1;		/* Negative */
> <  			else
> ---
>> 		if (dolower) {
By performing the lowercase here, you are doing a free() + malloc() + 
tolower() copying of the string per subpattern.
Just move the lowercasing code before the for.
>> 			u_int j;
>> 			if (low_string) free(low_string);
Which then makes this innecessary.
>> 			low_string = malloc(strlen(string) + 1);
Use xmalloc() here
>> 			for (j = 0; j<  strlen(string); ++j) low_string[j] =
tolower(string[j]);
I'd recommend breaking this in two lines.

The compiler will probably figure out that strlen() can be called only 
once, instead of creating O(n?) code,
but this is equivalent to ?for (j = 0; string[j]; ??
>> 			low_string[j] = 0;
Similar to the above NULL issue: low_string is a char*, so -although 
equivalent- it's generally better to use '\0'.
>> 		}
>> 		if (match_pattern((dolower ? low_string : string), sub)) {
>> 			if (negated) {
>> 				got_positive = -1;		/* Negative */
>> 				break;
>> 			} else
> 165,166c174,175
> <  	* Return success if got a positive match.  If there was a negative
> <  	* match, we have already returned -1 and never get here.
> ---
>> 	* Return success if there was a positive match;
>> 	 * return -1 if there was a negative match.
> 167a177
>> 	if (low_string) free(low_string);
Useless if

Best regards

?ngel, thanks for the comments. I'm really annoyed with myself about that
memory leak. :)  I guess that will teach me not to break one of my own
rules, i.e. always set the code aside for a while and take a second look
later.

I like your technique for terminating the string copy loop, i.e. testing
on string[i] instead of i < strlen(string). I got rid of the j
declaration, BTW.

Of course it makes more sense to move the string copy outside the loop.
I'm annoyed with myself about that, too.

I knew that free() is supposed to do nothing if you pass it a null
pointer, but it's an old habit from the days of sketchy C compilers.  :)

New diff attached.

On Fri, April 15, 2016 6:28 pm, ?ngel Gonz?lez wrote:
> On 16/04/16 00:32, Griff Miller II wrote:
>
>> Here is a better patch. Somehow I pasted an older version of my edits:
>>
>
> Hello Griff
>
>
> Thanks for your patch. That will surely help the maintainers.
> Here are a few comments from my part:
>
>
>> -------------------------------------------------------
>> % diff ./match.c /home/millerig/osrc/openssh-7.2p2/match.c
>> 121a122
>>
> I'd have prefered an unified diff :)
>
>
>
>>> char *low_string = 0;
> I know that the C standard guarantees that it has the exact
> same semantic, but please, if you are initializing a pointer, use NULL.
>
> (PS: there's exactly an instance of that in openssh code at
> server_input_hostkeys_prove, that should be changed, too)
>
> There's a memory leak on line 147 when a subpattern is too long.
>
>
>
>> 156,159c157,168
>> <  		if (match_pattern(string, sub)) {
>> <  			if (negated)
>> <  				return -1;		/* Negative */
>> <  			else
>> ---
>>
>>> if (dolower) {
> By performing the lowercase here, you are doing a free() + malloc() +
> tolower() copying of the string per subpattern. Just move the lowercasing
> code before the for.
>
>>> u_int j; if (low_string) free(low_string);
> Which then makes this innecessary.
>
>
>>> low_string = malloc(strlen(string) + 1);
> Use xmalloc() here
>
>
>>> for (j = 0; j<  strlen(string); ++j) low_string[j] >>>
tolower(string[j]);
> I'd recommend breaking this in two lines.
>
>
> The compiler will probably figure out that strlen() can be called only
> once, instead of creating O(n?) code, but this is equivalent to ?for (j
> 0; string[j];
?
>
>
>>> low_string[j] = 0;
> Similar to the above NULL issue: low_string is a char*, so -although
> equivalent- it's generally better to use '\0'.
>
>>> }
>>> if (match_pattern((dolower ? low_string : string), sub)) { if
(negated)
>>> {
>>> got_positive = -1;		/* Negative */ break; } else
>>>
>> 165,166c174,175
>> <  	* Return success if got a positive match.  If there was a
negative
>> <  	* match, we have already returned -1 and never get here.
>> ---
>>
>>> * Return success if there was a positive match;
>>> * return -1 if there was a negative match.
>>>
>> 167a177
>>
>>> if (low_string) free(low_string);
> Useless if
>
>
> Best regards
>
>
>