TarFile: avoid Implicit narrowing conversion #750
Conversation
| @@ -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; | |||
There was a problem hiding this comment.
does this not also apply to filepos as well then?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Test Results 273 files ±0 273 suites ±0 1h 6m 8s ⏱️ +57s For more details on these errors, see this check. Results for commit 514c03b. ± Comparison against base commit f5c8301. ♻️ This comment has been updated with latest results. |
skip() uses long integer
skip() uses long integer