From dbef94c2c1351277f40338aa120cfded3a8a4981 Mon Sep 17 00:00:00 2001 From: Alex Chekunkov Date: Tue, 26 Oct 2021 20:02:39 -0700 Subject: [PATCH 1/6] Add checker for static final buffers --- .../checkstyle/StaticFinalBufferCheck.java | 53 +++++++++++++++++++ .../main/resources/io/netty/checkstyle.xml | 4 +- 2 files changed, 56 insertions(+), 1 deletion(-) create mode 100644 common/src/main/java/io/netty/build/checkstyle/StaticFinalBufferCheck.java diff --git a/common/src/main/java/io/netty/build/checkstyle/StaticFinalBufferCheck.java b/common/src/main/java/io/netty/build/checkstyle/StaticFinalBufferCheck.java new file mode 100644 index 0000000..980057c --- /dev/null +++ b/common/src/main/java/io/netty/build/checkstyle/StaticFinalBufferCheck.java @@ -0,0 +1,53 @@ +package io.netty.build.checkstyle; + +import com.puppycrawl.tools.checkstyle.api.*; + +import java.util.regex.Pattern; + +public class StaticFinalBufferCheck extends AbstractCheck { + + private static final Pattern pattern = Pattern.compile( + "(Unpooled\\s*\\.)?unreleasableBuffer\\(.*?\\)\\s*\\.asReadOnly\\(\\)", + Pattern.MULTILINE); + + @Override + public int[] getRequiredTokens() { + return new int[]{TokenTypes.VARIABLE_DEF}; + } + + @Override + public int[] getDefaultTokens() { + return this.getRequiredTokens(); + } + + @Override + public int[] getAcceptableTokens() { + return this.getRequiredTokens(); + } + + @Override + public void visitToken(DetailAST ast) { + DetailAST modifiersAST = ast.findFirstToken(TokenTypes.MODIFIERS); + boolean isStatic = modifiersAST.findFirstToken(TokenTypes.LITERAL_STATIC) != null; + boolean isFinal = modifiersAST.findFirstToken(TokenTypes.FINAL) != null; + FullIdent typeIdent = FullIdent.createFullIdentBelow(ast.findFirstToken(TokenTypes.TYPE)); + if (!isStatic || !isFinal || !typeIdent.getText().endsWith("Buf")) { + return; + } + DetailAST assignAST = ast.findFirstToken(TokenTypes.ASSIGN); + DetailAST semiAST = ast.findFirstToken(TokenTypes.SEMI); + if (assignAST == null || semiAST == null) { + log(ast.getLineNo(), "Missing assignment for static final buffer"); + return; + } + FileContents fc = getFileContents(); + StringBuilder sb = new StringBuilder(); + for (int i = assignAST.getLineNo(); i <= semiAST.getLineNo(); i++) { + // getLineNo returns 1-based line number, getLine expects 0-based. + sb.append(fc.getLine(i - 1)); + } + if (!pattern.matcher(sb.toString()).find()) { + log(ast.getLineNo(), "static final buffer assignment should match pattern " + pattern); + } + } +} diff --git a/common/src/main/resources/io/netty/checkstyle.xml b/common/src/main/resources/io/netty/checkstyle.xml index aa40d65..0d86042 100644 --- a/common/src/main/resources/io/netty/checkstyle.xml +++ b/common/src/main/resources/io/netty/checkstyle.xml @@ -55,7 +55,7 @@ - + @@ -77,6 +77,8 @@ + + From c6ba7fe8e4df1b09a11720a1a12e7a5c8dd819b9 Mon Sep 17 00:00:00 2001 From: Alex Chekunkov Date: Wed, 27 Oct 2021 07:53:57 -0700 Subject: [PATCH 2/6] Add copyright and cleanup a couple of warnings --- .../checkstyle/StaticFinalBufferCheck.java | 20 +++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/common/src/main/java/io/netty/build/checkstyle/StaticFinalBufferCheck.java b/common/src/main/java/io/netty/build/checkstyle/StaticFinalBufferCheck.java index 980057c..8c79143 100644 --- a/common/src/main/java/io/netty/build/checkstyle/StaticFinalBufferCheck.java +++ b/common/src/main/java/io/netty/build/checkstyle/StaticFinalBufferCheck.java @@ -1,3 +1,19 @@ +/* + * Copyright 2021 The Netty Project + * + * The Netty Project licenses this file to you under the Apache License, + * version 2.0 (the "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at: + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ + package io.netty.build.checkstyle; import com.puppycrawl.tools.checkstyle.api.*; @@ -17,12 +33,12 @@ public int[] getRequiredTokens() { @Override public int[] getDefaultTokens() { - return this.getRequiredTokens(); + return getRequiredTokens(); } @Override public int[] getAcceptableTokens() { - return this.getRequiredTokens(); + return getRequiredTokens(); } @Override From e72e4cdcc7d8fb27880566cc3b6eae92c014858e Mon Sep 17 00:00:00 2001 From: Alex Chekunkov Date: Wed, 27 Oct 2021 08:12:58 -0700 Subject: [PATCH 3/6] Add docstring and trim lines while joining them. --- .../build/checkstyle/StaticFinalBufferCheck.java | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/common/src/main/java/io/netty/build/checkstyle/StaticFinalBufferCheck.java b/common/src/main/java/io/netty/build/checkstyle/StaticFinalBufferCheck.java index 8c79143..577474e 100644 --- a/common/src/main/java/io/netty/build/checkstyle/StaticFinalBufferCheck.java +++ b/common/src/main/java/io/netty/build/checkstyle/StaticFinalBufferCheck.java @@ -20,11 +20,17 @@ import java.util.regex.Pattern; +/** + * This check verifies that static final buffers are unreleasable and read-only. + * + * It aims to prevent corruption bugs like khttps://github.com/netty/netty/issues/11792 + * from happening in the future. + */ public class StaticFinalBufferCheck extends AbstractCheck { + // Pattern is single line because variable definition is flattened and trimmed before the match. private static final Pattern pattern = Pattern.compile( - "(Unpooled\\s*\\.)?unreleasableBuffer\\(.*?\\)\\s*\\.asReadOnly\\(\\)", - Pattern.MULTILINE); + "(Unpooled\\s*\\.)?unreleasableBuffer\\(.*?\\)\\s*\\.asReadOnly\\(\\)"); @Override public int[] getRequiredTokens() { @@ -60,7 +66,7 @@ public void visitToken(DetailAST ast) { StringBuilder sb = new StringBuilder(); for (int i = assignAST.getLineNo(); i <= semiAST.getLineNo(); i++) { // getLineNo returns 1-based line number, getLine expects 0-based. - sb.append(fc.getLine(i - 1)); + sb.append(fc.getLine(i - 1).trim()); } if (!pattern.matcher(sb.toString()).find()) { log(ast.getLineNo(), "static final buffer assignment should match pattern " + pattern); From 4df67855f58b4e70b5390db50c8a70c5ab9a125b Mon Sep 17 00:00:00 2001 From: Alex Chekunkov Date: Wed, 27 Oct 2021 09:56:39 -0700 Subject: [PATCH 4/6] Add support for suppressing checks using SuppressWarnings --- common/src/main/resources/io/netty/checkstyle.xml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/common/src/main/resources/io/netty/checkstyle.xml b/common/src/main/resources/io/netty/checkstyle.xml index 0d86042..77fd829 100644 --- a/common/src/main/resources/io/netty/checkstyle.xml +++ b/common/src/main/resources/io/netty/checkstyle.xml @@ -75,8 +75,10 @@ + + From 59144eb0469434acab33e255b2ae47e0f4dd0c2b Mon Sep 17 00:00:00 2001 From: Alex Chekunkov Date: Wed, 27 Oct 2021 18:09:38 -0700 Subject: [PATCH 5/6] Remove * import --- .../io/netty/build/checkstyle/StaticFinalBufferCheck.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/common/src/main/java/io/netty/build/checkstyle/StaticFinalBufferCheck.java b/common/src/main/java/io/netty/build/checkstyle/StaticFinalBufferCheck.java index 577474e..a93a85d 100644 --- a/common/src/main/java/io/netty/build/checkstyle/StaticFinalBufferCheck.java +++ b/common/src/main/java/io/netty/build/checkstyle/StaticFinalBufferCheck.java @@ -16,7 +16,11 @@ package io.netty.build.checkstyle; -import com.puppycrawl.tools.checkstyle.api.*; +import com.puppycrawl.tools.checkstyle.api.AbstractCheck; +import com.puppycrawl.tools.checkstyle.api.DetailAST; +import com.puppycrawl.tools.checkstyle.api.FileContents; +import com.puppycrawl.tools.checkstyle.api.FullIdent; +import com.puppycrawl.tools.checkstyle.api.TokenTypes; import java.util.regex.Pattern; From cb6847632af06545de1ba9c61d39c6d60dc76b32 Mon Sep 17 00:00:00 2001 From: Alex Chekunkov Date: Thu, 28 Oct 2021 10:01:41 -0700 Subject: [PATCH 6/6] minor fixes in the comments --- .../io/netty/build/checkstyle/StaticFinalBufferCheck.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/common/src/main/java/io/netty/build/checkstyle/StaticFinalBufferCheck.java b/common/src/main/java/io/netty/build/checkstyle/StaticFinalBufferCheck.java index a93a85d..839983f 100644 --- a/common/src/main/java/io/netty/build/checkstyle/StaticFinalBufferCheck.java +++ b/common/src/main/java/io/netty/build/checkstyle/StaticFinalBufferCheck.java @@ -27,12 +27,12 @@ /** * This check verifies that static final buffers are unreleasable and read-only. * - * It aims to prevent corruption bugs like khttps://github.com/netty/netty/issues/11792 + * It aims to prevent corruption bugs like https://github.com/netty/netty/issues/11792 * from happening in the future. */ public class StaticFinalBufferCheck extends AbstractCheck { - // Pattern is single line because variable definition is flattened and trimmed before the match. + // Pattern is not multiline because variable definition is flattened and trimmed before the match. private static final Pattern pattern = Pattern.compile( "(Unpooled\\s*\\.)?unreleasableBuffer\\(.*?\\)\\s*\\.asReadOnly\\(\\)");