Skip to content

Add validation of scanf format and arguments#10852

Merged
thewilsonator merged 10 commits intodlang:masterfrom
Luhrel:scanf-validation
Mar 6, 2020
Merged

Add validation of scanf format and arguments#10852
thewilsonator merged 10 commits intodlang:masterfrom
Luhrel:scanf-validation

Conversation

@Luhrel
Copy link
Contributor

@Luhrel Luhrel commented Mar 1, 2020

@WalterBright asked for it in the forums, so here it is.

Highly inspired by #10812.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @Luhrel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#10852"

Copy link
Contributor

@RazvanN7 RazvanN7 left a comment

Choose a reason for hiding this comment

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

Most of the logic here seems like a duplicate of chkprintf.d. Is there any difference between the mirrored functions? Even if there is, it should be minimal. I suggest that chkprintf file is renamed to chkformat and try to reuse the code as much as possible. For example: the parseFormatSpecifier function should be identical for both situations, there is no need in duplicating it; even if there are some differences, those should be special cased with if-else branches. Another example is the enum Format declaration: 2 declarations of the same thing.

@Luhrel Luhrel force-pushed the scanf-validation branch from 30aa90e to 7915bf8 Compare March 2, 2020 09:54
@Luhrel Luhrel force-pushed the scanf-validation branch from 7915bf8 to fca9f25 Compare March 2, 2020 10:18
@thewilsonator
Copy link
Contributor

Are sscanf and fscanf supported? (also for that matter sprtinf and fprintf?)

@Luhrel
Copy link
Contributor Author

Luhrel commented Mar 2, 2020

Most of the logic here seems like a duplicate of chkprintf.d. Is there any difference between the mirrored functions? Even if there is, it should be minimal. I suggest that chkprintf file is renamed to chkformat and try to reuse the code as much as possible. For example: the parseFormatSpecifier function should be identical for both situations, there is no need in duplicating it; even if there are some differences, those should be special cased with if-else branches. Another example is the enum Format declaration: 2 declarations of the same thing.

Done.

Are sscanf and fscanf supported? (also for that matter sprtinf and fprintf?)

Now, yes.

Copy link
Contributor

@thewilsonator thewilsonator left a comment

Choose a reason for hiding this comment

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

Also the changelog should mention that the file and string variants of scanf are also validated. This changelog entry should be merged with the printf one (if that hasn't already been released). It should also mention that fprintf and sprintf are also validated.

@Luhrel Luhrel force-pushed the scanf-validation branch from 8f746f4 to 606d2db Compare March 3, 2020 13:18
@thewilsonator thewilsonator dismissed their stale review March 4, 2020 04:44

Review addressed

@thewilsonator
Copy link
Contributor

This PR seems to have some issues with real. See the autotester for details e.g. https://auto-tester.puremagic.com/show-run.ghtml?projectid=1&runid=3964672&isPull=true

@Luhrel Luhrel force-pushed the scanf-validation branch from 6599590 to 59621f4 Compare March 4, 2020 20:32
@WalterBright
Copy link
Member

The errors in the test suite are about druntime's use of sprintf. This is why sprintf support should not have been lumped into this PR, as this PR is putatively about scanf.

I recommend removing fprintf and sprintf from this PR and putting them in a separate PR.

@Luhrel Luhrel force-pushed the scanf-validation branch from f345015 to 4b1ebbe Compare March 5, 2020 18:01
@Luhrel
Copy link
Contributor Author

Luhrel commented Mar 5, 2020

Done. @WalterBright Maybe you can merge this now.

@thewilsonator thewilsonator merged commit 2e18ba7 into dlang:master Mar 6, 2020
@Luhrel Luhrel deleted the scanf-validation branch March 6, 2020 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants