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 more DM_DEFAULT_ENCODING SpotBugs violations #6098

Merged
merged 22 commits into from
Jan 7, 2022
Merged
Show file tree
Hide file tree
Changes from all 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
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,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 @@ -1656,7 +1655,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
7 changes: 6 additions & 1 deletion core/src/main/java/hudson/cli/CLICommand.java
Original file line number Diff line number Diff line change
Expand Up @@ -433,7 +433,12 @@ public final String getUsage() {
@Restricted(NoExternalUse.class)
public final String getLongDescription() {
ByteArrayOutputStream out = new ByteArrayOutputStream();
PrintStream ps = new PrintStream(out);
PrintStream ps;
try {
ps = new PrintStream(out, false, getClientCharset().name());
} catch (UnsupportedEncodingException e) {
throw new AssertionError(e);
}

printUsageSummary(ps);
ps.close();
Expand Down
3 changes: 2 additions & 1 deletion core/src/main/java/hudson/cli/GroovyCommand.java
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,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 @@ -63,7 +64,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
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,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 @@ -84,7 +85,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
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
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,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 @@ -110,7 +113,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
6 changes: 6 additions & 0 deletions core/src/main/java/hudson/model/StreamBuildListener.java
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,12 @@ public StreamBuildListener(File out, Charset charset) throws IOException {
super(out, charset);
}

/**
* @deprecated as of TODO
* The caller should use {@link #StreamBuildListener(OutputStream, Charset)} to pass in
* the charset and output stream separately, so that this class can handle encoding correctly.
*/
@Deprecated
public StreamBuildListener(OutputStream w) {
super(w);
}
Expand Down
5 changes: 3 additions & 2 deletions core/src/main/java/hudson/os/SU.java
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
import java.io.File;
import java.io.IOException;
import java.io.PrintStream;
import java.nio.charset.Charset;
import java.util.Collections;

/**
Expand Down Expand Up @@ -83,7 +84,7 @@ protected String sudoExe() {
return "sudo";
}

@SuppressFBWarnings(value = {"COMMAND_INJECTION", "DM_DEFAULT_ENCODING"}, justification = "TODO needs triage")
@SuppressFBWarnings(value = "COMMAND_INJECTION", justification = "TODO needs triage")
@Override
protected Process sudoWithPass(ArgumentListBuilder args) throws IOException {
args.prepend(sudoExe(), "-S");
Expand All @@ -92,7 +93,7 @@ protected Process sudoWithPass(ArgumentListBuilder args) throws IOException {
Process p = pb.start();
// TODO: use -p to detect prompt
// TODO: detect if the password didn't work
try (PrintStream ps = new PrintStream(p.getOutputStream())) {
try (PrintStream ps = new PrintStream(p.getOutputStream(), false, Charset.defaultCharset().name())) {
ps.println(rootPassword);
ps.println(rootPassword);
ps.println(rootPassword);
Expand Down
18 changes: 16 additions & 2 deletions core/src/main/java/hudson/util/StreamTaskListener.java
Original file line number Diff line number Diff line change
Expand Up @@ -76,14 +76,21 @@ public StreamTaskListener(@NonNull PrintStream out) {
this(out, null);
}

/**
* @deprecated as of TODO
* The caller should use {@link #StreamTaskListener(OutputStream, Charset)} to pass in
* the charset and output stream separately, so that this class can handle encoding correctly,
* or use {@link #fromStdout()} or {@link #fromStderr()}.
*/
@Deprecated
public StreamTaskListener(@NonNull OutputStream out) {
this(out, null);
}

public StreamTaskListener(@NonNull OutputStream out, @CheckForNull Charset charset) {
try {
if (charset == null)
this.out = out instanceof PrintStream ? (PrintStream) out : new PrintStream(out, false);
this.out = out instanceof PrintStream ? (PrintStream) out : new PrintStream(out, false, Charset.defaultCharset().name());
else
this.out = new PrintStream(out, false, charset.name());
this.charset = charset;
Expand All @@ -93,6 +100,12 @@ public StreamTaskListener(@NonNull OutputStream out, @CheckForNull Charset chars
}
}

/**
* @deprecated as of TODO
* The caller should use {@link #StreamTaskListener(File, Charset)} to pass in
* the charset and file separately, so that this class can handle encoding correctly.
*/
@Deprecated
public StreamTaskListener(@NonNull File out) throws IOException {
this(out, null);
}
Expand Down Expand Up @@ -187,8 +200,9 @@ private void writeObject(ObjectOutputStream out) throws IOException {
private static /* not final */ boolean AUTO_FLUSH = SystemProperties.getBoolean(KEY_AUTO_FLUSH);

private void readObject(ObjectInputStream in) throws IOException, ClassNotFoundException {
out = new PrintStream((OutputStream) in.readObject(), AUTO_FLUSH);
OutputStream os = (OutputStream) in.readObject();
String name = (String) in.readObject();
out = new PrintStream(os, AUTO_FLUSH, name != null ? name : Charset.defaultCharset().name());
charset = name == null ? null : Charset.forName(name);
if (LOGGER.isLoggable(Level.FINE)) {
LOGGER.log(Level.FINE, null, new Throwable("deserializing here with AUTO_FLUSH=" + AUTO_FLUSH));
Expand Down
7 changes: 4 additions & 3 deletions core/src/main/java/jenkins/diagnosis/HsErrPidList.java
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
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, /* TODO switch agent logs to UTF-8 */ Charset.defaultCharset()), true)) {
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(), /* TODO switch agent logs to UTF-8 */ Charset.defaultCharset()), true);
Functions.printStackTrace(e, logw);
try {
event.getChannel().close();
Expand Down
12 changes: 1 addition & 11 deletions src/spotbugs/spotbugs-excludes.xml
Original file line number Diff line number Diff line change
Expand Up @@ -45,22 +45,12 @@
annotation indicating the reason why it is a false positive, then remove the exclusion from
this section.
- If it is not a false positive, fix the bug, then remove the exclusion from this section.
-->
-->
<Match>
<And>
<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