Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ruby] Constant propagation ignores side-effects on string object #3316

Closed
1 of 3 tasks
IagoAbal opened this issue Jun 9, 2021 · 4 comments · Fixed by #3894
Closed
1 of 3 tasks

[ruby] Constant propagation ignores side-effects on string object #3316

IagoAbal opened this issue Jun 9, 2021 · 4 comments · Fixed by #3894
Assignees
Labels
alpha Relates to an experimental feature feature:const-propagation

Comments

@IagoAbal
Copy link
Contributor

IagoAbal commented Jun 9, 2021

Describe the bug
Constant propagation incorrectly flags a variable as constant after it has been modified by a method call.

To Reproduce
https://semgrep.dev/s/4y19/

Expected behavior
No false positives.

What is the priority of the bug to you?

  • P0: blocking your adoption of Semgrep or workflow
  • P1: important to fix or quite annoying
  • P2: regular bug that should get fixed

Environment
If not using semgrep.dev: are you running off docker, an official binary, a local build?

@IagoAbal IagoAbal self-assigned this Jun 9, 2021
@IagoAbal
Copy link
Contributor Author

IagoAbal commented Jun 9, 2021

Some amount of false positives of this kind are expected since we only do intra-procedural analysis, and if we assume that anything can alter the value of a variable then we would also miss many true positives. But in this particular case I think the intention to modify the variable is clear and we should handle it.

@IagoAbal IagoAbal added alpha Relates to an experimental feature feature:const-propagation labels Jun 9, 2021
@IagoAbal
Copy link
Contributor Author

Actually this is mainly due to AST-to-IL not recognizing a call to the concat method as the Concat operation.

@stale
Copy link

stale bot commented Sep 13, 2021

This issue is being marked stale because there hasn't been any activity in 30 days. Please leave a comment if you think this issue is still relevant and should be prioritized, otherwise it will be automatically closed in 7 days (you can always reopen it later).

@stale stale bot added the stale needs prioritization; label applied by stalebot label Sep 13, 2021
@emjin
Copy link
Contributor

emjin commented Sep 14, 2021

@mmcqd Might be a good issue for you

@stale stale bot removed the stale needs prioritization; label applied by stalebot label Sep 14, 2021
IagoAbal added a commit that referenced this issue Sep 16, 2021
And make translation language-dependent.

Helps #3316

test plan:
make test # test included
IagoAbal added a commit that referenced this issue Sep 16, 2021
Closes #3316

test plan:
make test # test included
IagoAbal added a commit that referenced this issue Sep 17, 2021
And make translation language-dependent.

Helps #3316

test plan:
make test # test included
IagoAbal added a commit that referenced this issue Sep 17, 2021
Closes #3316

test plan:
make test # test included
IagoAbal added a commit that referenced this issue Sep 17, 2021
* AST_to_IL: Recognize "concat" method

And make translation language-dependent.

Helps #3316

test plan:
make test # test included

* constness: Assume that void method calls may update the callee

Closes #3316

test plan:
make test # test included

* Emma's review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
alpha Relates to an experimental feature feature:const-propagation
Development

Successfully merging a pull request may close this issue.

2 participants