Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

Fix 20778 - Ensure that _d_print_throwable prints the entire message #3787

Merged
merged 2 commits into from
Mar 25, 2022

Conversation

MoonlightSentinel
Copy link
Contributor

@MoonlightSentinel MoonlightSentinel commented Mar 24, 2022

The previous implementation simply forwarded the pointer + length to
fprintf and didn't check the return value. This caused the message to
be truncated for the following cases:

  • it contains \0 (the bug report)
  • the length is greater than int.max

This commit changes the implementation s.t. it uses fwrite instead
which doesn't stop at \0 and (theoretically) supports lengths up to
size_t.max


Also added a commit to make test/exceptions less noisy (because the huge_message test would generate a huge log entry)

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @MoonlightSentinel! 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

Auto-close Bugzilla Severity Description
20778 regression exception messages with nulls within are treated inconsistently

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 "stable + druntime#3787"

@dlang-bot dlang-bot added the Bug Fix Include reference to corresponding bugzilla issue label Mar 24, 2022
@MoonlightSentinel MoonlightSentinel added the Regression PRs that fix regressions label Mar 24, 2022
@MoonlightSentinel MoonlightSentinel force-pushed the truncated-exception-message branch 2 times, most recently from 96ecab2 to 94c7678 Compare March 24, 2022 18:54
@kinke
Copy link
Contributor

kinke commented Mar 24, 2022

Could https://www.cplusplus.com/reference/cstdio/fwrite/ be a simpler option to just write the whole thing without stopping at nulls?

@MoonlightSentinel MoonlightSentinel force-pushed the truncated-exception-message branch 3 times, most recently from 3e9df05 to f73bcf8 Compare March 24, 2022 22:55
@MoonlightSentinel
Copy link
Contributor Author

Could cplusplus.com/reference/cstdio/fwrite be a simpler option to just write the whole thing without stopping at nulls?

You're rigth, fwrite is a lot simpler. In theory it even supports larger messages (>= int.max) - but that behaviour doesn't seem to be portable (the test for huge messages failed on some machines, I've removed it because it's a highly unlikely case).

@MoonlightSentinel MoonlightSentinel marked this pull request as ready for review March 24, 2022 22:59
The previous implementation simply forwarded the pointer + length to
`fprintf` and didn't check the return value. This caused the message to
be truncated for the following cases:

- it contains `\0` (the bug report)
- the length is greater than `int.max`

This commit changes the implementation s.t. it uses `fwrite` instead
which doesn't stop at `\0` and (theoretically) supports lengths up to
`size_t.max`
To reduce the noise and save some time
@RazvanN7 RazvanN7 merged commit 13441f1 into dlang:stable Mar 25, 2022
@MoonlightSentinel MoonlightSentinel deleted the truncated-exception-message branch March 25, 2022 11:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug Fix Include reference to corresponding bugzilla issue Regression PRs that fix regressions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants