Skip to content

Track taint for String.valueOf(..) - #6382

Merged
aschackmull merged 2 commits into
github:mainfrom
bmuskalla:stringValueOfTaint
Aug 3, 2021
Merged

Track taint for String.valueOf(..)#6382
aschackmull merged 2 commits into
github:mainfrom
bmuskalla:stringValueOfTaint

Conversation

@bmuskalla

Copy link
Copy Markdown
Contributor

Given String.valueOf preserves the value of the passed parameter, mark its return value as tainted as well.

@bmuskalla bmuskalla added the Java label Jul 29, 2021
@bmuskalla
bmuskalla requested a review from aschackmull July 29, 2021 07:43
@bmuskalla
bmuskalla requested a review from a team as a code owner July 29, 2021 07:43
@aschackmull

Copy link
Copy Markdown
Contributor

This blanket inclusion of all the overloads deserve at least some investigation prior to merging. The char[] overloads are perfectly fine to include, but the ones for primitive types perhaps less so, and the one for Object should be considered together with general toString() and its impact on performance and FP-rates should be looked into prior to merging.

@bmuskalla

Copy link
Copy Markdown
Contributor Author

Thanks for the feedback @aschackmull - updated the PR accordingly and left out the valueOf(Object) variant for now. If you think it we should look into this right away, I'd need some help to gauge the implications of doing that.

@bmuskalla

Copy link
Copy Markdown
Contributor Author

And related to that: Do you think an addition like this is worth a change note?

@aschackmull

Copy link
Copy Markdown
Contributor

Do you think an addition like this is worth a change note?

No, this is too minor.

Also, the expected file needs an update.

@aschackmull aschackmull added the no-change-note-required This PR does not need a change note label Aug 3, 2021
@bmuskalla
bmuskalla force-pushed the stringValueOfTaint branch from 053c340 to 8ce8414 Compare August 3, 2021 12:48
@bmuskalla

Copy link
Copy Markdown
Contributor Author

Thanks. And sorry for the missing expected file, that was still lurking in my unstaged changes :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Java no-change-note-required This PR does not need a change note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants