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

TarFile: avoid Implicit narrowing conversion #750

Closed
wants to merge 1 commit into from
Closed

TarFile: avoid Implicit narrowing conversion #750

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions apitools/org.eclipse.pde.api.tools/src/org/eclipse/pde/api/tools/internal/util/TarFile.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ private static class TarInputStream extends FilterInputStream {
private int nextEntry = 0;
private int nextEOF = 0;
private int filepos = 0;
private int bytesread = 0;
private long bytesread = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

does this not also apply to filepos as well then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

May be, but codeQL does not complain about it. https://github.com/eclipse-pde/eclipse.pde/security/code-scanning

Refactoring all int to long here would be more complicated, because the used java.io.FilterInputStream.read(byte[], int, int) consumes only int.

Copy link
Contributor

Choose a reason for hiding this comment

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

But this then seems like codesmell, if the position in the file is limited to int, how can I ever read more bytes than an int can hold?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Due to the "Integer.decode(size.toString()).intValue();" the max filesize for files read with this class is limited to int size.
This commit is only to get rid of the false positive warning.

Copy link
Contributor

@laeubi laeubi Sep 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If it is a false positive there should be nothing to change (except some kind of suppress the warning itself). If we think there is a problem then it should be fixed e.g. by parsing the size as a long instead.

Maybe the warning can also go away by explicit casting the value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like to suppress warnings if its possible to change the code so that even the dump checker, who doesn't understand the whole workflow is happy.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the checker is dumb(?) we better disable it.
The purpose of such checks is to find problems in the code and fix them (in this case obviously long is better for handling possibly large files) than making the checker happy and give a false impression of security.

private TarEntry firstEntry = null;
private String longLinkName = null;

Expand Down Expand Up @@ -90,7 +90,7 @@ private long headerChecksum(byte[] header) {
* @throws IOException
*/
boolean skipToEntry(TarEntry entry) throws TarException, IOException {
int bytestoskip = entry.filepos - bytesread;
long bytestoskip = entry.filepos - bytesread;
if (bytestoskip < 0) {
return false;
}
Expand Down