[DISCOVERY PHP-CPG] Different CPG and discovery rule not working anymore on pattern 76

[compaluca]

• Many PHP discovery rules that were working fine on 01/02/2023 are not working anymore.
• Consider for instance pattern instance 1_instance_76_function_variable
• as far as I can see nothing changed for that pattern instance since 01/02/2023
• however some changes were done on
  • (CH1) PHP-CPG (newer version)
  • (CH2) joern (newer version)
  • (CH3) patch for the discovery rule (cf. [DISCOVERY] Implement automated work-around for missing line number from PHP-CPG / Joern  #42)
• I believe the problem is on (CH1)
• the CPG files generated for the pattern instance are different (cf. bin files in the uploaded archive):
  • cpg_2023-02-01-16-52-44_PHP_1_instance_76_function_variable.bin
  • cpg_2023-03-23-15-35-16_PHP_1_instance_76_function_variable.bin
    CPGs_1_instance_76_function_variable.zip
• By running joern in the console:
  • the discovery rule works fine when running on CPG cpg_2023-02-01-16-52-44_PHP_1_instance_76_function_variable.bin (also the patched one)
  • it does not work on the newer CPG

@mal-tee : can you please have a look? Let me know if more detail are needed

[simkoc]

A quick view into your .zip it seems the 2023 03 23 has an reported error:

,,,,/tp-framework/testability_patterns/PHP/76_function_variable/1_instance_76_function_variable/1_instance_76_function_variable.sc,ERROR_DISCOVERY. module 'config' has no attribute 'PATCHED_PREFIX'

I am not sure that this is an issue with CH1 as I am not aware of any PATCHED_PREFIX config being used in the CPG.

@mal-tee ?

[mal-tee]

this looks like a python error, probably a bug we missed in #48?

[compaluca]

Sorry for the missup in that csv result. I think that not all the code fixing #48 was used in the docker container. I re-build the docker container from scratch and rerun. Please consider this new zip archive:
CPGs_1_instance_76_function_variable-NEW.zip

However the problem remain (indeed the CPG generated are identical to those in the original zip archive of this issue). Notice that the test I did included using joern and PHP-CPG on their own without the tpframework. This is why I was pointing to (CH1).

Can you please re-check?

[mal-tee]

the node in question in the recent cpg:

joern> cpg.call("INIT_DYNAMIC_CALL").l 
res2: List[Call] = List(
  Call(
    id -> 512409557603043120L,
    argumentIndex -> -1,
    argumentName -> None,
    code -> "INIT_DYNAMIC_CALL 1 CV($func)",
    columnNumber -> None,
    dispatchType -> "STATIC_DISPATCH",
    dynamicTypeHintFullName -> ArraySeq(),
    lineNumber -> Some(value = 9),
    methodFullName -> "<empty>",
    name -> "INIT_DYNAMIC_CALL",
    order -> 4,
    signature -> "",
    typeFullName -> "<empty>"
  )
)

the query:

cpg.call("INIT_DYNAMIC_CALL").code(".*false.*").location.toJson

See my commit comment from the review:

I am not sure if the discovery rules makes sense. I don't know
where the false comes from. Wouldn't it be smarter to check for sth. like
.argument.code("CV")?

I still don't know what the false means in this rule. What was the intention here? Even in the old opcode dump there is no false there. https://github.com/testable-eu/sast-testability-patterns/blob/7c2f8595c1c33c95e1a0ba755719d48d6144f5ae/PHP/76_function_variable/1_instance_76_function_variable/1_instance_76_function_variable.bash#L12

Maybe this made sense in the old custom fork of the php-cpg generator? This is the reason why it doesn't match anymore.

---

I've checked the old cpg, the node there dumps internal PHP data (name: String, isRef: Bool, isTmp:Bool)about the CV.

code -> "INIT_DYNAMIC_CALL 1 Variable(func,false,false)",

So effectively the rule requires the variable to not be a reference or not a temp var. Why? This doesn't make a lot of sense to me.
In the current php-cpg we don't expose this internal information. Why did your custom version do this?

To fix this, I propose to change the discovery rule like I proposed in the commit message. Otherwise, if there is a reason to check for false, we could see how to model this in the recent version php-cpg.

[compaluca]

I am afraid that commit comment went lost in space. Are there others? Maybe it is better if we create specific issues for those doubts, just to keep more visible trace?

Concerning the specific example on pattern 76, I created a pulled request testable-eu/sast-testability-patterns#26

I will check about the other patterns for which the discovery result is not different and report that back here

[mal-tee]

Are there others?

I don't think so. Sorry for that. :D