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

Fix more DM_DEFAULT_ENCODING SpotBugs violations #6098

Merged
merged 22 commits into from
Jan 7, 2022
Merged

Fix more DM_DEFAULT_ENCODING SpotBugs violations #6098

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
98b0a3e
Fix more `DM_DEFAULT_ENCODING` SpotBugs violations
basil Dec 23, 2021
72d1a11
Inbound agent logs are stored on the controller, so they should be in…
basil Dec 23, 2021
3f8783c
Merge remote-tracking branch 'origin/master' into DM_DEFAULT_ENCODING
basil Dec 24, 2021
30658ee
Merge remote-tracking branch 'origin/master' into DM_DEFAULT_ENCODING
basil Dec 25, 2021
56ca70e
Merge remote-tracking branch 'origin/master' into DM_DEFAULT_ENCODING
basil Dec 27, 2021
28dcba1
Merge remote-tracking branch 'origin/master' into DM_DEFAULT_ENCODING
basil Dec 28, 2021
106ace3
Document the status quo
basil Dec 28, 2021
98fb885
Make TextFile internally consistent
basil Dec 28, 2021
758af4d
Revert "Make TextFile internally consistent"
basil Dec 28, 2021
7dca57c
Deprecate TextFile methods that use the default charset
basil Dec 28, 2021
8211d01
Merge remote-tracking branch 'origin/master' into DM_DEFAULT_ENCODING
basil Dec 29, 2021
b30b7ac
Merge remote-tracking branch 'origin/master' into DM_DEFAULT_ENCODING
basil Dec 30, 2021
907a10f
Merge remote-tracking branch 'origin/master' into DM_DEFAULT_ENCODING
basil Dec 31, 2021
797076f
Merge remote-tracking branch 'origin/master' into DM_DEFAULT_ENCODING
basil Jan 1, 2022
72353c8
Merge remote-tracking branch 'origin/master' into DM_DEFAULT_ENCODING
basil Jan 2, 2022
febd273
Fix a few more violations
basil Jan 2, 2022
b8d3f88
Deprecate methods without an explicit character set
basil Jan 2, 2022
fcf6413
Typo
basil Jan 2, 2022
f6553b5
Merge remote-tracking branch 'origin/master' into DM_DEFAULT_ENCODING
basil Jan 2, 2022
b1d9436
Merge remote-tracking branch 'origin/master' into DM_DEFAULT_ENCODING
basil Jan 3, 2022
e4b4978
Revert TextFile changes
basil Jan 4, 2022
1507bac
Merge remote-tracking branch 'origin/master' into DM_DEFAULT_ENCODING
basil Jan 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions core/src/main/java/hudson/FilePath.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,6 @@
import java.io.BufferedOutputStream;
import java.io.File;
import java.io.FileFilter;
import java.io.FileWriter;
import java.io.IOException;
import java.io.InputStream;
import java.io.InterruptedIOException;
Expand Down Expand Up @@ -1598,7 +1597,7 @@ public String invoke(File dir, VirtualChannel channel) throws IOException {
throw new IOException("Failed to create a temporary directory in "+dir,e);
}

try (Writer w = new FileWriter(f)) {
try (Writer w = Files.newBufferedWriter(Util.fileToPath(f), Charset.defaultCharset())) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Just a few lines above, the Javadoc states (emphasis mine):

Creates a temporary file in this directory […] and set the contents to the given text (encoded in the platform default encoding)

w.write(contents);
}

Expand Down
3 changes: 2 additions & 1 deletion core/src/main/java/hudson/cli/GroovyCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import groovy.lang.GroovyShell;
import hudson.Extension;
import java.io.IOException;
import java.io.OutputStreamWriter;
import java.io.PrintWriter;
import java.util.ArrayList;
import java.util.List;
Expand Down Expand Up @@ -62,7 +63,7 @@ protected int run() throws Exception {
Jenkins.get().checkPermission(Jenkins.ADMINISTER);

Binding binding = new Binding();
binding.setProperty("out",new PrintWriter(stdout,true));
binding.setProperty("out", new PrintWriter(new OutputStreamWriter(stdout, getClientCharset()), true));
binding.setProperty("stdin",stdin);
binding.setProperty("stdout",stdout);
binding.setProperty("stderr",stderr);
Expand Down
3 changes: 2 additions & 1 deletion core/src/main/java/hudson/cli/GroovyshCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import hudson.Extension;
import java.io.BufferedInputStream;
import java.io.InputStream;
import java.io.OutputStreamWriter;
import java.io.PrintStream;
import java.io.PrintWriter;
import java.util.ArrayList;
Expand Down Expand Up @@ -83,7 +84,7 @@ protected Groovysh createShell(InputStream stdin, PrintStream stdout,

Binding binding = new Binding();
// redirect "println" to the CLI
binding.setProperty("out", new PrintWriter(stdout,true));
binding.setProperty("out", new PrintWriter(new OutputStreamWriter(stdout, getClientCharset()), true));
binding.setProperty("hudson", Jenkins.get()); // backward compatibility
binding.setProperty("jenkins", Jenkins.get());

Expand Down
3 changes: 2 additions & 1 deletion core/src/main/java/hudson/cli/ListChangesCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import hudson.scm.ChangeLogSet;
import hudson.util.QuotedStringTokenizer;
import java.io.IOException;
import java.io.OutputStreamWriter;
import java.io.PrintWriter;
import java.util.List;
import jenkins.scm.RunWithSCM;
Expand Down Expand Up @@ -46,7 +47,7 @@ protected int act(List<Run<?, ?>> builds) throws IOException {
// No other permission check needed.
switch (format) {
case XML:
PrintWriter w = new PrintWriter(stdout);
PrintWriter w = new PrintWriter(new OutputStreamWriter(stdout, getClientCharset()));
w.println("<changes>");
for (Run<?, ?> build : builds) {
if (build instanceof RunWithSCM) {
Expand Down
7 changes: 5 additions & 2 deletions core/src/main/java/hudson/lifecycle/WindowsServiceLifecycle.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,12 @@
import hudson.util.StreamTaskListener;
import hudson.util.jna.Kernel32;
import java.io.File;
import java.io.FileWriter;
import java.io.IOException;
import java.io.Writer;
import java.net.URL;
import java.nio.charset.Charset;
import java.nio.file.Files;
import java.nio.file.StandardOpenOption;
import java.util.logging.Level;
import java.util.logging.Logger;
import jenkins.model.Jenkins;
Expand Down Expand Up @@ -109,7 +112,7 @@ public void rewriteHudsonWar(File by) throws IOException {
File baseDir = getBaseDir();
File copyFiles = new File(baseDir,baseName+".copies");

try (FileWriter w = new FileWriter(copyFiles, true)) {
try (Writer w = Files.newBufferedWriter(Util.fileToPath(copyFiles), Charset.defaultCharset(), StandardOpenOption.CREATE, StandardOpenOption.APPEND)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

A Windows-specific file, so we cannot assume UTF-8.

w.write(by.getAbsolutePath() + '>' + getHudsonWar().getAbsolutePath() + '\n');
}
}
Expand Down
3 changes: 1 addition & 2 deletions core/src/main/java/hudson/util/TextFile.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
import hudson.Util;
import java.io.BufferedReader;
import java.io.File;
import java.io.FileReader;
import java.io.IOException;
import java.io.PrintWriter;
import java.io.RandomAccessFile;
Expand Down Expand Up @@ -113,7 +112,7 @@ public void write(String text) throws IOException {
public @NonNull String head(int numChars) throws IOException {
char[] buf = new char[numChars];
int read = 0;
try (Reader r = new FileReader(file)) {
try (Reader r = Files.newBufferedReader(Util.fileToPath(file), Charset.defaultCharset())) {
Copy link
Member Author

Choose a reason for hiding this comment

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

TextFileTest uses Charset.defaultCharset when writing the test string, so it seems only reasonable that we would also use it when reading the same string.

Copy link
Member

Choose a reason for hiding this comment

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

Does not match

jenkins/core/src/main/java/hudson/util/TextFile.java

Lines 66 to 69 in 28dcba1

public String read() throws IOException {
StringWriter out = new StringWriter();
PrintWriter w = new PrintWriter(out);
try (BufferedReader in = Files.newBufferedReader(Util.fileToPath(file), StandardCharsets.UTF_8)) {
so I suggest fixing the test instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

But it does match

jenkins/core/src/main/java/hudson/util/TextFile.java

Lines 176 to 181 in 75ce81d

/**
* Uses the platform default encoding.
*/
public @NonNull String fastTail(int numChars) throws IOException {
return fastTail(numChars,Charset.defaultCharset());
}
so by the same reasoning I suggest leaving the code as-is. Fair enough?

Copy link
Member

Choose a reason for hiding this comment

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

These are secondary methods. read uses UTF-8 so everything else should be updated accordingly.

Copy link
Member Author

Choose a reason for hiding this comment

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

commit 14d980c FTR

Copy link
Member Author

Choose a reason for hiding this comment

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

read uses UTF-8 so everything else should be updated accordingly.

Pre-existing issue, but never mind that. Done.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually I am not comfortable taking on this risk changing the behavior of an existing public API. Reverted.

Copy link
Member

Choose a reason for hiding this comment

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

Then at least leave comments noting that the behavior of methods in this class is inconsistent. Not even sure if these non-read methods are used.

Copy link
Member Author

Choose a reason for hiding this comment

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

Opted for a more conservative approach of deprecating the methods that use the default charset in favor of non-deprecated versions that take an explicit charset.

Copy link
Member

Choose a reason for hiding this comment

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

read uses UTF-8 so everything else should be updated accordingly.

as does write and lines so these head/tail are bogus as is the comment on fastTail as the only possibility is for this to every be a UTF-8 encoded text file?

Thus forcing users to specify a charset for tail/head to be "correct" when the only correct possibility is "UTF-8" for the encoding of the text in the file seems wrong to me, and the mere existance of methods that take a charset would be a flag (and I note that some already exist!).

Besides that - this code is wrong as it is ignoring the passed in Charset and using Charset.defaultCharset()

there only seems to be a couple of usages of fastTail -
junit plugin seems to be using this for files that are not written by textFile but existing files. That seems like an abuse to to me, and changing behaviour here without first fixing that would likely be a regression.

usage-in-plugins reports just a few callers of these methods - so it would seem to be better to fix those (if they are similar to junit), and then not take a charset at all and make TextFile always be UTF-8?

flaky-test-handler:1.2.0
Methods
hudson/util/TextFile#fastTail(I)Ljava/lang/String;
hudson/util/TextFile#head(I)Ljava/lang/String;

junit:1.53
Methods
hudson/util/TextFile#fastTail(I)Ljava/lang/String;
hudson/util/TextFile#head(I)Ljava/lang/String;

mentor-questa-vrm:1.13
Methods
hudson/util/TextFile#fastTail(I)Ljava/lang/String;
hudson/util/TextFile#head(I)Ljava/lang/String;

while (read<numChars) {
int d = r.read(buf,read,buf.length-read);
if (d<0)
Expand Down
7 changes: 4 additions & 3 deletions core/src/main/java/jenkins/diagnosis/HsErrPidList.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,13 @@
import hudson.util.jna.Kernel32Utils;
import java.io.BufferedReader;
import java.io.File;
import java.io.FileReader;
import java.io.IOException;
import java.io.Reader;
import java.nio.MappedByteBuffer;
import java.nio.channels.FileChannel;
import java.nio.channels.FileChannel.MapMode;
import java.nio.charset.Charset;
import java.nio.file.Files;
import java.nio.file.InvalidPathException;
import java.nio.file.StandardOpenOption;
import java.util.ArrayList;
Expand Down Expand Up @@ -123,7 +124,7 @@ private void scan(String pattern) {
private void scanFile(File log) {
LOGGER.fine("Scanning "+log);

try (Reader rawReader = new FileReader(log);
try (Reader rawReader = Files.newBufferedReader(log.toPath(), Charset.defaultCharset());
basil marked this conversation as resolved.
Show resolved Hide resolved
BufferedReader r = new BufferedReader(rawReader)) {

if (!findHeader(r))
Expand All @@ -140,7 +141,7 @@ private void scanFile(File log) {
return;
}
}
} catch (IOException e) {
} catch (IOException | InvalidPathException e) {
// not a big enough deal.
LOGGER.log(Level.FINE, "Failed to parse hs_err_pid file: " + log, e);
}
Expand Down
6 changes: 4 additions & 2 deletions core/src/main/java/jenkins/slaves/DefaultJnlpSlaveReceiver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,12 @@
import hudson.slaves.SlaveComputer;
import java.io.IOException;
import java.io.OutputStream;
import java.io.OutputStreamWriter;
import java.io.PrintWriter;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.nio.channels.ClosedChannelException;
import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;
import java.security.MessageDigest;
import java.util.concurrent.ExecutionException;
Expand Down Expand Up @@ -152,7 +154,7 @@ public void beforeChannel(@NonNull JnlpConnectionState event) {
final SlaveComputer computer = state.getNode();
final OutputStream log = computer.openLogFile();
state.setLog(log);
try (PrintWriter logw = new PrintWriter(log, true)) {
try (PrintWriter logw = new PrintWriter(new OutputStreamWriter(log, Charset.defaultCharset()), true)) {
Copy link
Member

Choose a reason for hiding this comment

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

Agent log files should be in UTF-8.

Copy link
Member Author

Choose a reason for hiding this comment

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

Here and elsewhere, please provide reasoning when you make requests of me.

Copy link
Member

Choose a reason for hiding this comment

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

Any plugin can write text to these logs, and it is intended solely for use by Jenkins, so there is no reason to allow non-UTF-8 encoding.

Looks like some other places need to be fixed; at least:

jenkins/core/src/main/java/hudson/slaves/SlaveComputer.java

Line 155 in 75ce81d

this.taskListener = new StreamTaskListener(decorate(this.log));

jenkins/core/src/main/java/hudson/slaves/SlaveComputer.java

Line 623 in 75ce81d

final TaskListener taskListener = launchLog != null ? new StreamTaskListener(launchLog) : TaskListener.NULL;

jenkins/core/src/main/java/hudson/model/Computer.java

Line 335 in 75ce81d

return Util.loadFile(getLogFile(), /* TODO switch agent logs to UTF-8 */ Charset.defaultCharset());

jenkins/core/src/main/java/hudson/model/Computer.java

Line 343 in 75ce81d

return new AnnotatedLargeText<>(getLogFile(), Charset.defaultCharset(), false, this);

Copy link
Member Author

Choose a reason for hiding this comment

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

A bigger change than what I am trying to achieve in this PR. For now I have added a comment matching

jenkins/core/src/main/java/hudson/model/Computer.java

Line 335 in 75ce81d

return Util.loadFile(getLogFile(), /* TODO switch agent logs to UTF-8 */ Charset.defaultCharset());
to document the status quo.

logw.println("Inbound agent connected from " + event.getRemoteEndpointDescription());
}
for (ChannelConfigurator cc : ChannelConfigurator.all()) {
Expand All @@ -172,7 +174,7 @@ public void afterChannel(@NonNull JnlpConnectionState event) {
try {
computer.setChannel(event.getChannel(), state.getLog(), null);
} catch (IOException | InterruptedException e) {
PrintWriter logw = new PrintWriter(state.getLog(), true);
PrintWriter logw = new PrintWriter(new OutputStreamWriter(state.getLog(), Charset.defaultCharset()), true);
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Functions.printStackTrace(e, logw);
try {
event.getChannel().close();
Expand Down
9 changes: 0 additions & 9 deletions src/spotbugs/spotbugs-excludes.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,16 +51,7 @@
<Bug pattern="DM_DEFAULT_ENCODING"/>
<Or>
<Class name="hudson.cli.CLICommand"/>
<Class name="hudson.cli.GroovyCommand"/>
<Class name="hudson.cli.GroovyshCommand"/>
<Class name="hudson.cli.ListChangesCommand"/>
<Class name="hudson.FilePath$CreateTextTempFile"/>
<Class name="hudson.lifecycle.WindowsServiceLifecycle"/>
<Class name="hudson.util.StreamTaskListener"/>
<Class name="hudson.util.TextFile"/>
<Class name="jenkins.diagnosis.HsErrPidList"/>
<Class name="jenkins.security.s2m.AdminWhitelistRule"/>
<Class name="jenkins.slaves.DefaultJnlpSlaveReceiver"/>
</Or>
</And>
</Match>
Expand Down