Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nano SVG is still locale dependent #139

Open
zezic opened this issue Nov 30, 2018 · 10 comments
Open

Nano SVG is still locale dependent #139

zezic opened this issue Nov 30, 2018 · 10 comments

Comments

@zezic
Copy link

zezic commented Nov 30, 2018

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

@wcout
Copy link
Contributor

wcout commented Dec 4, 2018

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
Copy link
Author

zezic commented Dec 14, 2018

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

memononen added a commit that referenced this issue Dec 14, 2018
Fix issue #139: Nano SVG is still locale dependent
@poke1024
Copy link
Contributor

This issue could get closed now I guess.

@Albrecht-S
Copy link
Contributor

This issue could get closed now I guess.

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

Unfortunately commit c3ad36e, merged in PR #205 (commit 214cf85) 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
Copy link
Contributor

oehhar commented Jul 6, 2022

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
Copy link
Contributor

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 pushed a commit to fltk/nanosvg that referenced this issue Jul 8, 2022
…en#139)

This commit fixes the locale dependency (re-)introduced by commit
c3ad36e by using sscanf() to parse
floating point values.

This modification uses nsvg__atof() which is independent of the
current locale.
@Albrecht-S
Copy link
Contributor

Please see PR #220 for my proposed fix.

Albrecht-S pushed a commit to fltk/nanosvg that referenced this issue Jul 9, 2022
…en#139)

This commit fixes the locale dependency (re-)introduced by commit
c3ad36e by using sscanf() to parse
floating point values.

This modification uses nsvg__atof() which is independent of the
current locale.
memononen added a commit that referenced this issue Jul 9, 2022
Make nsvg__parseColorRGB() independent of the current locale (#139)
@oehhar
Copy link
Contributor

oehhar commented Aug 2, 2022

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
Copy link
Contributor

Albrecht-S commented Aug 3, 2022

@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 (#220 (comment)) 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
Copy link
Contributor

oehhar commented Aug 3, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants