You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ant.apache.org by KUNES Michael <Mi...@frequentis.com.INVALID> on 2023/02/23 07:59:29 UTC
Improvement ideas for Ant 1.10.13
Hi
Dear maintainers of the Apache Ant library. We were using this lib in an older version and found 2 improvements that would be nice to consider also for the next versions of the library since the code did not change in these parts as I've reviewed this week in 1.10.13
1) org.apache.tools.tar.TarEntry
The method public TarEntry[] getDirectoryEntries() could be improved to make a null check for
final String[] list = this.file.list();
because the result of this statement could be null and in this case the next statement would fail with NPE
TarEntry[] result = new TarEntry[list.length];
Possible/proposed solution:
public TarEntry[] getDirectoryEntries() {
if (this.file == null || !this.file.isDirectory()) {
return new TarEntry[0];
}
final TarEntry[] result;
final String[] list = this.file.list();
if (null != list) {
result = new TarEntry[list.length];
for (int i = 0; i < list.length; ++i) {
result[i] = new TarEntry(new File(this.file, list[i]));
}
}
else {
result = new TarEntry[0];
}
return result;
}
or
public TarEntry[] getDirectoryEntries() {
if (file == null || !file.isDirectory() || file.list() == null) {
return new TarEntry[0];
}
String[] list = file.list();
TarEntry[] result = new TarEntry[list.length];
for (int i = 0; i < list.length; ++i) {
result[i] = new TarEntry(new File(file, list[i]));
}
return result;
}
2) org.apache.tools.tar.TarBuffer
The method writeBlock() has this statement
this.outStream.write(this.blockBuffer, 0, this.blockSize);
we think, that this statement
this.outStream.write(this.blockBuffer, 0, this.currRecIdx * this.recordSize);
would be a better solution
Reason: with original code, after the EOF-blocks, the remaining data (garbage) from the last block was also put to the outstream
Since this is my first mail to this mailing list, I hope it was in correct form, understandable and does not violate any form of etiquette! If not, please tell me for future mails.
br
Michael
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Re: Improvement ideas for Ant 1.10.13
Posted by Stefan Bodewig <bo...@apache.org>.
On 2023-03-02, KUNES Michael wrote:
> Thank you for the hint! We will evaluate that alternative soon.
> Problem for such an evaluation is: we've a huge amount of different HW types (all embedded) with SW from brand new to VERY old (10y+). Therefore, such replacements always are dangerous and need lot of tests. In these cases: "never change a running SW" really has a meaning...
Ah, I see. In the case of "really old HW" the required minimum Java
version might become an issue. For Commons Compress and Ant 1.10.x that
would be Java 8 and might be too ambituous for your usage. (feels a bit
odd to write that in times where major frameworks move to Java 17 as
baseline, but I've worked on very conservative long running projects
myself :-).
The second issue you reported should have been fixed with Ant 1.8.0
which is compatible with Java 1.4 (like any other 1.8.x release).
The 1.9.x branch which is still maintained by us for serious bug fixes
requires Java 5 with 1.9.16 being the latest release.
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
RE: Improvement ideas for Ant 1.10.13
Posted by KUNES Michael <Mi...@frequentis.com.INVALID>.
Hi
Thank you for the hint! We will evaluate that alternative soon.
Problem for such an evaluation is: we've a huge amount of different HW types (all embedded) with SW from brand new to VERY old (10y+). Therefore, such replacements always are dangerous and need lot of tests. In these cases: "never change a running SW" really has a meaning...
Br
Michael
-----Original Message-----
From: Stefan Bodewig <bo...@apache.org>
Sent: Donnerstag, 2. März 2023 11:07
To: dev@ant.apache.org
Subject: Re: Improvement ideas for Ant 1.10.13
*EXTERNAL source*
On 2023-02-27, KUNES Michael wrote:
> We are using tar as part of our software to prepare data sent to
> devices that use tar or tgz format for receiving e.g.: CSV, XML,...
> files
Then I'd strongly recommend you look into replacing it with Commons Commpress at one point in time. The libary does support some POSIX tricks Ant's codebase doesn't support and they've added a TarFile class for random access (much like ZipFile) that would be out of scope for Ant.
Still.
> 1) I agree that this NPE might not occur under normal circumstances.
> Therefore, I am quite sure that the finding is due to our static code
> analysis tool remarks and just some beautification. But danger to
> introduce problems is quite low.
Do you want to prepare a formal bug report and patch - or a github pull request? Or should I just go ahead and add a null check myself? Either way is fine with me.
> 2) we did this fix some 10-15 years ago since we had some problem with
> one of our embedded devices that is not very fault-tolerant. But to be
> true, that's all I remember. Big sorry!
No problem at all. It would be good if you could give the updated Ant codebase with Arrays.fill a try to verify this really fixes the problem for you as well.
Many thanks
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org For additional commands, e-mail: dev-help@ant.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Re: Improvement ideas for Ant 1.10.13
Posted by Stefan Bodewig <bo...@apache.org>.
On 2023-03-02, Stefan Bodewig wrote:
> On 2023-02-27, KUNES Michael wrote:
>> 1) I agree that this NPE might not occur under normal
>> circumstances. Therefore, I am quite sure that the finding is due to
>> our static code analysis tool remarks and just some
>> beautification. But danger to introduce problems is quite low.
> Do you want to prepare a formal bug report and patch - or a github pull
> request? Or should I just go ahead and add a null check myself? Either
> way is fine with me.
I just realized it would be good to scan for other places where we don't
check File#list's result properly
https://github.com/apache/ant/commit/38b7e94c17c304aa3b7e13fc7d9ccdb47e4a4f89
This also addresses the TarEntry case.
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Re: Improvement ideas for Ant 1.10.13
Posted by Stefan Bodewig <bo...@apache.org>.
On 2023-02-27, KUNES Michael wrote:
> We are using tar as part of our software to prepare data sent to
> devices that use tar or tgz format for receiving e.g.: CSV,
> XML,... files
Then I'd strongly recommend you look into replacing it with Commons
Commpress at one point in time. The libary does support some POSIX
tricks Ant's codebase doesn't support and they've added a TarFile class
for random access (much like ZipFile) that would be out of scope for
Ant.
Still.
> 1) I agree that this NPE might not occur under normal
> circumstances. Therefore, I am quite sure that the finding is due to
> our static code analysis tool remarks and just some
> beautification. But danger to introduce problems is quite low.
Do you want to prepare a formal bug report and patch - or a github pull
request? Or should I just go ahead and add a null check myself? Either
way is fine with me.
> 2) we did this fix some 10-15 years ago since we had some problem with
> one of our embedded devices that is not very fault-tolerant. But to be
> true, that's all I remember. Big sorry!
No problem at all. It would be good if you could give the updated Ant
codebase with Arrays.fill a try to verify this really fixes the problem
for you as well.
Many thanks
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
RE: Improvement ideas for Ant 1.10.13
Posted by KUNES Michael <Mi...@frequentis.com.INVALID>.
Hi Stefan,
thanks for your nice reply!
We are using tar as part of our software to prepare data sent to devices that use tar or tgz format for receiving e.g.: CSV, XML,... files
Some more info
1) I agree that this NPE might not occur under normal circumstances. Therefore, I am quite sure that the finding is due to our static code analysis tool remarks and just some beautification. But danger to introduce problems is quite low.
2) we did this fix some 10-15 years ago since we had some problem with one of our embedded devices that is not very fault-tolerant. But to be true, that's all I remember. Big sorry!
We patched the code at this time and - shame on me - did not report this back to the community ☹ Since that time we used the adapted function without any problem.
Your suggestion sounds very good as best combination of two ideas. But since our fix was introduced such a long time ago my hope goes for some detailed analysis of the code by the maintainer - maybe due to some other code changes, my proposal is not needed anymore or would introduce new problems.
Br Michael
-----Original Message-----
From: Stefan Bodewig <bo...@apache.org>
Sent: Sonntag, 26. Februar 2023 12:35
To: dev@ant.apache.org
Subject: Re: Improvement ideas for Ant 1.10.13
*EXTERNAL source*
Hi Michael
many thanks for your suggestions. You could have just opened enahncement requests in Bugzilla but personally I appreciate you trying to have a discussion about the changes here first.
I am wondering whether you are using the tar package directly or you are actually facing issues with the code as it is while running Ant. In the former case I'd suggest you move to Apache Commons Compress which contains an improved offspring of Ant's tar package - it may even already contain fixes that are nor present in Ant's original code as they have not been backported.
On 2023-02-23, KUNES Michael wrote:
> 1) org.apache.tools.tar.TarEntry
> The method public TarEntry[] getDirectoryEntries() could be improved to make a null check for
> final String[] list = this.file.list(); because the result of this
> statement could be null and in this case the next statement would fail
> with NPE
you are correct.
Commons Compress solves this by not invoking #list anymore
https://github.com/apache/commons-compress/blob/master/src/main/java/org/apache/commons/compress/archivers/tar/TarArchiveEntry.java#L856
The case of #list returning null is a curious case since it probably represents and I/O error according to the javadoc - the other reason file not being a directory has already been checked for. Still returning an empty array probably is a better fallback than throwing an NPE.
> 2) org.apache.tools.tar.TarBuffer
> The method writeBlock() has this statement
> this.outStream.write(this.blockBuffer, 0, this.blockSize); we
> think, that this statement
> this.outStream.write(this.blockBuffer, 0, this.currRecIdx *
> this.recordSize); would be a better solution
> Reason: with original code, after the EOF-blocks, the remaining data
> (garbage) from the last block was also put to the outstream
Doesn't the Arrays.fill at the end of writeBlock ensure there is no remaining garbage in the block buffer?
Actually Common Compress writes records directly instead of batching them to full blocks and only ensures EOF is padded with 0-blocks to fill the last block, so it effectively avoid the garbage problem you talk about.
The proper fix here probably would be to apply your suggestion and in addition write enough data to fill the rest with 0s - or make sure blckbuffer is padded with zeros before writing it completely.
Sounds a lot like https://issues.apache.org/jira/browse/COMPRESS-81
which lead me to find
https://bz.apache.org/bugzilla/show_bug.cgi?id=47421 which suggest the same change you did. And back then I changed the code to zero out the block buffer. You said you were using th ecode of an older version of Ant, maybe you haven't applied
https://svn.apache.org/viewvc/ant/core/trunk/src/main/org/apache/tools/tar/TarBuffer.java?r1=789556&r2=789555&pathrev=789556
?
If you are already using the current master code then I wonder how the garbage gets into the blockBuffer so we can better understand how to fix this.
Cheers
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org For additional commands, e-mail: dev-help@ant.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Re: Improvement ideas for Ant 1.10.13
Posted by Stefan Bodewig <bo...@apache.org>.
Hi Michael
many thanks for your suggestions. You could have just opened enahncement
requests in Bugzilla but personally I appreciate you trying to have a
discussion about the changes here first.
I am wondering whether you are using the tar package directly or you are
actually facing issues with the code as it is while running Ant. In the
former case I'd suggest you move to Apache Commons Compress which
contains an improved offspring of Ant's tar package - it may even
already contain fixes that are nor present in Ant's original code as
they have not been backported.
On 2023-02-23, KUNES Michael wrote:
> 1) org.apache.tools.tar.TarEntry
> The method public TarEntry[] getDirectoryEntries() could be improved to make a null check for
> final String[] list = this.file.list();
> because the result of this statement could be null and in this case the next statement would fail with NPE
you are correct.
Commons Compress solves this by not invoking #list anymore
https://github.com/apache/commons-compress/blob/master/src/main/java/org/apache/commons/compress/archivers/tar/TarArchiveEntry.java#L856
The case of #list returning null is a curious case since it probably
represents and I/O error according to the javadoc - the other reason
file not being a directory has already been checked for. Still returning
an empty array probably is a better fallback than throwing an NPE.
> 2) org.apache.tools.tar.TarBuffer
> The method writeBlock() has this statement
> this.outStream.write(this.blockBuffer, 0, this.blockSize);
> we think, that this statement
> this.outStream.write(this.blockBuffer, 0, this.currRecIdx * this.recordSize);
> would be a better solution
> Reason: with original code, after the EOF-blocks, the remaining data (garbage) from the last block was also put to the outstream
Doesn't the Arrays.fill at the end of writeBlock ensure there is no
remaining garbage in the block buffer?
Actually Common Compress writes records directly instead of batching
them to full blocks and only ensures EOF is padded with 0-blocks to fill
the last block, so it effectively avoid the garbage problem you talk
about.
The proper fix here probably would be to apply your suggestion and in
addition write enough data to fill the rest with 0s - or make sure
blckbuffer is padded with zeros before writing it completely.
Sounds a lot like https://issues.apache.org/jira/browse/COMPRESS-81
which lead me to find
https://bz.apache.org/bugzilla/show_bug.cgi?id=47421 which suggest the
same change you did. And back then I changed the code to zero out the
block buffer. You said you were using th ecode of an older version of
Ant, maybe you haven't applied
https://svn.apache.org/viewvc/ant/core/trunk/src/main/org/apache/tools/tar/TarBuffer.java?r1=789556&r2=789555&pathrev=789556
?
If you are already using the current master code then I wonder how the
garbage gets into the blockBuffer so we can better understand how to fix
this.
Cheers
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org