Fix invalid casts int and float

[kkmuffme]

e.g. https://psalm.dev/r/0d284c6f48 gives Warning: Object of class stdClass could not be converted to int

Since by default there cannot be any "PossiblyInvalidCasts", as we always get 0/0.0 in error case, added mostly unwanted typecasts that are a leading cause of off-by-one errors as PossiblyInvalidCasts

Fix: #6966 (the remaining half + cherry-pick the other half from master to 4.x)

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/0d284c6f48

<?php

$foo = (object) null;

$bar = (int) $foo;
/** @psalm-trace $bar */;

echo $bar;

Psalm output (using commit 24f7920):

INFO: Trace - 6:25 - $bar: int

[kkmuffme]

Feedback appreciated :-)

Could someone please help with the failing taint tests?

[AndrolGenhald]

There's a lot going on here that I don't quite understand, could you add a description of the issue you're trying to solve?

Could someone please help with the failing taint tests?

If taint tests (or unused variable tests) are failing, you probably messed up $parent_nodes somewhere.

If you have this code:

$foobar = $_GET["foobar"];
if (is_string($foobar)) {
    echo $foobar; // XSS vulnerability!
}

Then what happens (at a high level) is that on line 1 $foobar is tracked with type mixed, then on line 2 a new context is created for the if, and in that context $foobar is set to have type string. For Psalm to track that $foobar is tainted (or to track if it's ever used when doing unused variable analysis), when the type is changed on line 2, a parent node is added to the new type pointing to the previous assignment (it's been a while since I've dealt with it so I might not have the specifics exactly right, but this is the general idea).

[kkmuffme]

There's a lot going on here that I don't quite understand, could you add a description of the issue you're trying to solve?

Summary: this PR adds correct support for typecasting to int/float.

• typecasting of objects/resources to int/float gives a PHP warning, but Psalm doesn't say anything about that. This PR adds InvalidCast error for this, https://psalm.dev/r/0d284c6f48

• typecasting arrays to int/float is always 0 or 1, not generic int https://psalm.dev/r/9e15ff2d1f

• due to this 0/1, typecasting of arrays often leads to off-by-one errors in code, that are extremely hard to track down in some cases. This is now added as "PossiblyInvalidCast" for int/float casts of arrays.

• typecasting (string) true is literal string '1', not generic string.

If taint tests (or unused variable tests) are failing, you probably messed up $parent_nodes somewhere.

Yes, thanks. That was it, the first commit that just unified the code messed it up. Reverted it now (will make it look nice later).
Now there are some other errors, e.g. for (array) casts, even though they weren't changed.
Some input on the failing tests would be great.

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/0d284c6f48

<?php

$foo = (object) null;

$bar = (int) $foo;
/** @psalm-trace $bar */;

echo $bar;

Psalm output (using commit 24f7920):

INFO: Trace - 6:25 - $bar: int

https://psalm.dev/r/9e15ff2d1f

<?php

$foo = array( 'hello' );
$data = (int) $foo;

/** @psalm-trace $data */;
echo $data;

Psalm output (using commit 24f7920):

INFO: Trace - 6:26 - $data: int

[AndrolGenhald]

Now there are some other errors, e.g. for (array) casts, even though they weren't changed.
Some input on the failing tests would be great.

I think the array one that's failing is the one I mentioned previously, looks like the rest of them are some taint tests that are still failing.

[AndrolGenhald]

This is now added as "PossiblyInvalidCast" for int/float casts of arrays.

@orklah What do you think about adding DangerousCast for this instead? I think PossiblyInvalidCast should be used when casting a variable whose type is a union where the cast might be valid or might be invalid (although I think there are probably some issues where we use a PossiblyInvalid... issue when it should just be Invalid... and vice versa).

[orklah]

Yeah, that makes sense, we should try to keep this rule consistent

[kkmuffme]

@AndrolGenhald added it as RiskyCast now

[kkmuffme]

This PR will also fix the pending half of #6966

[kkmuffme]

@orklah Ready to be reviewed & merged finally, I figured out the taint stuff.

Also cherry-picked that other PR, removed the code that is better handled with this PR and adjusted tests for 4.x

[kkmuffme]

@AndrolGenhald rebased and issues fixed, except "ComplexMethod" in these 2 functions as it doesn't make sense to rework this now, when this has to be modified again in #8486 and
#8366 (review)

I think fixing these 2 issues now, will just make it harder for the followup work.

[AndrolGenhald]

@AndrolGenhald rebased and issues fixed, except "ComplexMethod" in these 2 functions as it doesn't make sense to rework this now, when this has to be modified again in #8486 and #8366 (review)

I think fixing these 2 issues now, will just make it harder for the followup work.

Agreed, could you @psalm-suppress them though so they don't break other PRs once merged?

Also, it looks like there's now a regression in the bitwiseAssignment test.

[AndrolGenhald]

Out of curiosity, what was the issue with the broken bitwiseAssignment test?

[kkmuffme]

@AndrolGenhald when I used TIntRange(0, 1) psalm reduced this to "int" and reported "unused psalm suppress". So I changed it to literals:

// do NOT use TIntRange here, as it will cause invalid behavior, e.g. bitwiseAssignment
$valid_ints[] = new TLiteralInt(0);
$valid_ints[] = new TLiteralInt(1);

[AndrolGenhald]

Huh, weird. Guess there's other bugs around causing that then. @orklah everything look good to you?

[orklah]

Code seems good, could you add some tests for RiskyCast please?

[orklah]

Thanks!