Skip to content

Java: Add AsyncTask additional value step #7092

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 15, 2021
Merged

Java: Add AsyncTask additional value step #7092

merged 1 commit into from
Nov 15, 2021

Conversation

atorralba
Copy link
Contributor

@atorralba atorralba commented Nov 9, 2021 •
edited
Loading

This PR adds a new AdditionalValueStep for Android's AsyncTask. When execute or executeOnExecutor are called for a certain AsyncTask, their params argument flows (preserving value) to the doInBackground method of that AsyncTask implementation.

@atorralba atorralba requested a review from a team as a code owner November 9, 2021 16:50
@github-actions github-actions bot added the Java label Nov 9, 2021
intrigus-lgtm
intrigus-lgtm reviewed Nov 9, 2021
View reviewed changes
java/ql/lib/semmle/code/java/dataflow/FlowSteps.qll Outdated
*
* Extend this class to add additional jump steps that should apply to all
* dataflow configurations.
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the difference between a flow/taint step and a jump step?
A flow/taint step keeps the call context, a jump step doesn't?
Should this be explained in the docs in some way or is the following implication assumed to be true:
user uses AdditionalJumpStep -> user knows what a jump step is

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty much, but users don't need to know.

@aschackmull
Copy link
Contributor

I don't think we should expose the concept of jump steps in this way. Instead we can just add support for additional value steps and sort them into jump steps and local steps. Alternative implementation here: #7134

@atorralba
Copy link
Contributor Author

atorralba commented Nov 15, 2021 •
edited
Loading

I don't think we should expose the concept of jump steps in this way. Instead we can just add support for additional value steps and sort them into jump steps and local steps. Alternative implementation here: #7134

I actually renamed it to AdditionalJumpStep when I saw we'd need to add separate steps in two different places, but I didn't think of just reusing the same type for both. Your approach is much cleaner, thanks!

I'll wait for your PR to get merged and I'll rewrite this one to just include the AsyncTask step using the new type, then.

@atorralba atorralba force-pushed the atorralba/android-asynctask-jumpstep branch from 2e39d43 to 87ebcea Compare November 15, 2021 15:14
@atorralba atorralba changed the title Java: Add AsyncTask jump step Java: Add AsyncTask additional value step Nov 15, 2021
@atorralba atorralba merged commit d97b130 into github:main Nov 15, 2021
@atorralba atorralba deleted the atorralba/android-asynctask-jumpstep branch November 15, 2021 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@intrigus-lgtm intrigus-lgtm intrigus-lgtm left review comments

@aschackmull aschackmull aschackmull approved these changes

Assignees
No one assigned
Labels
Java
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@atorralba @aschackmull @intrigus-lgtm