Á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
>
>
>
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@xxxxxxxxxxx
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
