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

fix(encoder-writer): use write_all instead of write #90

Closed

Conversation

dignifiedquire
Copy link

If the inner writer does not write everything, the missing data is never written
using write_all, ensures all data is written to the inner writer

If the inner writer does not write everything, the missing data is never written
using write_all, ensures all data is written to the inner writer
@codecov-io
Copy link

Codecov Report

Merging #90 into master will decrease coverage by 0.67%.
The diff coverage is 82.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #90      +/-   ##
==========================================
- Coverage   96.52%   95.85%   -0.68%     
==========================================
  Files          12       12              
  Lines        1412     1448      +36     
==========================================
+ Hits         1363     1388      +25     
- Misses         49       60      +11
Impacted Files Coverage Δ
src/write/encoder.rs 85.71% <50%> (-2.39%) ⬇️
src/write/encoder_tests.rs 94.22% <86.11%> (-2.88%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1e211f1...8c8ef4b. Read the comment docs.

@marshallpierce
Copy link
Owner

You're right; good catch. (Sigh, and I've caught this same bug in other projects -- you'd think I'd have gotten it right in my own.)

Unfortunately this approach is not correct; an implementation of Write.write is only allowed to call the underlying delegate's write once, and write_all will potentially call it multiple times. See documentation for https://doc.rust-lang.org/std/io/trait.Write.html#tymethod.write.

@marshallpierce marshallpierce self-assigned this Dec 9, 2018
@dignifiedquire
Copy link
Author

dignifiedquire commented Dec 9, 2018

Maybe I am missing something, but this is the way that BufWriter solves this problem in the std lib effectively: https://doc.rust-lang.org/src/std/io/buffered.rs.html#473, so it seems there is some different interpretation of this.

@dignifiedquire
Copy link
Author

answering my own question: I guess the difference is that BufWriter is explicitly designed to change writes, vs any other „regular“ writer that produced data.

@marshallpierce marshallpierce mentioned this pull request Dec 15, 2018
@marshallpierce
Copy link
Owner

Opened rust-lang/rust#56889 about write_all and write not playing well.

@marshallpierce
Copy link
Owner

Given that the above issue is unlikely to be resolved quickly, I think I'll proceed with an implementation that returns Ok(0) and caution people to be careful about write_all. You can always write your own straightforward write_all clone in a couple of lines that doesn't do the nasty behavior with Ok(0)...

marshallpierce added a commit that referenced this pull request Dec 23, 2018
They'll be retried on subsequent writes, but this plays poorly with
write_all. See rust-lang/rust#56889.

Hat tip to #90.
@marshallpierce
Copy link
Owner

Closing this in favor of #92, which honors the contract of write by breaking write_all in some cases... :'(

bors bot referenced this pull request in comit-network/xmr-btc-swap Mar 17, 2021
332: Bump base64 from 0.12.3 to 0.13.0 r=thomaseizinger a=dependabot[bot]

Bumps [base64](https://github.com/marshallpierce/rust-base64) from 0.12.3 to 0.13.0.
<details>
<summary>Changelog</summary>
<p><em>Sourced from <a href="https://github.com/marshallpierce/rust-base64/blob/master/RELEASE-NOTES.md">base64's changelog</a>.</em></p>
<blockquote>
<h1>0.13.0</h1>
<ul>
<li>Config methods are const</li>
<li>Added <code>EncoderStringWriter</code> to allow encoding directly to a String</li>
<li><code>EncoderWriter</code> now owns its delegate writer rather than keeping a reference to it (though refs still work)
<ul>
<li>As a consequence, it is now possible to extract the delegate writer from an <code>EncoderWriter</code> via <code>finish()</code>, which returns <code>Result&lt;W&gt;</code> instead of <code>Result&lt;()&gt;</code>. If you were calling <code>finish()</code> explicitly, you will now need to use <code>let _ = foo.finish()</code> instead of just <code>foo.finish()</code> to avoid a warning about the unused value.</li>
</ul>
</li>
<li>When decoding input that has both an invalid length and an invalid symbol as the last byte, <code>InvalidByte</code> will be emitted instead of <code>InvalidLength</code> to make the problem more obvious.</li>
</ul>
<h1>0.12.2</h1>
<ul>
<li>Add <code>BinHex</code> alphabet</li>
</ul>
<h1>0.12.1</h1>
<ul>
<li>Add <code>Bcrypt</code> alphabet</li>
</ul>
<h1>0.12.0</h1>
<ul>
<li>A <code>Read</code> implementation (<code>DecoderReader</code>) to let users transparently decoded data from a b64 input source</li>
<li>IMAP's modified b64 alphabet</li>
<li>Relaxed type restrictions to just <code>AsRef&lt;[ut8]&gt;</code> for main <code>encode*</code>/<code>decode*</code> functions</li>
<li>A minor performance improvement in encoding</li>
</ul>
<h1>0.11.0</h1>
<ul>
<li>Minimum rust version 1.34.0</li>
<li><code>no_std</code> is now supported via the two new features <code>alloc</code> and <code>std</code>.</li>
</ul>
<h1>0.10.1</h1>
<ul>
<li>Minimum rust version 1.27.2</li>
<li>Fix bug in streaming encoding (<a href="https://github.com/marshallpierce/rust-base64/pull/90">#90</a>): if the underlying writer didn't write all the bytes given to it, the remaining bytes would not be retried later. See the docs on <code>EncoderWriter::write</code>.</li>
<li>Make it configurable whether or not to return an error when decoding detects excess trailing bits.</li>
</ul>
<h1>0.10.0</h1>
<ul>
<li>Remove line wrapping. Line wrapping was never a great conceptual fit in this library, and other features (streaming encoding, etc) either couldn't support it or could support only special cases of it with a great increase in complexity. Line wrapping has been pulled out into a <a href="https://crates.io/crates/line-wrap">line-wrap</a> crate, so it's still available if you need it.
<ul>
<li><code>Base64Display</code> creation no longer uses a <code>Result</code> because it can't fail, which means its helper methods for common
configs that <code>unwrap()</code> for you are no longer needed</li>
</ul>
</li>
<li>Add a streaming encoder <code>Write</code> impl to transparently base64 as you write.</li>
<li>Remove the remaining <code>unsafe</code> code.</li>
<li>Remove whitespace stripping to simplify <code>no_std</code> support. No out of the box configs use it, and it's trivial to do yourself if needed: <code>filter(|b| !b&quot; \n\t\r\x0b\x0c&quot;.contains(b)</code>.</li>
<li>Detect invalid trailing symbols when decoding and return an error rather than silently ignoring them.</li>
</ul>
<h1>0.9.3</h1>
<ul>
<li>Update safemem</li>
</ul>
<h1>0.9.2</h1>
<ul>
<li>Derive <code>Clone</code> for <code>DecodeError</code>.</li>
</ul>
<!-- raw HTML omitted -->
</blockquote>
<p>... (truncated)</p>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a href="https://github.com/marshallpierce/rust-base64/commit/b4fc91325ec985e2a18e83e95a3c08eebd636af4"><code>b4fc913</code></a> v0.13.0</li>
<li><a href="https://github.com/marshallpierce/rust-base64/commit/bba4c5d11e3b5f0974d7d126342e0ca67bb4cc1f"><code>bba4c5d</code></a> Merge pull request <a href="https://github.com/marshallpierce/rust-base64/issues/145">#145</a> from marshallpierce/mp/cleanup</li>
<li><a href="https://github.com/marshallpierce/rust-base64/commit/42967320b3c675c8ae5d3d46dd6e6aea4a7fc970"><code>4296732</code></a> Add docs and other cleanup</li>
<li><a href="https://github.com/marshallpierce/rust-base64/commit/6bb35566333bd4c2cc74b8b820af5bfcda12a1b8"><code>6bb3556</code></a> Merge pull request <a href="https://github.com/marshallpierce/rust-base64/issues/144">#144</a> from untitaker/invalid-bytes-not-length</li>
<li><a href="https://github.com/marshallpierce/rust-base64/commit/5b40e0c04ed522d10cfeaae0f0b343dd3e6f1d4d"><code>5b40e0c</code></a> Merge pull request <a href="https://github.com/marshallpierce/rust-base64/issues/142">#142</a> from marshallpierce/mp/string-writer</li>
<li><a href="https://github.com/marshallpierce/rust-base64/commit/8b1ae22babb33a5c7cee84f0445434bff6880c76"><code>8b1ae22</code></a> Rename StrWrite to StrConsumer</li>
<li><a href="https://github.com/marshallpierce/rust-base64/commit/27ccb6591e22dbe81840268feec0917feaaebd97"><code>27ccb65</code></a> fix tests</li>
<li><a href="https://github.com/marshallpierce/rust-base64/commit/d15cd384e1a91fa73a7c57f30d4b76df2149df68"><code>d15cd38</code></a> Give better error messages when decoding data with trailing newlines</li>
<li><a href="https://github.com/marshallpierce/rust-base64/commit/5a56885c655deb54ccb206165e8351b0476c78b8"><code>5a56885</code></a> Introduce StrWriter to allow ESW to wrap both a String and a &amp;mut String</li>
<li><a href="https://github.com/marshallpierce/rust-base64/commit/2dc0296d2a0c01387f6361c4c710f7d0c9e9aee2"><code>2dc0296</code></a> Merge pull request <a href="https://github.com/marshallpierce/rust-base64/issues/143">#143</a> from marshallpierce/mp/invalid-length-doc</li>
<li>Additional commits viewable in <a href="https://github.com/marshallpierce/rust-base64/compare/v0.12.3...v0.13.0">compare view</a></li>
</ul>
</details>
<br />


[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=base64&package-manager=cargo&previous-version=0.12.3&new-version=0.13.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
- `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)


</details>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
abraham-nixon referenced this pull request in abraham-nixon/xmr-btc-swap Feb 15, 2022
332: Bump base64 from 0.12.3 to 0.13.0 r=thomaseizinger a=dependabot[bot]

Bumps [base64](https://github.com/marshallpierce/rust-base64) from 0.12.3 to 0.13.0.
<details>
<summary>Changelog</summary>
<p><em>Sourced from <a href="https://github.com/marshallpierce/rust-base64/blob/master/RELEASE-NOTES.md">base64's changelog</a>.</em></p>
<blockquote>
<h1>0.13.0</h1>
<ul>
<li>Config methods are const</li>
<li>Added <code>EncoderStringWriter</code> to allow encoding directly to a String</li>
<li><code>EncoderWriter</code> now owns its delegate writer rather than keeping a reference to it (though refs still work)
<ul>
<li>As a consequence, it is now possible to extract the delegate writer from an <code>EncoderWriter</code> via <code>finish()</code>, which returns <code>Result&lt;W&gt;</code> instead of <code>Result&lt;()&gt;</code>. If you were calling <code>finish()</code> explicitly, you will now need to use <code>let _ = foo.finish()</code> instead of just <code>foo.finish()</code> to avoid a warning about the unused value.</li>
</ul>
</li>
<li>When decoding input that has both an invalid length and an invalid symbol as the last byte, <code>InvalidByte</code> will be emitted instead of <code>InvalidLength</code> to make the problem more obvious.</li>
</ul>
<h1>0.12.2</h1>
<ul>
<li>Add <code>BinHex</code> alphabet</li>
</ul>
<h1>0.12.1</h1>
<ul>
<li>Add <code>Bcrypt</code> alphabet</li>
</ul>
<h1>0.12.0</h1>
<ul>
<li>A <code>Read</code> implementation (<code>DecoderReader</code>) to let users transparently decoded data from a b64 input source</li>
<li>IMAP's modified b64 alphabet</li>
<li>Relaxed type restrictions to just <code>AsRef&lt;[ut8]&gt;</code> for main <code>encode*</code>/<code>decode*</code> functions</li>
<li>A minor performance improvement in encoding</li>
</ul>
<h1>0.11.0</h1>
<ul>
<li>Minimum rust version 1.34.0</li>
<li><code>no_std</code> is now supported via the two new features <code>alloc</code> and <code>std</code>.</li>
</ul>
<h1>0.10.1</h1>
<ul>
<li>Minimum rust version 1.27.2</li>
<li>Fix bug in streaming encoding (<a href="https://github.com/marshallpierce/rust-base64/pull/90">#90</a>): if the underlying writer didn't write all the bytes given to it, the remaining bytes would not be retried later. See the docs on <code>EncoderWriter::write</code>.</li>
<li>Make it configurable whether or not to return an error when decoding detects excess trailing bits.</li>
</ul>
<h1>0.10.0</h1>
<ul>
<li>Remove line wrapping. Line wrapping was never a great conceptual fit in this library, and other features (streaming encoding, etc) either couldn't support it or could support only special cases of it with a great increase in complexity. Line wrapping has been pulled out into a <a href="https://crates.io/crates/line-wrap">line-wrap</a> crate, so it's still available if you need it.
<ul>
<li><code>Base64Display</code> creation no longer uses a <code>Result</code> because it can't fail, which means its helper methods for common
configs that <code>unwrap()</code> for you are no longer needed</li>
</ul>
</li>
<li>Add a streaming encoder <code>Write</code> impl to transparently base64 as you write.</li>
<li>Remove the remaining <code>unsafe</code> code.</li>
<li>Remove whitespace stripping to simplify <code>no_std</code> support. No out of the box configs use it, and it's trivial to do yourself if needed: <code>filter(|b| !b&quot; \n\t\r\x0b\x0c&quot;.contains(b)</code>.</li>
<li>Detect invalid trailing symbols when decoding and return an error rather than silently ignoring them.</li>
</ul>
<h1>0.9.3</h1>
<ul>
<li>Update safemem</li>
</ul>
<h1>0.9.2</h1>
<ul>
<li>Derive <code>Clone</code> for <code>DecodeError</code>.</li>
</ul>
<!-- raw HTML omitted -->
</blockquote>
<p>... (truncated)</p>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a href="https://github.com/marshallpierce/rust-base64/commit/b4fc91325ec985e2a18e83e95a3c08eebd636af4"><code>b4fc913</code></a> v0.13.0</li>
<li><a href="https://github.com/marshallpierce/rust-base64/commit/bba4c5d11e3b5f0974d7d126342e0ca67bb4cc1f"><code>bba4c5d</code></a> Merge pull request <a href="https://github.com/marshallpierce/rust-base64/issues/145">#145</a> from marshallpierce/mp/cleanup</li>
<li><a href="https://github.com/marshallpierce/rust-base64/commit/42967320b3c675c8ae5d3d46dd6e6aea4a7fc970"><code>4296732</code></a> Add docs and other cleanup</li>
<li><a href="https://github.com/marshallpierce/rust-base64/commit/6bb35566333bd4c2cc74b8b820af5bfcda12a1b8"><code>6bb3556</code></a> Merge pull request <a href="https://github.com/marshallpierce/rust-base64/issues/144">#144</a> from untitaker/invalid-bytes-not-length</li>
<li><a href="https://github.com/marshallpierce/rust-base64/commit/5b40e0c04ed522d10cfeaae0f0b343dd3e6f1d4d"><code>5b40e0c</code></a> Merge pull request <a href="https://github.com/marshallpierce/rust-base64/issues/142">#142</a> from marshallpierce/mp/string-writer</li>
<li><a href="https://github.com/marshallpierce/rust-base64/commit/8b1ae22babb33a5c7cee84f0445434bff6880c76"><code>8b1ae22</code></a> Rename StrWrite to StrConsumer</li>
<li><a href="https://github.com/marshallpierce/rust-base64/commit/27ccb6591e22dbe81840268feec0917feaaebd97"><code>27ccb65</code></a> fix tests</li>
<li><a href="https://github.com/marshallpierce/rust-base64/commit/d15cd384e1a91fa73a7c57f30d4b76df2149df68"><code>d15cd38</code></a> Give better error messages when decoding data with trailing newlines</li>
<li><a href="https://github.com/marshallpierce/rust-base64/commit/5a56885c655deb54ccb206165e8351b0476c78b8"><code>5a56885</code></a> Introduce StrWriter to allow ESW to wrap both a String and a &amp;mut String</li>
<li><a href="https://github.com/marshallpierce/rust-base64/commit/2dc0296d2a0c01387f6361c4c710f7d0c9e9aee2"><code>2dc0296</code></a> Merge pull request <a href="https://github.com/marshallpierce/rust-base64/issues/143">#143</a> from marshallpierce/mp/invalid-length-doc</li>
<li>Additional commits viewable in <a href="https://github.com/marshallpierce/rust-base64/compare/v0.12.3...v0.13.0">compare view</a></li>
</ul>
</details>
<br />


[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=base64&package-manager=cargo&previous-version=0.12.3&new-version=0.13.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
- `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)


</details>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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.

3 participants