Bug 25578

Created attachment 31969 [details] [review]
Propsed Patch

goo/FixedPoint.cc
This crash is not seen for every ppt. Some ppt there is overflow in datatype for FixedPoint mutiplication and due to which its coming -ve value which is getting used in sqrt function and its returning zero.

Result in divide function is giving crash.

Please find the atached propsed attachment. 

Review it and let me know the status.

Hi Amit, thanks for the patch, some considerations:
 * Could you please post a proper git diff or diff -u instead of what you posted?
 * When you say ppt you mean pdf, right?
 * Do you have a pdf file that reproduces this crash than you can attach?
 * Also, please do not set the bug to fixed since it's not until someone commits a fix into the repository.

The patch is wrong for several reasons:
 * FixedPoint val field is suposed to be a 16.16 value, you are turning it into a 64 bits value which removes the point of using FixedPoint arithmetics and you could as well use the regular double one
 * You made div return 0 when diving by -1

The actual problem is that the multiplication of 215.876 * 215.876 doesn't fit in a 16.16 field

Of course the obvious fix is not using FixedPoint math for that multiplication

diff --git a/splash/SplashFTFont.cc b/splash/SplashFTFont.cc
index d4675f7..65371ba 100644
--- a/splash/SplashFTFont.cc
+++ b/splash/SplashFTFont.cc
@@ -40,6 +40,8 @@
 #include "SplashFTFontFile.h"
 #include "SplashFTFont.h"

+#include <cmath>
+
 //------------------------------------------------------------------------

 static int glyphPathMoveTo(const FT_Vector *pt, void *path);
@@ -67,13 +69,13 @@ SplashFTFont::SplashFTFont(SplashFTFontFile *fontFileA, SplashCoord *matA,
     return;
   }
   face->size = sizeObj;
-  size = splashSqrt(mat[2]*mat[2] + mat[3]*mat[3]);
+  size = sqrt((double)mat[2]*(double)mat[2] + (double)mat[3]*(double)mat[3]);
   if (FT_Set_Pixel_Sizes(face, 0, (int)size)) {
     return;
   }
   // if the textMat values are too small, FreeType's fixed point
   // arithmetic doesn't work so well
-  textScale = splashSqrt(textMat[2]*textMat[2] + textMat[3]*textMat[3]) / size;
+  textScale = sqrt((double)textMat[2]*(double)textMat[2] + (double)textMat[3]*(double)textMat[3]) / size;

   div = face->bbox.xMax > 20000 ? 65536 : 1;

OK thanks for the reply but splshSqrt is meant for fixedPoint also now its
using only sqrt function.

Thanks
Amit



On Sat, Dec 12, 2009 at 4:34 AM, <bugzilla-daemon@freedesktop.org> wrote:

> http://bugs.freedesktop.org/show_bug.cgi?id=25578
>
>
>
>
>
> --- Comment #4 from Albert Astals Cid <tsdgeos@terra.es>  2009-12-11
> 15:04:09 PST ---
> The patch is wrong for several reasons:
>  * FixedPoint val field is suposed to be a 16.16 value, you are turning it
> into
> a 64 bits value which removes the point of using FixedPoint arithmetics and
> you
> could as well use the regular double one
>  * You made div return 0 when diving by -1
>
> The actual problem is that the multiplication of 215.876 * 215.876 doesn't
> fit
> in a 16.16 field
>
> Of course the obvious fix is not using FixedPoint math for that
> multiplication
>
> diff --git a/splash/SplashFTFont.cc b/splash/SplashFTFont.cc
> index d4675f7..65371ba 100644
> --- a/splash/SplashFTFont.cc
> +++ b/splash/SplashFTFont.cc
> @@ -40,6 +40,8 @@
>  #include "SplashFTFontFile.h"
>  #include "SplashFTFont.h"
>
> +#include <cmath>
> +
>  //------------------------------------------------------------------------
>
>  static int glyphPathMoveTo(const FT_Vector *pt, void *path);
> @@ -67,13 +69,13 @@ SplashFTFont::SplashFTFont(SplashFTFontFile *fontFileA,
> SplashCoord *matA,
>     return;
>   }
>   face->size = sizeObj;
> -  size = splashSqrt(mat[2]*mat[2] + mat[3]*mat[3]);
> +  size = sqrt((double)mat[2]*(double)mat[2] +
> (double)mat[3]*(double)mat[3]);
>   if (FT_Set_Pixel_Sizes(face, 0, (int)size)) {
>     return;
>   }
>   // if the textMat values are too small, FreeType's fixed point
>   // arithmetic doesn't work so well
> -  textScale = splashSqrt(textMat[2]*textMat[2] + textMat[3]*textMat[3]) /
> size;
> +  textScale = sqrt((double)textMat[2]*(double)textMat[2] +
> (double)textMat[3]*(double)textMat[3]) / size;
>
>   div = face->bbox.xMax > 20000 ? 65536 : 1;
>
>
> --
> Configure bugmail: http://bugs.freedesktop.org/userprefs.cgi?tab=email
> ------- You are receiving this mail because: -------
> You reported the bug.
>

as i said, you can't fit the result of 215.876 * 215.876 in a FixedPoint (16.16 field) so the only option is not to use FixedPoint for that operation.

I have tried to fix this now for Maemo by using single precision floating point instead of fixed point.

Fixed point has the usual fixed point problems, and single precision floating point is actually faster than fixed point on the hardware that we care about in Maemo.

Created attachment 32429 [details] [review]
Add --enable-single-precision option

Upps, that patch includes the collateral damages to the autocruft.  Here is the relevant part:

Index: poppler/configure.ac
===================================================================
--- poppler.orig/configure.ac	2009-12-30 15:22:10.000000000 +0200
+++ poppler/configure.ac	2009-12-30 15:22:26.000000000 +0200
@@ -67,6 +67,10 @@
               enable_xpdf_headers="no")
 AM_CONDITIONAL(ENABLE_XPDF_HEADERS, test x$enable_xpdf_headers = xyes)
 
+AC_ARG_ENABLE(single-precision,
+[  --enable-single-precision     use single precision arithmetic (instead of double precision)],
+AC_DEFINE(USE_FLOAT,      [1], [Use single precision arithmetic]))
+
 AC_ARG_ENABLE(fixedpoint,
 [  --enable-fixedpoint     use fixed point (instead of floating point) arithmetic],
 AC_DEFINE(USE_FIXEDPOINT, [1], [Use fixed point arithmetic]))
Index: poppler/splash/SplashTypes.h
===================================================================
--- poppler.orig/splash/SplashTypes.h	2009-12-30 15:23:53.000000000 +0200
+++ poppler/splash/SplashTypes.h	2009-12-30 15:24:01.000000000 +0200
@@ -33,8 +33,12 @@
 #include "goo/FixedPoint.h"
 typedef FixedPoint SplashCoord;
 #else
+#if USE_FLOAT
+typedef float SplashCoord;
+#else
 typedef double SplashCoord;
 #endif
+#endif
 
 //------------------------------------------------------------------------
 // antialiasing
Index: poppler/splash/SplashMath.h
===================================================================
--- poppler.orig/splash/SplashMath.h	2009-12-30 15:25:05.000000000 +0200
+++ poppler/splash/SplashMath.h	2009-12-30 15:27:53.000000000 +0200
@@ -18,48 +18,72 @@
 #if USE_FIXEDPOINT
   return FixedPoint::abs(x);
 #else
+#if USE_FLOAT
+  return fabsf(x);
+#else
   return fabs(x);
 #endif
+#endif
 }
 
 static inline int splashFloor(SplashCoord x) {
   #if USE_FIXEDPOINT
     return FixedPoint::floor(x);
   #else
+  #if USE_FLOAT
+    return (int)floorf(x);
+  #else
     return (int)floor(x);
   #endif
+  #endif
 }
 
 static inline int splashCeil(SplashCoord x) {
 #if USE_FIXEDPOINT
   return FixedPoint::ceil(x);
 #else
+#if USE_FLOAT
+  return (int)ceilf(x);
+#else
   return (int)ceil(x);
 #endif
+#endif
 }
 
 static inline int splashRound(SplashCoord x) {
 #if USE_FIXEDPOINT
   return FixedPoint::round(x);
 #else
+#if USE_FLOAT
+  return (int)floorf(x + 0.5);
+#else
   return (int)floor(x + 0.5);
 #endif
+#endif
 }
 
 static inline SplashCoord splashSqrt(SplashCoord x) {
 #if USE_FIXEDPOINT
   return FixedPoint::sqrt(x);
 #else
+#if USE_FLOAT
+  return sqrtf(x);
+#else
   return sqrt(x);
 #endif
+#endif
 }
 
 static inline SplashCoord splashPow(SplashCoord x, SplashCoord y) {
 #if USE_FIXEDPOINT
   return FixedPoint::pow(x, y);
 #else
+#if USE_FLOAT
+  return powf(x, y);
+#else
   return pow(x, y);
 #endif
+#endif
 }
 
 static inline SplashCoord splashDist(SplashCoord x0, SplashCoord y0,
@@ -81,8 +105,12 @@
     return dya * FixedPoint::sqrt(dxa / dya + 1);
   }
 #else
+#if USE_FLOAT
+  return sqrtf(dx * dx + dy * dy);
+#else
   return sqrt(dx * dx + dy * dy);
 #endif
+#endif
 }
 
 #endif

That patch seems ok to me, only remaining question is, does nokia allow to license that code under the GPL2 or later?

Also we have lines in code with copyright like
// Copyright (C) 2010 Foo Bar <foo@bar.com>

What should i use in this case?

(In reply to comment #10)
> That patch seems ok to me, only remaining question is, does nokia allow to
> license that code under the GPL2 or later?

Yes.  I don't think my patch is copyrightable, actually, being so small and repetitive.

If you need a more formal statement, I can provide that as well.  I could add a header to the patch, with the usual GPL2 or later statement.

> Also we have lines in code with copyright like
> // Copyright (C) 2010 Foo Bar <foo@bar.com>
> 
> What should i use in this case?

Hmm, are you asking in general, or is this related to the patch?

Poppler is already used in Maemo since version 0.10 at least, so it is considered legally OK for that by Nokia.

But personally, I recommend to spell out the license in each source file, following the instructions that come with the GPL2.

Patch commited

(In reply to comment #12)
> Patch commited

Thank you!