Re: [32/34] ant git commit: java 5-8

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: [32/34] ant git commit: java 5-8

Stefan Bodewig
Matt,

something I overlooked on my first pass

On 2017-04-13, <[hidden email]> wrote:

> http://git-wip-us.apache.org/repos/asf/ant/blob/b7d1e9bd/src/main/org/apache/tools/ant/helper/ProjectHelper2.java
> @@ -256,7 +259,7 @@ public class ProjectHelper2 extends ProjectHelper {
>                      zf = new ZipFile(org.apache.tools.ant.launch.Locator
>                                       .fromJarURI(uri), "UTF-8");
>                      inputStream =
> -                        zf.getInputStream(zf.getEntry(uri.substring(pling + 1)));
> +                        zf.getInputStream(zf.getEntry(uri.substring(pling + 2)));

why did you change the offset from 1 to 2? Doesn't look right to me but
if it is we should probably change it for 1.9.x as well.

Stefan

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [32/34] ant git commit: java 5-8

Matt Benson-2
Hi Stefan,
  I had forgotten about this. When testing my other changes I noted that no
InputStream was returned here, due to the requested entry name including
the leading slash from the jar resource URL; the jar entries encountered
during testing, at least, lacked the leading slash. Due to this the actual
reading of the resource was deferred to the XML parser. Skipping the slash
fixed this issue for me and seemed anecdotally to decrease the amount of
time needed to run the tests. You of course know much more about the file
format than I; I have been unable to find a definitive resource that
confirms that jar file entries should always lack a leading separator.

Matt

On Wed, Apr 19, 2017 at 7:58 AM, Stefan Bodewig <[hidden email]> wrote:

> Matt,
>
> something I overlooked on my first pass
>
> On 2017-04-13, <[hidden email]> wrote:
>
> > http://git-wip-us.apache.org/repos/asf/ant/blob/b7d1e9bd/
> src/main/org/apache/tools/ant/helper/ProjectHelper2.java
> > @@ -256,7 +259,7 @@ public class ProjectHelper2 extends ProjectHelper {
> >                      zf = new ZipFile(org.apache.tools.ant.
> launch.Locator
> >                                       .fromJarURI(uri), "UTF-8");
> >                      inputStream =
> > -                        zf.getInputStream(zf.getEntry(uri.substring(pling
> + 1)));
> > +                        zf.getInputStream(zf.getEntry(uri.substring(pling
> + 2)));
>
> why did you change the offset from 1 to 2? Doesn't look right to me but
> if it is we should probably change it for 1.9.x as well.
>
> Stefan
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [32/34] ant git commit: java 5-8

Stefan Bodewig
On 2017-04-19, Matt Benson wrote:

> I had forgotten about this. When testing my other changes I noted that no
> InputStream was returned here, due to the requested entry name including
> the leading slash from the jar resource URL; the jar entries encountered
> during testing, at least, lacked the leading slash.

So this has never worked and nobody has noticed so far, strange.

> Due to this the actual reading of the resource was deferred to the XML
> parser. Skipping the slash fixed this issue for me and seemed
> anecdotally to decrease the amount of time needed to run the
> tests.

Wouldn't we end up with a null InputStream and an exception being thrown
later on?

> You of course know much more about the file format than I; I have been
> unable to find a definitive resource that confirms that jar file
> entries should always lack a leading separator.

A leading slash would violate the ZIP spec

https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT

       4.4.17.1 The name of the file, with optional relative path.
       The path stored MUST not contain a drive or
       device letter, or a leading slash. ...

so the current code is certainly wrong. I'll try to dig deeper into this
the coming days unless anybody else beats me to it.

Many thanks

        Stefan

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [32/34] ant git commit: java 5-8

Matt Benson-2
On Thu, Apr 20, 2017 at 2:48 AM, Stefan Bodewig <[hidden email]> wrote:

> On 2017-04-19, Matt Benson wrote:
>
> > I had forgotten about this. When testing my other changes I noted that no
> > InputStream was returned here, due to the requested entry name including
> > the leading slash from the jar resource URL; the jar entries encountered
> > during testing, at least, lacked the leading slash.
>
> So this has never worked and nobody has noticed so far, strange.
>
> > Due to this the actual reading of the resource was deferred to the XML
> > parser. Skipping the slash fixed this issue for me and seemed
> > anecdotally to decrease the amount of time needed to run the
> > tests.
>
> Wouldn't we end up with a null InputStream and an exception being thrown
> later on?
>
> In my debugging the XML parser allowed the null InputStream and took it
upon itself to go find the named resource. This caused a weird exception
that somehow coincided with the changes to Path and Union to use the Java 8
Stream APIs, prompting me to find and fix this bug.


> > You of course know much more about the file format than I; I have been
> > unable to find a definitive resource that confirms that jar file
> > entries should always lack a leading separator.
>
> A leading slash would violate the ZIP spec
>
> https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT
>
>        4.4.17.1 The name of the file, with optional relative path.
>        The path stored MUST not contain a drive or
>        device letter, or a leading slash. ...
>
> so the current code is certainly wrong. I'll try to dig deeper into this
> the coming days unless anybody else beats me to it.
>
> By "current" do you mean the code that increments past the ':' but not the
'/', i.e. until I fixed said bug?


> Many thanks
>
> Not at all, thank you!

Matt


>         Stefan
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [32/34] ant git commit: java 5-8

Stefan Bodewig
On 2017-04-20, Matt Benson wrote:

> On Thu, Apr 20, 2017 at 2:48 AM, Stefan Bodewig <[hidden email]> wrote:

>> Wouldn't we end up with a null InputStream and an exception being thrown
>> later on?

> In my debugging the XML parser allowed the null InputStream and took it
> upon itself to go find the named resource.

Ah, because of the SYSTEM id, I see.

>> so the current code is certainly wrong. I'll try to dig deeper into this
>> the coming days unless anybody else beats me to it.

> By "current" do you mean the code that increments past the ':' but not
> the '/', i.e. until I fixed said bug?

Right, the code in master has been fixed but the one in 1.9.x is still
broken. Apart from porting your fix I'd like to grep through the code
base to look for similar errors and maybe try to come up with a
test. Given the XML parser may be doing the job correctly anyway,
writing a test may not be possible.

Stefan

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]