Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:114558
MIME-Version: 1.0
References: <194DA850-19F7-41C4-97CF-944F13B57AFD@cmpct.info>
In-Reply-To: <194DA850-19F7-41C4-97CF-944F13B57AFD@cmpct.info>
Reply-To: Levi Morrison <levi.morrison@datadoghq.com>
Date: Fri, 21 May 2021 13:48:01 -0600
Message-ID: <CAAKU0ukenZ2Uj2k01CT3cH+k4K=+dU+2uirE0DAqHxmdWjSSQQ@mail.gmail.com>
To: Calvin Buckley <calvin@cmpct.info>
Cc: PHP internals <internals@lists.php.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PHP-DEV] Using clang-analyzer with PHP: experiences?
From: internals@lists.php.net ("Levi Morrison via internals")

On Fri, May 21, 2021 at 1:01 PM Calvin Buckley <calvin@cmpct.info> wrote:
>
> Hi internals@,
>
> I maintain an extension and I suspect there are some issues in the code. =
As such, I=E2=80=99ve been trying various tools to try to make it easier to=
 catch the issues. (For the curious: I=E2=80=99ve tried *San, which I feel =
doesn=E2=80=99t work very well unless you /totally control/ the entire stac=
k, which I didn=E2=80=99t have the luxury of. I also tried Valgrind, but I =
need ro revisit this to deal with possible false positives in the library.)=
 This time, I decided to try static analysis through LLVM.
>
> Luckily, clang-analyzer is pretty simple. Just prepending =E2=80=9Cscan-b=
uild=E2=80=9D to my make invocation. Easy, right? Unfortunately, I noticed =
that due to an inconsistency in the codebase (a use of realloc instead of e=
realloc), that it doesn=E2=80=99t seem to account for i.e emalloc vs. mallo=
c. Possible leaks =E2=80=9Cwent away=E2=80=9D from the output when I conver=
ted them to the PHP memory management functions.
>
> Has anyone ever used clang-analyzer with PHP before? I noticed there was =
some tooling for a previous PHP transition [1], but I don=E2=80=99t know if=
 anyone=E2=80=99s tackled the low-hanging fruit of memory functions. I supp=
ose I could just redefine emalloc and friends, but I feel that would probab=
ly be inaccurate with things like zend_string.
>
> Regards,
> Calvin
>
> [1]: https://github.com/johannes/clang-php-checker
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: https://www.php.net/unsub.php
>

Just to check: are you setting the environment variable USE_ZEND_ALLOC
to 0? This causes the engine to use malloc:
https://heap.space/xref/PHP-7.4/Zend/zend_alloc.c?r=3D600402d9#2738.

For what it's worth, I was recently annoyed _again_ by valgrind being
so noisy because zend_string_equal_val intentionally reads past the
end of a zend_string. The allocator ensures that memory was allocated,
but it isn't guaranteed to be initialized. We should find some way to
initialize this memory for future releases -- maybe add a function
which null terminates a zend string by adding not 1 null byte but as
many as necessary to reach the end of the allocation. This should be
trivial enough in cost to do, compared to some other solutions like
always zero'ing out the whole memory block or initializing the
trailing bytes at zend_string_alloc time.

Also, I'm not sure this read-past-the-end technique is actually safe,
such as when USE_ZEND_ALLOC is set to zero and we use malloc directly,
which does not make the same guarantees about alignment and padding on
the string...

Nikita pushed up this change only today, but it would theoretically
help with valgrind being used a runtime but not compiled with valgrind
support: https://github.com/php/php-src/commit/a0c44fbaf19841164c7984a6c21b=
364d391f3750.
I say theoretically only because I haven't tested it yet.