memononen / nanosvg

Simple stupid SVG parser
zlib License
1.71k stars 363 forks source link

Nano SVG is still locale dependent #139

Open zezic opened 5 years ago

zezic commented 5 years ago

It looks like Nano SVG parser is still depends on locale. Here is the related issue: VCVRack/Rack#461

wcout commented 5 years ago

Reason is most likely that nanosvg.h uses sscanf() to read float values.

Here is a patch to replace them with locale independent code (using what's already available):

diff --git a/src/nanosvg.h b/src/nanosvg.h
index 33fe706..5ad039f 100644
--- a/src/nanosvg.h
+++ b/src/nanosvg.h
@@ -1422,8 +1422,7 @@ static unsigned int nsvg__parseColor(const char* str)

 static float nsvg__parseOpacity(const char* str)
 {
-   float val = 0;
-   sscanf(str, "%f", &val);
+   float val = nsvg__atof(str);
    if (val < 0.0f) val = 0.0f;
    if (val > 1.0f) val = 1.0f;
    return val;
@@ -1431,8 +1430,7 @@ static float nsvg__parseOpacity(const char* str)

 static float nsvg__parseMiterLimit(const char* str)
 {
-   float val = 0;
-   sscanf(str, "%f", &val);
+   float val = nsvg__atof(str);
    if (val < 0.0f) val = 0.0f;
    return val;
 }
@@ -1463,9 +1461,9 @@ static int nsvg__parseUnits(const char* units)
 static NSVGcoordinate nsvg__parseCoordinateRaw(const char* str)
 {
    NSVGcoordinate coord = {0, NSVG_UNITS_USER};
-   char units[32]="";
-   sscanf(str, "%f%31s", &coord.value, units);
-   coord.units = nsvg__parseUnits(units);
+   char buf[64];
+   coord.units = nsvg__parseUnits(nsvg__parseNumber(str, buf, 64));
+   coord.value = nsvg__atof(buf);
    return coord;
 }

@@ -2505,7 +2503,25 @@ static void nsvg__parseSVG(NSVGparser* p, const char** attr)
            } else if (strcmp(attr[i], "height") == 0) {
                p->image->height = nsvg__parseCoordinate(p, attr[i + 1], 0.0f, 0.0f);
            } else if (strcmp(attr[i], "viewBox") == 0) {
-               sscanf(attr[i + 1], "%f%*[%%, \t]%f%*[%%, \t]%f%*[%%, \t]%f", &p->viewMinx, &p->viewMiny, &p->viewWidth, &p->viewHeight);
+               const char *s = attr[i + 1];
+               char buf[64];
+               s = nsvg__parseNumber(s, buf, 64);
+               p->viewMinx = nsvg__atof(buf);
+               while (*s && (nsvg__isspace(*s) || *s == '%' || *s == ',')) s++;
+               if (*s) {
+                   s = nsvg__parseNumber(s, buf, 64);
+                   p->viewMiny = nsvg__atof(buf);
+                   while (*s && (nsvg__isspace(*s) || *s == '%' || *s == ',')) s++;
+                   if (*s) {
+                       s = nsvg__parseNumber(s, buf, 64);
+                       p->viewWidth = nsvg__atof(buf);
+                       while (*s && (nsvg__isspace(*s) || *s == '%' || *s == ',')) s++;
+                       if (*s) {
+                           s = nsvg__parseNumber(s, buf, 64);
+                           p->viewHeight = nsvg__atof(buf);
+                       }
+                   }
+               }
            } else if (strcmp(attr[i], "preserveAspectRatio") == 0) {
                if (strstr(attr[i + 1], "none") != 0) {
                    // No uniform scaling
zezic commented 5 years ago

@wcout do you have a time to create PR from this patch?

poke1024 commented 5 years ago

This issue could get closed now I guess.

Albrecht-S commented 2 years ago

This issue could get closed now I guess.

@memononen For your information (and for all interested users):

Unfortunately commit c3ad36ef81992ff714cdbb4543cd67cb66daad8c, merged in PR #205 (commit 214cf85efcdc67524335ad0e2a2d5982246b6a72) on Apr 7, 2022 introduced a new locale dependency by using sscanf() with %f conversion specifier. Diff:

-   if (sscanf(str, "rgb(%u%%, %u%%, %u%%)", &r, &g, &b) == 3)  // decimal integer percentage
-       return NSVG_RGB(r*255/100, g*255/100, b*255/100);
+   float rf=0, gf=0, bf=0;
+   if (sscanf(str, "rgb(%f%%, %f%%, %f%%)", &rf, &gf, &bf) == 3)   // decimal integer percentage
+       return NSVG_RGB(round(rf*2.55f), round(gf*2.55f), round(bf*2.55f)); // (255 / 100.0f)

Before this commit only integer percent values would be parsed correctly, now fractional percent values are allowed but the locale dependency would break parsing fractional values if used on a locale that doesn't use '.' as separator.

(This is untested: noticed by code review.)

oehhar commented 2 years ago

Yes... This old issues. xfig was already sensible to this, that files could not exchanged, as German locale uses the "," as separator, but English locale uses ".". It would be great to also use "nsvg__atof(str)" for this case.

Albrecht-S commented 2 years ago

It would be great to also use "nsvg__atof(str)" for this case.

@oehhar I'm working on a fix and making good progress. I'll open a PR when all my final tests succeeded.

Albrecht-S commented 2 years ago

Please see PR #220 for my proposed fix.

oehhar commented 2 years ago

Thanks, Albrecht, great!

I suppose this may be closed.

As a side note: I use an archaic MS-VC6++ compiler for tests and it throws the following 4 warnings:

Warning 1

C:\test\git\tksvg\win\..\generic\nanosvg.h(1317) : warning C4244: '=' : conversion from 'double ' to 'float ', possible loss of data

The related code line is: rgbf[i] = nsvg__atof(str);

Warning 2

C:\test\git\tksvg\win\..\generic\nanosvg.h(1341) : warning C4244: '=' : conversion from 'float ' to 'unsigned int ', possible loss of data

The related code line is: rgbi[0] = roundf(rgbf[0] * 2.55f);

Warning 3

C:\test\git\tksvg\win\..\generic\nanosvg.h(1342) : warning C4244: '=' : conversion from 'float ' to 'unsigned int ', possible loss of data

The related code line is: rgbi[1] = roundf(rgbf[1] * 2.55f);

Warning 4

C:\test\git\tksvg\win\..\generic\nanosvg.h(1343) : warning C4244: '=' : conversion from 'float ' to 'unsigned int ', possible loss of data

The related code line is: rgbi[2] = roundf(rgbf[2] * 2.55f);

It might be ok (or not) to address them, as the rest of nanosvg does not throw any warnings. Simple casting may help.

Thank you all, Harald

Albrecht-S commented 2 years ago

@oehhar Thanks for your comment. Yes, MSVC compilers tend to issue all sorts of these warnings. Which warning level (usually /W3 or /W4) are you using?

That said, although I'm not the maintainer of this project, I intend to create another PR that should remove some kind of code duplication by implementing nsvg__strtof() and using it rather than the current nsvg__atof() as discussed elsewhere (https://github.com/memononen/nanosvg/pull/220#issuecomment-1179596094) in this project. I'll keep this issue in mind and I'll try to fix it with the new PR - unless someone else does it.

Unfortunately I'm currently too busy with another project but I'll come back to this when time permits.

oehhar commented 2 years ago

Dear Albrecht, thank you for the comment. There is no hurry. It is all great work and there is no need for beautification.

Your fix is in tksvg and tk 8.7 now, thanks for that.

About the warning level? It is "/W 3".

For your information, the whole nmake output is below. The compiler is MSVC6 with Paltform SDK 2003SP1.

Thank you again, Harald

C:\test\git\tksvg\win>nmake -f makefile.vc TCLDIR=c:\test\tcl8.6.12 TKDIR=c:\test\tk8.6.12 INSTALLDIR=c:\test\tksvg

Microsoft (R) Program Maintenance Utility   Version 6.00.9782.0
Copyright (C) Microsoft Corp 1988-1998. All rights reserved.

*** Using c:\test\tcl8.6.12\win\rules.vc
*** Compiler has 'Pentium 0x0f fix'

C:\test\git\tksvg\win>echo 0,11,0,0  1>>versions.vc
*** Building against Tcl at 'c:\test\tcl8.6.12'
*** Building against Tk at 'c:\test\tk8.6.12'
*** Intermediate directory will be 'C:\test\git\tksvg\win\Release\tksvg_ThreadedDynamic'
*** Output directory will be 'C:\test\git\tksvg\win\Release'
*** Installation, if selected, will be in 'c:\test\tksvg'
*** Suffix for binaries will be 't'
*** Compiler version 6. Target IX86, host AMD64.
  0 '@PACKAGE_VERSION@' => '0.11'
  1 '@PACKAGE_NAME@' => 'tksvg'
  2 '@PACKAGE_TCLNAME@' => 'tksvg'
  3 '@PKG_LIB_FILE@' => 'tksvg011t.dll'
  4 '@PKG_LIB_FILE8@' => 'tksvg011t.dll'
  5 '@PKG_LIB_FILE9@' => 'tcl9tksvg011t.dll'
        rc -fo C:\test\git\tksvg\win\Release\tksvg_ThreadedDynamic\tksvg.res -r -i "C:\test\git\tksvg\win\..\generic" -i "C:\test\git\tksvg\win\Release\tksvg_ThreadedDynamic"  -I"c:\test\tcl8.6.12\generic" -I"c:\test\tcl8.6.12\win"  /DDEBUG=0 -d UNCHECKED=0  /DCOMMAVERSION=0,11,0,0  /DDOTVERSION=\"0.11\"  /DVERSION=\"011\"  /DSUFX=\"t\"  /DPROJECT=\"tksvg\"  /DPRJLIBNAME=\"tksvg011t.dll\" C:\test\git\tksvg\win\Release\tksvg_ThreadedDynamic\tksvg.rc
        cl -nologo -c /D_ATL_XP_TARGETING  -W3 -FpC:\test\git\tksvg\win\Release\tksvg_ThreadedDynamic\ -Op -QI0f -O2 -YX -MD -I"c:\test\tcl8.6.12\generic" -I"c:\test\tcl8.6.12\win" -I"c:\test\tk8.6.12\generic" -I"c:\test\tk8.6.12\win" -I"c:\test\tk8.6.12\xlib"  -I"C:\test\git\tksvg\win\..\generic" -I"C:\test\git\tksvg\win\..\win" -I"C:\test\git\tksvg\win\..\compat"  -Dinline=__inline /DSTDC_HEADERS /DUSE_NMAKE=1 /DMP_NO_STDINT=1 /DTCL_THREADS=1 /DUSE_THREAD_ALLOC=1 /DNDEBUG /DTCL_CFG_OPTIMIZED /DNO_STRTOI64=1 /DUSE_TCL_STUBS /DUSE_TCLOO_STUBS /DUSE_TK_STUBS /DPACKAGE_NAME="\"tksvg\""  /DPACKAGE_TCLNAME="\"tksvg\""  /DPACKAGE_VERSION="\"0.11\""  /DMODULE_SCOPE=extern /DBUILD_tksvg -FoC:\test\git\tksvg\win\Release\tksvg_ThreadedDynamic\ @C:\Users\oehhar\AppData\Local\Temp\nmc06916.
tkImgSVG.c
C:\test\git\tksvg\win\..\generic\nanosvg.h(1317) : warning C4244: '=' : conversion from 'double ' to 'float ', possible loss of data
C:\test\git\tksvg\win\..\generic\nanosvg.h(1341) : warning C4244: '=' : conversion from 'float ' to 'unsigned int ', possible loss of data
C:\test\git\tksvg\win\..\generic\nanosvg.h(1342) : warning C4244: '=' : conversion from 'float ' to 'unsigned int ', possible loss of data
C:\test\git\tksvg\win\..\generic\nanosvg.h(1343) : warning C4244: '=' : conversion from 'float ' to 'unsigned int ', possible loss of data
        link -nologo -machine:IX86  -release -opt:ref -opt:icf,3 -dll -out:C:\test\git\tksvg\win\Release\tksvg011t.dll kernel32.lib advapi32.lib gdi32.lib user32.lib uxtheme.lib  "c:\test\tcl8.6.12\win\Release\tclstub86.lib" "c:\test\tcl8.6.12\win\Release\tcl86t.lib" "c:\test\tk8.6.12\win\Release\tkstub86.lib" "c:\test\tk8.6.12\win\Release\tk86t.lib" C:\test\git\tksvg\win\Release\tksvg_ThreadedDynamic\tkImgSVG.obj C:\test\git\tksvg\win\Release\tksvg_ThreadedDynamic\tksvg.res
   Creating library C:\test\git\tksvg\win\Release\tksvg011t.lib and object C:\test\git\tksvg\win\Release\tksvg011t.exp