Bug report from static analysis

[alexandr-u]

Description

Hello.

I love that you shared your projects with the world. Thank you for that!

I checked PHP using the PVS-Studio static analyzer and published an article: "What if your elephant thinks it is bug?".

Perhaps you would like to take a look.

PHP Version

I checked the PHP project at the `c998c36` commit.

Operating System

Ubuntu 24.04

[iluuu1994]

Thanks for bringing this to our attention. Unfortunately, most of your reports are false positives or benign. This would be fine, except that the blog post is taking shots at PHP. Given the poor quality of the report, this is disappointing.

... an elephant with the habits of a bug
... and developers don't stop to invent new ways to shoot themselves in the foot.
... Let's get back to our elephants in a bug's clothing.
... Dozen mind-bending bugs

Fragment N1

Note that this only affects php-cgi, which isn't very widely used anymore. Nonetheless, we'll see whether this can be improved.

Fragment N2

This is valid, though note that:

This fragment contains over a thousand characters in a single line. There's no way to spot the problem like that, so we've split it up for the article. By the way, we can understand what the problem with code sausage is in a short article.

You're looking at a generated file. See ext/phar/makestub.php.

Fragment N3

You're missing that previously the same if (extends_ast) { condition was used to assign to ce->parent_name. So, while checking ce->parent_name is valid, so is extends_ast.

Fragment N4, N5, N6

Technically true, but nowadays operating systems will very rarely ever return NULL from system allocators. It's much more likely that the process will be killed if it consumes too much memory. Nonetheless, it may be worth handling this more gracefully.

Fragment N7

This is only an issue if op_array->num_dynamic_func_defs < opline->op2.num may be true, which it can't.

Fragment N8

I don't disagree that using 1ull is a good idea to make the integer semantics more obviously correct. But the post implies that the current code doesn't work as intended. However, both versions behave equivalently.

Fragment N9

This one is legit. For timezones with timezone offsets that are not whole hours, this will implicitly be floored to hours, which is incorrect.

Fragment N10

Duplicate code does not necessarily indicate a bug. In this case, it's the opposite. An assumption was made that was later fixed, and a comment was added on why that assumption was wrong. Both branches happen to now be the same, but the comment applies only to one, which is presumably why they weren't unified.

Fragment N11

It should be immediately obvious that this code after zend_error_noreturn() is harmless and there for consistency.

Fragment N12

minilua.c is not part of the PHP project. It's only there to compile the ext/opcache/jit/ir/ir_*.dasc files, and is only used at compile-time.

Given the size of php-src, the number of real issues detected PVS-Studio seems negligible.