Re: com php-src: avoid repeated strlen() usage: ext/zip/php_zip.c
| From: | Nikita Popov | Date: | Sun, 14 Sep 2014 12:34:52 +0000 |
| Subject: | Re: com php-src: avoid repeated strlen() usage: ext/zip/php_zip.c | ||
| References: | 1 | Groups: | php.cvs |
| Request: | Send a blank email to php-cvs+get-81595@lists.php.net to get a copy of this message | ||
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=b6b376efe538997369efc3dfffede295f433105dH@
> F+.Dʔ
>
> 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';
}
}
Nikita