Fop
  1. Fop
  2. FOP-1559

Handling of font URL pointing to a JAR entry don't work

    Details

    • Type: Bug Bug
    • Status: Closed
    • Resolution: Fixed
    • Affects Version/s: trunk
    • Fix Version/s: None
    • Component/s: font/unqualified
    • Labels:
      None
    • Environment:
      Operating System: Linux
      Platform: PC
    • External issue ID:
      45490

      Description

      There are a few lines of code in method
      public Source org.apache.fop.apps.FOURIResolver.resolve(String href, String base), which inhibit inclusion of fonts from a JAR file.

      If we have a JAR-File /path_to_jar/file.jar in filesystem, and within this JAR the font metrics exists in the JAR entry
      /path_within_jar/fonts/ttfnewsgothic.xml, then a valid URL to this entry is:

      jar:file:/path_to_jar/file.jar!/path_within_jar/fonts/ttfnewsgothic.xml

      If you use this URL, the method FOURIResolver.resolve(String href, String base)
      is called with the two arguments
      href: jar:file:/path_to_jar/file.jar!/path_within_jar/fonts/ttfnewsgothic.xml
      base: jar:file:/path_to_jar/file.jar!/path_within_jar/

      The problem is, that lines 194-196 remove "jar:" from href, and we run into
      the problem, that in line 208 the constructor new URL(URL basURL, String href)
      is called with a href still containing a protocol (file:), which is different
      from the protocol of the baseURL (jar, so
      absoluteURL = new URL(baseURL, href) sets absoluteURL to
      file:/path_to_file/file.jar!/path_within_jar/fonts/ttfnewsgothic.xml.

      This URL doesn't work, cause it's missing the proper protocol specification (jar.

      I have to admit, that I don't understand, what lines 197-203 should do (Ok, we prepend a slash to the url, if there is a colon and a slash in the url, and the
      colon is places before the slash, but why?), so I'm not sure that I can give a proper solution for that problem.

      But if it's not needed to remove the scheme from the url, if scheme isn't
      "file:" (cause only for this scheme, the slash is prepended), this diff whould
      help:

      195c236
      < if (href.startsWith(scheme)) {

      > if (href.startsWith(scheme) && "file:".equals(scheme)) {
      197d237
      < if ("file:".equals(scheme))

      { 206d245 < }

      (I already tried a posting on fop-dev, but actually I didn't get a proper contact person, so I try it this way.)

        Issue Links

          Activity

          Hide
          Andreas L. Delmelle added a comment -

          (In reply to comment #0)

          Just to make sure: which version of FOP are you using? The line numbers don't seem to match FOP Trunk, so it could help if we knew whether the issue still exists, and which version of the source files your comments refer to.

          <snip />
          > I have to admit, that I don't understand, what lines 197-203 should do (Ok, we
          > prepend a slash to the url, if there is a colon and a slash in the url, and the
          > colon is places before the slash, but why?), so I'm not sure that I can give a
          > proper solution for that problem.

          The reason: if the URL is a file URL, and the portion after 'file:' contains a colon, and the first slash (if any) appears after that colon, then the leading slash is missing (can happen on Windows, where some java.net.URL implementations return a "URL" like 'file:C:\...', which is obviously invalid)

          Show
          Andreas L. Delmelle added a comment - (In reply to comment #0) Just to make sure: which version of FOP are you using? The line numbers don't seem to match FOP Trunk, so it could help if we knew whether the issue still exists, and which version of the source files your comments refer to. <snip /> > I have to admit, that I don't understand, what lines 197-203 should do (Ok, we > prepend a slash to the url, if there is a colon and a slash in the url, and the > colon is places before the slash, but why?), so I'm not sure that I can give a > proper solution for that problem. The reason: if the URL is a file URL, and the portion after 'file:' contains a colon, and the first slash (if any) appears after that colon, then the leading slash is missing (can happen on Windows, where some java.net.URL implementations return a "URL" like 'file:C:\...', which is obviously invalid)
          Hide
          Thomas S. added a comment -

          > Just to make sure: which version of FOP are you using? The line numbers don't
          > seem to match FOP Trunk, so it could help if we knew whether the issue still
          > exists, and which version of the source files your comments refer to.

          Indeed giving line numbers isn't useful without specifiying version !

          I already tested this issue with 0.95b, with the sdame result. The line numbers correspond to release 0.94, but I looked at TRUNK version, and the relevant source isn't changed there.

          In the actual trunk (rev. 679326), the line numbers are 235-237, the code block seems to be unchanged since rev. 551874.

          > The reason: if the URL is a file URL, and the portion after 'file:' contains a
          > colon, and the first slash (if any) appears after that colon, then the leading
          > slash is missing (can happen on Windows, where some java.net.URL
          > implementations return a "URL" like 'file:C:\...', which is obviously invalid)

          Ooops, sometimes I forget this "other" operating system , now I understand the code. And I also think that my patch (see comment #0) will fix this issue, cause it only changes the behaviour of URLs with protocol != "file:".

          Show
          Thomas S. added a comment - > Just to make sure: which version of FOP are you using? The line numbers don't > seem to match FOP Trunk, so it could help if we knew whether the issue still > exists, and which version of the source files your comments refer to. Indeed giving line numbers isn't useful without specifiying version ! I already tested this issue with 0.95b, with the sdame result. The line numbers correspond to release 0.94, but I looked at TRUNK version, and the relevant source isn't changed there. In the actual trunk (rev. 679326), the line numbers are 235-237, the code block seems to be unchanged since rev. 551874. > The reason: if the URL is a file URL, and the portion after 'file:' contains a > colon, and the first slash (if any) appears after that colon, then the leading > slash is missing (can happen on Windows, where some java.net.URL > implementations return a "URL" like 'file:C:\...', which is obviously invalid) Ooops, sometimes I forget this "other" operating system , now I understand the code. And I also think that my patch (see comment #0) will fix this issue, cause it only changes the behaviour of URLs with protocol != "file:".
          Hide
          Andreas L. Delmelle added a comment -

          (In reply to comment #2)

          Thanks for supplying the additional info.

          <snip />
          > Ooops, sometimes I forget this "other" operating system , now I understand
          > the code. And I also think that my patch (see comment #0) will fix this issue,
          > cause it only changes the behaviour of URLs with protocol != "file:".
          >

          Somewhat of a nit, I think the suggested fix is, strictly speaking, not sufficient. The related portion of code is, as the preceding extensive comment explains, more or less a forced construction to retain backward compatibility with older parsers. The example URL 'file:c:\...' is not invalid according to RFC1630, provided the base URL is 'file:/'.

          Strictly speaking, I think the behavior should apply to other protocols as well, so simply bypassing the entire check for non-file URLs would not be the right choice. Even if the code-block does not contain any code yet for other protocols, I'm not sure we may simply exclude that.

          I suggest changing:

          if (href.startsWith(scheme)) {
          href = href.substring(...

          by

          if (href.startsWith(scheme)) {
          String tmp = href.substring(...

          Then use the tmp variable further down when prepending the missing slash.

          That way, if the protocol is something else than 'file:', at least the original href does not get corrupted.

          Show
          Andreas L. Delmelle added a comment - (In reply to comment #2) Thanks for supplying the additional info. <snip /> > Ooops, sometimes I forget this "other" operating system , now I understand > the code. And I also think that my patch (see comment #0) will fix this issue, > cause it only changes the behaviour of URLs with protocol != "file:". > Somewhat of a nit, I think the suggested fix is, strictly speaking, not sufficient. The related portion of code is, as the preceding extensive comment explains, more or less a forced construction to retain backward compatibility with older parsers. The example URL 'file:c:\...' is not invalid according to RFC1630, provided the base URL is 'file:/'. Strictly speaking, I think the behavior should apply to other protocols as well, so simply bypassing the entire check for non-file URLs would not be the right choice. Even if the code-block does not contain any code yet for other protocols, I'm not sure we may simply exclude that. I suggest changing: if (href.startsWith(scheme)) { href = href.substring(... by if (href.startsWith(scheme)) { String tmp = href.substring(... Then use the tmp variable further down when prepending the missing slash. That way, if the protocol is something else than 'file:', at least the original href does not get corrupted.
          Hide
          Thomas S. added a comment -

          Hi Andreas,

          I'm no sure if I understand the difference between your suggestion and mine.

          So I decided to paste the full source and the suggested changes (as far
          as I did understand them) here!

          Here's the original implementation:

          if (href.startsWith(scheme)) {
          href = href.substring(scheme.length());
          if ("file:".equals(scheme)) {
          int colonPos = href.indexOf(':');
          int slashPos = href.indexOf('/');
          if (slashPos >= 0 && colonPos >= && colonPos < slashPos)

          { href = "/" + href; // Absolute file URL doesn't have a leading slash }

          }
          }

          If I understand your comment #3, the result of the scheme removal should
          go to a temporary var. The check for colon and slash index will be made
          on this variable, right? And then, depending on the last condition, a
          leading slash will be added, ok?

          if (href.startsWith(scheme)) {
          String tmp = href.substring(scheme.length());
          if ("file:".equals(scheme)) {
          int colonPos = tmp.indexOf(':');
          int slashPos = tmp.indexOf('/');
          if (slashPos >= 0 && colonPos >= && colonPos < slashPos)

          { href = "/" + tmp; // Absolute file URL doesn't have a leading slash }

          }
          }

          But if I'm right here, I see no difference between my suggestion and yours?

          That's mine:

          if (href.startsWith(scheme) && "file:".equals(scheme)) {
          int colonPos = tmp.indexOf(':');
          int slashPos = tmp.indexOf('/');
          if (slashPos >= 0 && colonPos >= && colonPos < slashPos)

          { href = "/" + tmp; // Absolute file URL doesn't have a leading slash }

          }

          Help me out of my ignorance !

          Show
          Thomas S. added a comment - Hi Andreas, I'm no sure if I understand the difference between your suggestion and mine. So I decided to paste the full source and the suggested changes (as far as I did understand them) here! Here's the original implementation: if (href.startsWith(scheme)) { href = href.substring(scheme.length()); if ("file:".equals(scheme)) { int colonPos = href.indexOf(':'); int slashPos = href.indexOf('/'); if (slashPos >= 0 && colonPos >= && colonPos < slashPos) { href = "/" + href; // Absolute file URL doesn't have a leading slash } } } If I understand your comment #3, the result of the scheme removal should go to a temporary var. The check for colon and slash index will be made on this variable, right? And then, depending on the last condition, a leading slash will be added, ok? if (href.startsWith(scheme)) { String tmp = href.substring(scheme.length()); if ("file:".equals(scheme)) { int colonPos = tmp.indexOf(':'); int slashPos = tmp.indexOf('/'); if (slashPos >= 0 && colonPos >= && colonPos < slashPos) { href = "/" + tmp; // Absolute file URL doesn't have a leading slash } } } But if I'm right here, I see no difference between my suggestion and yours? That's mine: if (href.startsWith(scheme) && "file:".equals(scheme)) { int colonPos = tmp.indexOf(':'); int slashPos = tmp.indexOf('/'); if (slashPos >= 0 && colonPos >= && colonPos < slashPos) { href = "/" + tmp; // Absolute file URL doesn't have a leading slash } } Help me out of my ignorance !
          Hide
          Andreas L. Delmelle added a comment -

          (In reply to comment #4)
          >
          > I'm no sure if I understand the difference between your suggestion and mine.

          The end-result is definitely the same, right now, since the workaround for older parsers actually only does something for the file-protocol, while I think the same holds for other types of URLs as well... It's just that 'file:' is probably the most commonly used.

          That's what I meant by "Even if the code-block does not contain any code yet for other protocols...".

          Your suggestion would simply bypass the whole block, while mine would only avoid the assignment/modification of the original 'href' variable.
          If it turns out that in the future, we need a similar workaround for a different protocol, then your change would have to be undone, and mine would probably have to be chosen as an alternative.

          But in the end, you're right that the result is identical. Not sure what other devs prefer. If nobody gives an explicit opinion in due time (a couple of days), I'll stick my wet finger in the air, and see which one comes out.

          Again, thanks for the feedback. If only all bug-reports contained a pointer to the related lines in the source-files, that would make our lives a lot easier...

          Show
          Andreas L. Delmelle added a comment - (In reply to comment #4) > > I'm no sure if I understand the difference between your suggestion and mine. The end-result is definitely the same, right now, since the workaround for older parsers actually only does something for the file-protocol, while I think the same holds for other types of URLs as well... It's just that 'file:' is probably the most commonly used. That's what I meant by "Even if the code-block does not contain any code yet for other protocols...". Your suggestion would simply bypass the whole block, while mine would only avoid the assignment/modification of the original 'href' variable. If it turns out that in the future, we need a similar workaround for a different protocol, then your change would have to be undone, and mine would probably have to be chosen as an alternative. But in the end, you're right that the result is identical. Not sure what other devs prefer. If nobody gives an explicit opinion in due time (a couple of days), I'll stick my wet finger in the air, and see which one comes out. Again, thanks for the feedback. If only all bug-reports contained a pointer to the related lines in the source-files, that would make our lives a lot easier...
          Hide
          Thomas S. added a comment -

          Ok, now I got it!

          The question, if one will prefer an implementation, which is either open for future add-on or is the simplest possible implementation for the actual demand is rather a religious question, so I don't think it's useful to discuss it here .

          Anyway I thank you and look foreward to a fix of the observed issue: It will give us a nice, new opportunity for our layouting engine.

          Kind regards,
          Thomas

          Show
          Thomas S. added a comment - Ok, now I got it! The question, if one will prefer an implementation, which is either open for future add-on or is the simplest possible implementation for the actual demand is rather a religious question, so I don't think it's useful to discuss it here . Anyway I thank you and look foreward to a fix of the observed issue: It will give us a nice, new opportunity for our layouting engine. Kind regards, Thomas
          Hide
          Andreas L. Delmelle added a comment -

          Fixed in FOP Trunk. see: http://svn.apache.org/viewvc?rev=684491&view=rev

          Thanks for reporting, and suggesting the fix!

          Show
          Andreas L. Delmelle added a comment - Fixed in FOP Trunk. see: http://svn.apache.org/viewvc?rev=684491&view=rev Thanks for reporting, and suggesting the fix!
          Hide
          Glenn Adams added a comment -

          batch transition pre-FOP1.0 resolved+fixed bugs to closed+fixed

          Show
          Glenn Adams added a comment - batch transition pre-FOP1.0 resolved+fixed bugs to closed+fixed

            People

            • Assignee:
              fop-dev
              Reporter:
              Thomas S.
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development