Bug 59893 - Forbid calls to InputStream.available
Summary: Forbid calls to InputStream.available
Status: RESOLVED FIXED
Alias: None
Product: POI
Classification: Unclassified
Component: POI Overall (show other bugs)
Version: 3.15-dev
Hardware: All All
: P2 normal (vote)
Target Milestone: ---
Assignee: POI Developers List
URL:
Keywords:
Depends on: 59841
Blocks:
  Show dependency tree
 
Reported: 2016-07-23 12:11 UTC by Andreas Beeker
Modified: 2018-04-27 21:39 UTC (History)
0 users



Attachments
replace available() calls and add forbidden-apis-check (92.81 KB, patch)
2016-07-23 12:11 UTC, Andreas Beeker
Details | Diff
replace available() calls and add forbidden-apis-check (94.86 KB, patch)
2016-07-23 18:01 UTC, Andreas Beeker
Details | Diff
patch with some of the unrelated code removed (53.01 KB, patch)
2017-02-17 09:10 UTC, Javen O'Neal
Details | Diff
patch with all of the unrelated code removed (46.02 KB, patch)
2017-02-17 09:55 UTC, Javen O'Neal
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Beeker 2016-07-23 12:11:41 UTC
Created attachment 34067 [details]
replace available() calls and add forbidden-apis-check

In #59841 I've realized a lot of wrong calls to InputStream.available().
The wrong assumption is, that it returns the bytes until the EOF, whereas it really return the bytes until the next blocking read [1]

So when working with streams, you really need to read until EOF and not rely on available().

This patch removes most of the calls to available() or ignores them for delegates.


[1] http://stackoverflow.com/questions/3695372/what-does-inputstream-available-do-in-java
Comment 1 Andreas Beeker 2016-07-23 18:01:57 UTC
Created attachment 34068 [details]
replace available() calls and add forbidden-apis-check

fix issue with PropertySet
Comment 2 Dominik Stadler 2016-07-26 05:33:28 UTC
The patch has a number of unrelated changes, whitespaces, @Test, ... can you apply those and re-base the patch, that would make reviewing much easier.
Comment 3 Javen O'Neal 2017-02-17 09:10:21 UTC
Created attachment 34760 [details]
patch with some of the unrelated code removed

I committed some of Andi's unrelated changes from attachment 34068 [details] in r1783347 and r1783353.

Some of the line numbers may be off since I deleted unrelated changes from the patch by directly editing the text file.
Comment 4 Javen O'Neal 2017-02-17 09:28:02 UTC
ChunkedCipherInputStream.read needs attention. Changed from checking the return value of reading from the underlying stream from 1 to -1.

I have temporarily reverted this to restore the original behavior in 1783356. This should probably have been put back into the patch that excludes the unrelated stuff, since this behavior is related to the available() changes.
Comment 5 Javen O'Neal 2017-02-17 09:28:20 UTC
r1783356
Comment 6 Javen O'Neal 2017-02-17 09:55:21 UTC
Created attachment 34761 [details]
patch with all of the unrelated code removed

Less than half the size of the original. Hopefully this makes it easier to review, though it still needs to be rebased to trunk.
Comment 7 Javen O'Neal 2017-02-17 09:56:01 UTC
(unrelated changes) close leaked resources in r1783362
Comment 8 Andreas Beeker 2018-04-27 21:39:40 UTC
Patch rebased, reviewed and committed via r1830400