ssh-keygen -R is case-sensitive, but should not be

[Griff Miller II]

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

-------------------------------------------------------

[Ángel González]

Hello Griff

Thanks for your patch. That will surely help the maintainers.

Post by Griff Miller II
-------------------------------------------------------
% diff ./match.c /home/millerig/osrc/openssh-7.2p2/match.c
121a122

I'd have prefered an unified diff :)

Post by Griff Miller II

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.

Post by Griff Miller II
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.

Post by Griff Miller II

u_int j;
if (low_string) free(low_string);

Which then makes this innecessary.

Post by Griff Miller II

low_string = malloc(strlen(string) + 1);

Use xmalloc() here

Post by Griff Miller II

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]; …»

Post by Griff Miller II

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'.

Post by Griff Miller II

}
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

[Griff Miller II]

I guess attachments get stripped out. I'll paste it:

----------------------------------------------------
--- match.c 2016-03-09 12:04:48.000000000 -0600
+++ /home/millerig/osrc/openssh-7.2p2/match.c 2016-04-15
22:50:08.917536100 -0500
@@ -119,10 +119,18 @@
match_pattern_list(const char *string, const char *pattern, int dolower)
{
char sub[1024];
+ char *low_string = NULL;
int negated;
int got_positive;
u_int i, subi, len = strlen(pattern);

+ if (dolower) {
+ low_string = xmalloc(strlen(string) + 1);
+ for (i = 0; string[i]; ++i)
+ low_string[i] = tolower(string[i]);
+ low_string[i] = '\0';
+ }
+
got_positive = 0;
for (i = 0; i < len;) {
/* Check if the subpattern is negated. */
@@ -142,8 +150,10 @@
sub[subi] = dolower && isupper((u_char)pattern[i]) ?
tolower((u_char)pattern[i]) : pattern[i];
/* If subpattern too long, return failure (no match). */
- if (subi >= sizeof(sub) - 1)
+ if (subi >= sizeof(sub) - 1) {
+ free(low_string);
return 0;
+ }

/* If the subpattern was terminated by a comma, skip the comma. */
if (i < len && pattern[i] == ',')
@@ -153,18 +163,20 @@
sub[subi] = '\0';

/* Try to match the subpattern against the string. */
- if (match_pattern(string, sub)) {
- if (negated)
- return -1; /* Negative */
- else
+ if (match_pattern((dolower ? low_string : string), sub)) {
+ if (negated) {
+ got_positive = -1; /* Negative */
+ break;
+ } else
got_positive = 1; /* Positive */
}
}

/*
- * 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.
*/
+ free(low_string);
return got_positive;
}

----------------------------------------------------

[Griff Miller II]

Á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.

Post by Ángel González
Hello Griff
Thanks for your patch. That will surely help the maintainers.

Post by Griff Miller II
-------------------------------------------------------
% diff ./match.c /home/millerig/osrc/openssh-7.2p2/match.c
121a122

I'd have prefered an unified diff :)

Post by Griff Miller II

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.

Post by Griff Miller II
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.

Post by Griff Miller II

u_int j; if (low_string) free(low_string);

Which then makes this innecessary.

Post by Griff Miller II

low_string = malloc(strlen(string) + 1);

Use xmalloc() here

Post by Griff Miller II

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]; …»

Post by Griff Miller II

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'.

Post by Griff Miller II

}
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