Re: com php-src: avoid repeated strlen() usage: ext/zip/php_zip.c

On Sun, September 14, 2014 14:34, Nikita Popov wrote: > On Sun, Sep 14, 2014 at 12:59 PM, Anatol Belski <ab@php.net> wrote: > > >> Commit: b6b376efe538997369efc3dfffede295f433105d >> Author: Anatol Belski <ab@php.net> Sun, 14 Sep 2014 12:59:31 >> +0200 >> Parents: f1ba5389cd0e13da2230dd411c6151dedde88552 >> Branches: master >> >> >> Link: >> >> http://git.php.net/?p=php-src.git;a=commitdiff;h=b6b376efe538997369efc3d >> fffede295f433105d >> >> Log: >> avoid repeated strlen() usage >> >> Changed paths: >> M ext/zip/php_zip.c >> >> >> >> Diff: >> diff --git a/ext/zip/php_zip.c b/ext/zip/php_zip.c index 5f98a63..0f6a6bf >> 100644 >> --- a/ext/zip/php_zip.c >> +++ b/ext/zip/php_zip.c >> @@ -1609,9 +1609,11 @@ static void >> php_zip_add_from_pattern(INTERNAL_FUNCTION_PARAMETERS, int type) /* >> RETURN_FALSE; >> } >> >> >> - if (remove_path && remove_path_len > 1 && >> (remove_path[strlen(remove_path) - 1] == '/' || >> - remove_path[strlen(remove_path) - 1] == '\\')) { >> - remove_path[strlen(remove_path) - 1] = '\0'; >> + if (remove_path && remove_path_len > 1) { >> + int tmp_idx = (int)strlen(remove_path) - 1; >> + if (remove_path[tmp_idx] == '/' || remove_path[tmp_idx] >> == >> '\\') { >> + remove_path[tmp_idx] = '\0'; >> + } >> } >> >> >> if (type == 1) { >> > > Not really related to the change, but: If remove_path = "\0\0", then > remove_path_len = 2 > 1, but strlen(remove_path) = 0, so tmp_idx = -1, > leading to out of bounds access. Can that happen? I think it would be > safer to write this as follows: > > if (remove_path) { size_t real_len = strlen(remove_path); if (real_len > 1 > && (remove_path[real_len - 1] == '/' || > remove_path[real_len - 1] == '\\')) { remove_path[real_len - 1] = '\0'; } > } > Yeah, depends on how remove_path_len was computed, but safer as you say anyway. Thanks Anatol