mernst opened a new pull request #117:
URL: https://github.com/apache/commons-io/pull/117
In many parts of the Commons IO API, `null` may be passed as a Charset or
the name of one, and Commons IO uses the platform's default character encoding.
I assumed that was the case for ReversedLinesFileReader, and the result was
a null pointer exception.
Consider the following minimal example:
```
import java.io.File;
import java.io.IOException;
import java.nio.charset.Charset;
import org.apache.commons.io.input.ReversedLinesFileReader;
public class TestReversedLinesFileReader {
public static void main(String[] args) throws IOException {
ReversedLinesFileReader rfr =
new ReversedLinesFileReader(new
File("TestReversedLinesFileReader.java"),
(Charset) null);
}
}
```
Running this program results in:
```
Exception in thread "main" java.lang.NullPointerException
at java.lang.String.getBytes(String.java:940)
at
org.apache.commons.io.input.ReversedLinesFileReader.<init>(ReversedLinesFileReader.java:130)
at
org.apache.commons.io.input.ReversedLinesFileReader.<init>(ReversedLinesFileReader.java:78)
at TestReversedLinesFileReader.main(TestReversedLinesFileReader.java:8)
```
I assume that the `ReversedLinesFileReader` constructor is intended to
accept `null` as an argument because of [this
line](https://github.com/apache/commons-io/blob/6a78ef8903ef7c2c785b8d0cc08a150d1e2ba818/src/main/java/org/apache/commons/io/input/ReversedLinesFileReader.java#L139):
```
final Charset charset = Charsets.toCharset(encoding);
```
which calls `toCharset(Charset)` which is a no-op unless its argument is
`null`. So there would be no purpose to that line except to handle a null
`encoding` argument to the constructor.
The problem comes [a few lines
later](https://github.com/apache/commons-io/blob/6a78ef8903ef7c2c785b8d0cc08a150d1e2ba818/src/main/java/org/apache/commons/io/input/ReversedLinesFileReader.java#L169)
when the possibly-null value `encoding` is used:
This pull request changes the code so the `encoding` field is non-null even
if the `encoding` formal parameter is null, and uses `this.encoding` instead of
`encoding` in 4 locations.
I think that setting the field `encoding` to a non-null value is the right
thing because there are three calls to `new String` that use `encoding` as if
it is non-null.
An alternate fix would be to:
* document the Charset argument as being non-null,
* throw an exception within the constructor if a client passes null,
* remove the declaration of the `charset` local variable, and
* chang all uses of the `charset` local variable into uses of `encoding`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
