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.

Sign up for GitHub

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

Jump to bottom

Add checker for static final buffers #12

Merged
merged 6 commits into from Oct 29, 2021
Merged

Add checker for static final buffers #12

merged 6 commits into from Oct 29, 2021

Conversation

alexc-db
Copy link
Contributor

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

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.

@alexc-db alexc-db mentioned this pull request Oct 27, 2021
Closed
@alexc-db
Copy link
Contributor Author

Note: initially I attempted to make it a RegexpMultiline check, but some multiline regexes caused checks to hang forever, and regex grew large and unreadable quick enough, I found it easier to implement checker that uses AST to narrow down to the static final byte buffer variable definitions and then uses simpler regex to verify that it's unreleasable and read-only.

@alexc-db alexc-db mentioned this pull request Oct 27, 2021
Merged
@normanmaurer normanmaurer merged commit 52567de into netty:master Oct 29, 2021
@normanmaurer
Copy link
Member

@alexc-db thanks a lot!

normanmaurer pushed a commit to netty/netty that referenced this pull request Nov 3, 2021
@alexc-db
Make static final buffers unreleasable and read-only (#11802)
30d9404 
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.
normanmaurer pushed a commit to netty/netty that referenced this pull request Nov 3, 2021
@alexc-db @normanmaurer
Make static final buffers unreleasable and read-only (#11802)
14f04b5 
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
@alexc-db @laosijikaichele
Make static final buffers unreleasable and read-only (netty#11802)
8f5be26 
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
@alexc-db @laosijikaichele
Make static final buffers unreleasable and read-only (netty#11802)
033ba24 
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
@alexc-db
Make static final buffers unreleasable and read-only (netty#11802)
445eedf 
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
Reviewers

@trustin trustin trustin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@alexc-db @normanmaurer @trustin