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

Make static final buffers unreleasable and read-only #11802

Merged
merged 6 commits into from Nov 3, 2021

Conversation

alexc-db
Copy link
Contributor

@alexc-db alexc-db commented Oct 27, 2021

Motivation:

Fix #11792 and avoid potential corruption issues caused by writable shared buffers.

TL;DR:

HttpObjectEncoder sends a duplicate of its static final ByteBuf field to encode HTTP chunks, such as 0\r\n\r\n. This reused buffer is neither read-only nor unexpandable, and thus anyone in the pipeline can modify or expand it. As a result, SslHandler can sometimes expand it and append stuff to it, causing unexpected data corruption.

Modification:

Most of the static final buffers in the project are made unreleasable and read-only by wrapping them in Unpooled.unreleasableBuffer(...).asReadOnly(). Buffers that were excluded:

  • Unpooled.EMPTY_BUFFER - EmptyByteBuf instances are not readable/writable/releasable. Wrapping this buffer in Unpooled.unreleasableBuffer(...).asReadOnly() would change the instance type, since it's a public API it could be a potential breaking change.
  • ReplayingDecoderByteBuf.EMPTY_BUFFER - since it uses Unpooled.EMPTY_BUFFER and not releasable too.

Excluded buffers are marked with SuppressWarnings to skip check added in netty/netty-build#12.

Result:

Fixes #11792.

@chrisvest
Copy link
Contributor

The reader indexes of these buffers can still be changed, and to guard for that you'd need to call duplicate() on every initial use site.

@chrisvest
Copy link
Contributor

Alternatively, the asReadOnly does wrapping already, so if that's moved to the use sites, we'd obtain the independent-indexes effect of duplicate that way too.

@alexc-db
Copy link
Contributor Author

Thanks @chrisvest! In most cases .duplicate() or .retainedDuplicate() was already used, I added a couple of missing ones in the tests.

…not readable or writable. Since it's a public API it may break the downstream dependencies. Added checkstyle suppression. Same for ReplayingDecoderByteBuf since it uses Unpooled.EMPTY_BUFFER.
@alexc-db alexc-db changed the title [WIP] Make all static final buffers unreleasable and read-only [WIP] Mark static final buffers unreleasable and read-only Oct 27, 2021
@alexc-db alexc-db changed the title [WIP] Mark static final buffers unreleasable and read-only [WIP] Make static final buffers unreleasable and read-only Oct 27, 2021
@alexc-db alexc-db changed the title [WIP] Make static final buffers unreleasable and read-only Make static final buffers unreleasable and read-only Oct 28, 2021
@@ -87,6 +87,7 @@
/**
* A buffer whose capacity is {@code 0}.
*/
@SuppressWarnings("checkstyle:StaticFinalBuffer") // EmptyByteBuf is not writeable or readable.
public static final ByteBuf EMPTY_BUFFER = ALLOC.buffer(0, 0);
Copy link
Member

Choose a reason for hiding this comment

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

I guess we could also just call asReadOnly() and remove the SupressWarnings ?

Copy link
Member

Choose a reason for hiding this comment

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

Also we should ensure to wrap it in unreleasableBuffer to make it more "future proof"

Copy link
Contributor Author

@alexc-db alexc-db Oct 29, 2021

Choose a reason for hiding this comment

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

That'd be an incompatible public API change. EMPTY_BUFFER is an instance of EmptyByteBuf (see static assert below). asReadOnly wraps instance into the ReadOnlyByteBuf (and unreleasableBuffer wraps in the UnreleasableByteBuf). If instance type is changed it'll break any downstream dependency that does instanceof EmptyByteBuf check.

What you suggested was my initial implementation, but the assert below and EmptyByteBuf API guarantees that it's not readable nor writable. Functionally, asReadOnly is not needed here, and SuppressWarnings + comment should be sufficient to document the expected behavior.

Let me know if I misses something, or if you think comment should be more detailed.

Copy link
Member

Choose a reason for hiding this comment

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

got it... ok yeah its ok then :)

@@ -44,6 +44,7 @@
private boolean terminated;
private SwappedByteBuf swapped;

@SuppressWarnings("checkstyle:StaticFinalBuffer") // Unpooled.EMPTY_BUFFER is not writeable or readable.
static final ReplayingDecoderByteBuf EMPTY_BUFFER = new ReplayingDecoderByteBuf(Unpooled.EMPTY_BUFFER);
Copy link
Member

Choose a reason for hiding this comment

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

See above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ditto #11802 (comment) - it's a package-private static variable that wraps an instance of EmptyByteBuf, asReadOnly would change the type of the wrapped instance.

Though I wasn't able to find usages, maybe this one can be removed? Or did I miss it?

normanmaurer pushed a commit to netty/netty-build that referenced this pull request Oct 29, 2021
Add validation for static final byte buffers to make sure they are unreleasable and read-only as a follow up for netty/netty#11792. Also enabled support for checkstyle warning suppression using `SuppressWarnings` annotation.

Verified by installing it locally and running it against local Netty code. Example:

```
→ mvn validate
[INFO] Scanning for projects...
[INFO] ------------------------------------------------------------------------
[INFO] Detecting the operating system and CPU architecture
[INFO] ------------------------------------------------------------------------
[INFO] os.detected.name: osx
[INFO] os.detected.arch: x86_64
[INFO] os.detected.bitness: 64
[INFO] os.detected.version: 10.16
[INFO] os.detected.version.major: 10
[INFO] os.detected.version.minor: 16
[INFO] os.detected.classifier: osx-x86_64
[INFO]
[INFO] ---------------------< io.netty:netty-codec-http >----------------------
[INFO] Building Netty/Codec/HTTP 4.1.70.Final-SNAPSHOT
[INFO] --------------------------------[ jar ]---------------------------------
[INFO]
[INFO] --- maven-enforcer-plugin:1.4.1:enforce (enforce-maven) @ netty-codec-http ---
[INFO]
[INFO] --- maven-enforcer-plugin:1.4.1:enforce (enforce-tools) @ netty-codec-http ---
[INFO]
[INFO] --- maven-checkstyle-plugin:3.1.0:check (check-style) @ netty-codec-http ---
[INFO] Starting audit...
[ERROR] /Users/alex.chekunkov/Dev/github.com/alexc-db/netty/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectEncoder.java:53: static final buffer assignment should match pattern (Unpooled\s*\.)?unreleasableBuffer\(.*?\)\s*\.asReadOnly\(\) [StaticFinalBuffer]
[ERROR] /Users/alex.chekunkov/Dev/github.com/alexc-db/netty/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectEncoder.java:54: static final buffer assignment should match pattern (Unpooled\s*\.)?unreleasableBuffer\(.*?\)\s*\.asReadOnly\(\) [StaticFinalBuffer]
[ERROR] /Users/alex.chekunkov/Dev/github.com/alexc-db/netty/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocket00FrameEncoder.java:34: static final buffer assignment should match pattern (Unpooled\s*\.)?unreleasableBuffer\(.*?\)\s*\.asReadOnly\(\) [StaticFinalBuffer]
[ERROR] /Users/alex.chekunkov/Dev/github.com/alexc-db/netty/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocket00FrameEncoder.java:36: static final buffer assignment should match pattern (Unpooled\s*\.)?unreleasableBuffer\(.*?\)\s*\.asReadOnly\(\) [StaticFinalBuffer]
[ERROR] /Users/alex.chekunkov/Dev/github.com/alexc-db/netty/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocket00FrameEncoder.java:38: static final buffer assignment should match pattern (Unpooled\s*\.)?unreleasableBuffer\(.*?\)\s*\.asReadOnly\(\) [StaticFinalBuffer]
Audit done.
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  3.755 s
[INFO] Finished at: 2021-10-26T20:00:56-07:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:3.1.0:check (check-style) on project netty-codec-http: Failed during checkstyle execution: There are 5 errors reported by Checkstyle 8.29 with io/netty/checkstyle.xml ruleset. -> [Help 1]
[ERROR]
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR]
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException
```

Findings from the check are fixed in netty/netty#11802.
Copy link
Contributor

@chrisvest chrisvest left a comment

Choose a reason for hiding this comment

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

I'm good with this if Norman's comments are addressed.

@normanmaurer
Copy link
Member

@alexc-db can you sign our ICLA: https://netty.io/s/icla ?

@normanmaurer normanmaurer added this to the 4.1.70.Final milestone Nov 1, 2021
@alexc-db
Copy link
Contributor Author

alexc-db commented Nov 1, 2021

@normanmaurer our legal team works on signing the CCLA, I'll check with them.

In the meantime - I noticed that you released netty-build with netty/netty-build#12, would you like me to include the updated version with a check in this PR or submit a separate PR afterwards?

@normanmaurer
Copy link
Member

@alexc-db yes please

@normanmaurer
Copy link
Member

@alexc-db let me know once the legal stuff is sorted as I would like to pull this in and do a release :)

@alexc-db
Copy link
Contributor Author

alexc-db commented Nov 2, 2021

@normanmaurer should be done as of now, please check if you can see the signed CCLA on your side. sorry for the delay :)

@normanmaurer
Copy link
Member

@alexc-db yep its there... thanks!

@normanmaurer normanmaurer merged commit 30d9404 into netty:4.1 Nov 3, 2021
normanmaurer pushed a commit that referenced this pull request Nov 3, 2021
Motivation:

Fix #11792 and avoid potential corruption issues caused by writable shared buffers.

TL;DR:

`HttpObjectEncoder` sends a duplicate of its static final `ByteBuf` field to encode HTTP chunks, such as `0\r\n\r\n`. This reused buffer is neither read-only nor unexpandable, and thus anyone in the pipeline can modify or expand it. As a result, `SslHandler` can sometimes expand it and append stuff to it, causing unexpected data corruption.

Modification:

Most of the static final buffers in the project are made unreleasable and read-only by wrapping them in `Unpooled.unreleasableBuffer(...).asReadOnly()`. Buffers that were excluded:

- Unpooled.EMPTY_BUFFER - EmptyByteBuf instances are not readable/writable/releasable. Wrapping this buffer in `Unpooled.unreleasableBuffer(...).asReadOnly()` would change the instance type, since it's a public API it could be a potential breaking change.
- ReplayingDecoderByteBuf.EMPTY_BUFFER - since it uses Unpooled.EMPTY_BUFFER and not releasable too.

Excluded buffers are marked with `SuppressWarnings` to skip check added in netty/netty-build#12.

Result:

Fixes #11792.
laosijikaichele pushed a commit to laosijikaichele/netty that referenced this pull request Dec 16, 2021
Motivation:

Fix netty#11792 and avoid potential corruption issues caused by writable shared buffers.

TL;DR:

`HttpObjectEncoder` sends a duplicate of its static final `ByteBuf` field to encode HTTP chunks, such as `0\r\n\r\n`. This reused buffer is neither read-only nor unexpandable, and thus anyone in the pipeline can modify or expand it. As a result, `SslHandler` can sometimes expand it and append stuff to it, causing unexpected data corruption.

Modification:

Most of the static final buffers in the project are made unreleasable and read-only by wrapping them in `Unpooled.unreleasableBuffer(...).asReadOnly()`. Buffers that were excluded:

- Unpooled.EMPTY_BUFFER - EmptyByteBuf instances are not readable/writable/releasable. Wrapping this buffer in `Unpooled.unreleasableBuffer(...).asReadOnly()` would change the instance type, since it's a public API it could be a potential breaking change.
- ReplayingDecoderByteBuf.EMPTY_BUFFER - since it uses Unpooled.EMPTY_BUFFER and not releasable too.

Excluded buffers are marked with `SuppressWarnings` to skip check added in netty/netty-build#12.

Result:

Fixes netty#11792.
laosijikaichele pushed a commit to laosijikaichele/netty that referenced this pull request Dec 16, 2021
Motivation:

Fix netty#11792 and avoid potential corruption issues caused by writable shared buffers.

TL;DR:

`HttpObjectEncoder` sends a duplicate of its static final `ByteBuf` field to encode HTTP chunks, such as `0\r\n\r\n`. This reused buffer is neither read-only nor unexpandable, and thus anyone in the pipeline can modify or expand it. As a result, `SslHandler` can sometimes expand it and append stuff to it, causing unexpected data corruption.

Modification:

Most of the static final buffers in the project are made unreleasable and read-only by wrapping them in `Unpooled.unreleasableBuffer(...).asReadOnly()`. Buffers that were excluded:

- Unpooled.EMPTY_BUFFER - EmptyByteBuf instances are not readable/writable/releasable. Wrapping this buffer in `Unpooled.unreleasableBuffer(...).asReadOnly()` would change the instance type, since it's a public API it could be a potential breaking change.
- ReplayingDecoderByteBuf.EMPTY_BUFFER - since it uses Unpooled.EMPTY_BUFFER and not releasable too.

Excluded buffers are marked with `SuppressWarnings` to skip check added in netty/netty-build#12.

Result:

Fixes netty#11792.
raidyue pushed a commit to raidyue/netty that referenced this pull request Jul 8, 2022
Motivation:

Fix netty#11792 and avoid potential corruption issues caused by writable shared buffers.

TL;DR:

`HttpObjectEncoder` sends a duplicate of its static final `ByteBuf` field to encode HTTP chunks, such as `0\r\n\r\n`. This reused buffer is neither read-only nor unexpandable, and thus anyone in the pipeline can modify or expand it. As a result, `SslHandler` can sometimes expand it and append stuff to it, causing unexpected data corruption.

Modification:

Most of the static final buffers in the project are made unreleasable and read-only by wrapping them in `Unpooled.unreleasableBuffer(...).asReadOnly()`. Buffers that were excluded:

- Unpooled.EMPTY_BUFFER - EmptyByteBuf instances are not readable/writable/releasable. Wrapping this buffer in `Unpooled.unreleasableBuffer(...).asReadOnly()` would change the instance type, since it's a public API it could be a potential breaking change.
- ReplayingDecoderByteBuf.EMPTY_BUFFER - since it uses Unpooled.EMPTY_BUFFER and not releasable too.

Excluded buffers are marked with `SuppressWarnings` to skip check added in netty/netty-build#12.

Result:

Fixes netty#11792.
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.

Data corruption due to interaction between SslHandler and HttpObjectEncoder
3 participants