Bug 185964

Summary: Codeset conversion to Chinese Simplified (HZ) is broken / segfaults
Product: Base System Reporter: Manuel <manuel-freebsd>
Component: miscAssignee: Tijl Coosemans <tijl>
Status: Closed FIXED    
Severity: Affects Only Me CC: xistence
Priority: Normal    
Version: 10.0-RELEASE   
Hardware: Any   
OS: Any   
Attachments:
Description Flags
iconv-HZ-start-comes-before-end.patch
none
iconv-VIQR-boundary-check.patch
none
iconv-void-ptr-ptr.patch none

Description Manuel 2014-01-21 13:30:00 UTC 
        Codeset conversion to Chinese Simplified (HZ) is broken and just segfaults

Fix: 

none known
How-To-Repeat:   $ echo "foo" | /usr/bin/iconv -f UTF-8 -t HZ
  ... or ...
  $ echo "foo" | /usr/bin/iconv -f UTF-8 -t HZ8
  ... or ...
        $ echo "foo" | /usr/bin/iconv -f UTF-8 -t HZ-GB-2312

        gdb backtrace:
  #0  0x0000000801a034af in _citrus_HZ_stdenc_getops () from /usr/lib/i18n/libHZ.so.4
  #1  0x0000000800a931a9 in _citrus_prop_parse_variable () from /lib/libc.so.7
  #2  0x0000000801a02abf in ?? () from /usr/lib/i18n/libHZ.so.4
  #3  0x0000000800a92db9 in _citrus_stdenc_open () from /lib/libc.so.7
  #4  0x0000000800dd69ef in ?? () from /usr/lib/i18n/libiconv_std.so.4
  #5  0x0000000800a94832 in _citrus_lookup_factory_convert () from /lib/libc.so.7
  #6  0x0000000800a926db in __bsd_iconv_open () from /lib/libc.so.7#
Comment 1 Manuel 2014-01-24 12:22:14 UTC 
Attached are patches against head to fix the reported problem + another
one I discovered during testing:

iconv-void-ptr-ptr.patch
...fixes the reported segfault, however HZ conversion still doesn't work.

iconv-HZ-start-comes-before-end.patch
...makes HZ conversion work. Verified using libiconv HZ test file.

iconv-VIQR-boundary-check.patch
...fixes a missing boundary check crash in VIQR codeset conversion
module I've discovered.

cheers,
manuel
Comment 2 Philippe Michel 2014-02-23 22:08:37 UTC 
This bug does not seem to affect only explicit conversion to Chinese.

At least it breaks the deskutils/fbreader port (it coredumps at startup 
with the deeper frames identical to those mentionned in the bug report). 
The patches attached to the first followup fix this as well.

Could you consider increasing its priority ?

Best regards,

Philippe Michel
Comment 3 Tijl Coosemans freebsd_committer freebsd_triage 2014-02-24 10:36:33 UTC 
Responsible Changed
From-To: freebsd-bugs->tijl

Take.
Comment 4 dfilter service freebsd_committer freebsd_triage 2014-02-24 13:33:29 UTC 
Author: tijl
Date: Mon Feb 24 13:33:20 2014
New Revision: 262441
URL: http://svnweb.freebsd.org/changeset/base/262441

Log:
  Consistently pass around context information using a simple pointer.  This
  fixes some dereferencing bugs in Chinese character set conversions.
  
  PR:		185964
  MFC after:	5 days

Modified:
  head/lib/libc/iconv/citrus_prop.c
  head/lib/libc/iconv/citrus_prop.h
  head/lib/libiconv_modules/BIG5/citrus_big5.c
  head/lib/libiconv_modules/HZ/citrus_hz.c

Modified: head/lib/libc/iconv/citrus_prop.c
==============================================================================
--- head/lib/libc/iconv/citrus_prop.c	Mon Feb 24 12:45:03 2014	(r262440)
+++ head/lib/libc/iconv/citrus_prop.c	Mon Feb 24 13:33:20 2014	(r262441)
@@ -339,7 +339,7 @@ name_found:
 
 static int
 _citrus_prop_parse_element(struct _memstream * __restrict ms,
-    const _citrus_prop_hint_t * __restrict hints, void ** __restrict context)
+    const _citrus_prop_hint_t * __restrict hints, void * __restrict context)
 {
 	int ch, errnum;
 #define _CITRUS_PROP_HINT_NAME_LEN_MAX	255
@@ -435,8 +435,7 @@ _citrus_prop_parse_variable(const _citru
 		if (ch == EOF || ch == '\0')
 			break;
 		_memstream_ungetc(&ms, ch);
-		errnum = _citrus_prop_parse_element(
-		    &ms, hints, (void ** __restrict)context);
+		errnum = _citrus_prop_parse_element(&ms, hints, context);
 		if (errnum != 0)
 			return (errnum);
 	}

Modified: head/lib/libc/iconv/citrus_prop.h
==============================================================================
--- head/lib/libc/iconv/citrus_prop.h	Mon Feb 24 12:45:03 2014	(r262440)
+++ head/lib/libc/iconv/citrus_prop.h	Mon Feb 24 13:33:20 2014	(r262441)
@@ -42,7 +42,7 @@ typedef struct _citrus_prop_hint_t _citr
 
 #define _CITRUS_PROP_CB0_T(_func_, _type_) \
 typedef int (*_citrus_prop_##_func_##_cb_func_t) \
-    (void ** __restrict, const char *, _type_); \
+    (void * __restrict, const char *, _type_); \
 typedef struct { \
 	_citrus_prop_##_func_##_cb_func_t func; \
 } _citrus_prop_##_func_##_cb_t;
@@ -52,7 +52,7 @@ _CITRUS_PROP_CB0_T(str, const char *)
 
 #define _CITRUS_PROP_CB1_T(_func_, _type_) \
 typedef int (*_citrus_prop_##_func_##_cb_func_t) \
-    (void ** __restrict, const char *, _type_, _type_); \
+    (void * __restrict, const char *, _type_, _type_); \
 typedef struct { \
 	_citrus_prop_##_func_##_cb_func_t func; \
 } _citrus_prop_##_func_##_cb_t;

Modified: head/lib/libiconv_modules/BIG5/citrus_big5.c
==============================================================================
--- head/lib/libiconv_modules/BIG5/citrus_big5.c	Mon Feb 24 12:45:03 2014	(r262440)
+++ head/lib/libiconv_modules/BIG5/citrus_big5.c	Mon Feb 24 13:33:20 2014	(r262441)
@@ -172,7 +172,7 @@ _citrus_BIG5_check_excludes(_BIG5Encodin
 }
 
 static int
-_citrus_BIG5_fill_rowcol(void ** __restrict ctx, const char * __restrict s,
+_citrus_BIG5_fill_rowcol(void * __restrict ctx, const char * __restrict s,
     uint64_t start, uint64_t end)
 {
 	_BIG5EncodingInfo *ei;
@@ -191,7 +191,7 @@ _citrus_BIG5_fill_rowcol(void ** __restr
 
 static int
 /*ARGSUSED*/
-_citrus_BIG5_fill_excludes(void ** __restrict ctx,
+_citrus_BIG5_fill_excludes(void * __restrict ctx,
     const char * __restrict s __unused, uint64_t start, uint64_t end)
 {
 	_BIG5EncodingInfo *ei;
@@ -237,7 +237,6 @@ static int
 _citrus_BIG5_encoding_module_init(_BIG5EncodingInfo * __restrict ei,
     const void * __restrict var, size_t lenvar)
 {
-	void *ctx = (void *)ei;
 	const char *s;
 	int err;
 
@@ -259,9 +258,9 @@ _citrus_BIG5_encoding_module_init(_BIG5E
 	}
 
 	/* fallback Big5-1984, for backward compatibility. */
-	_citrus_BIG5_fill_rowcol((void **)&ctx, "row", 0xA1, 0xFE);
-	_citrus_BIG5_fill_rowcol((void **)&ctx, "col", 0x40, 0x7E);
-	_citrus_BIG5_fill_rowcol((void **)&ctx, "col", 0xA1, 0xFE);
+	_citrus_BIG5_fill_rowcol(ei, "row", 0xA1, 0xFE);
+	_citrus_BIG5_fill_rowcol(ei, "col", 0x40, 0x7E);
+	_citrus_BIG5_fill_rowcol(ei, "col", 0xA1, 0xFE);
 
 	return (0);
 }

Modified: head/lib/libiconv_modules/HZ/citrus_hz.c
==============================================================================
--- head/lib/libiconv_modules/HZ/citrus_hz.c	Mon Feb 24 12:45:03 2014	(r262440)
+++ head/lib/libiconv_modules/HZ/citrus_hz.c	Mon Feb 24 13:33:20 2014	(r262441)
@@ -505,12 +505,12 @@ _citrus_HZ_encoding_module_uninit(_HZEnc
 }
 
 static int
-_citrus_HZ_parse_char(void **context, const char *name __unused, const char *s)
+_citrus_HZ_parse_char(void *context, const char *name __unused, const char *s)
 {
 	escape_t *escape;
 	void **p;
 
-	p = (void **)*context;
+	p = (void **)context;
 	escape = (escape_t *)p[0];
 	if (escape->ch != '\0')
 		return (EINVAL);
@@ -522,14 +522,14 @@ _citrus_HZ_parse_char(void **context, co
 }
 
 static int
-_citrus_HZ_parse_graphic(void **context, const char *name, const char *s)
+_citrus_HZ_parse_graphic(void *context, const char *name, const char *s)
 {
 	_HZEncodingInfo *ei;
 	escape_t *escape;
 	graphic_t *graphic;
 	void **p;
 
-	p = (void **)*context;
+	p = (void **)context;
 	escape = (escape_t *)p[0];
 	ei = (_HZEncodingInfo *)p[1];
 	graphic = malloc(sizeof(*graphic));
@@ -591,13 +591,13 @@ _CITRUS_PROP_HINT_END
 };
 
 static int
-_citrus_HZ_parse_escape(void **context, const char *name, const char *s)
+_citrus_HZ_parse_escape(void *context, const char *name, const char *s)
 {
 	_HZEncodingInfo *ei;
 	escape_t *escape;
 	void *p[2];
 
-	ei = (_HZEncodingInfo *)*context;
+	ei = (_HZEncodingInfo *)context;
 	escape = malloc(sizeof(*escape));
 	if (escape == NULL)
 		return (EINVAL);
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 5 dfilter service freebsd_committer freebsd_triage 2014-02-24 13:43:18 UTC 
Author: tijl
Date: Mon Feb 24 13:43:11 2014
New Revision: 262442
URL: http://svnweb.freebsd.org/changeset/base/262442

Log:
  Fix Simplified Chinese character set conversions by switching around the
  fields of an internal struct so it corresponds with the way variables of
  this type are initialised.
  
  PR:		185964
  Submitted by:	Manuel Mausz <manuel-freebsd@mausz.at>
  MFC after:	5 days

Modified:
  head/lib/libiconv_modules/HZ/citrus_hz.c

Modified: head/lib/libiconv_modules/HZ/citrus_hz.c
==============================================================================
--- head/lib/libiconv_modules/HZ/citrus_hz.c	Mon Feb 24 13:33:20 2014	(r262441)
+++ head/lib/libiconv_modules/HZ/citrus_hz.c	Mon Feb 24 13:43:11 2014	(r262442)
@@ -65,8 +65,8 @@ typedef enum {
 } charset_t;
 
 typedef struct {
-	int	 end;
 	int	 start;
+	int	 end;
 	int	 width;
 } range_t;
 
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 6 dfilter service freebsd_committer freebsd_triage 2014-02-24 14:40:36 UTC 
Author: tijl
Date: Mon Feb 24 14:40:28 2014
New Revision: 262447
URL: http://svnweb.freebsd.org/changeset/base/262447

Log:
  Fix an array index out of bounds bug in iconv VIQR (Vietnamese) module.
  
  PR:		185964
  Submitted by:	Manuel Mausz <manuel-freebsd@mausz.at>
  MFC after:	5 days

Modified:
  head/lib/libiconv_modules/VIQR/citrus_viqr.c

Modified: head/lib/libiconv_modules/VIQR/citrus_viqr.c
==============================================================================
--- head/lib/libiconv_modules/VIQR/citrus_viqr.c	Mon Feb 24 13:59:23 2014	(r262446)
+++ head/lib/libiconv_modules/VIQR/citrus_viqr.c	Mon Feb 24 14:40:28 2014	(r262447)
@@ -457,7 +457,7 @@ _citrus_VIQR_encoding_module_init(_VIQRE
 			return (errnum);
 		}
 	}
-	for (i = 0;; ++i) {
+	for (i = 0; i < mnemonic_ext_size; ++i) {
 		p = &mnemonic_ext[i];
 		n = strlen(p->name);
 		if (ei->mb_cur_max < n)
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 7 Tijl Coosemans freebsd_committer freebsd_triage 2014-02-24 14:41:15 UTC 
State Changed
From-To: open->patched

Committed in r262441, r262442 and r262447.
Comment 8 dfilter service freebsd_committer freebsd_triage 2014-03-04 11:43:10 UTC 
Author: tijl
Date: Tue Mar  4 11:43:01 2014
New Revision: 262731
URL: http://svnweb.freebsd.org/changeset/base/262731

Log:
  MFC r262441-262442,262447,262461-262464,262655:
  
  - Consistently pass around context information using a simple pointer.
    This fixes some dereferencing bugs in Chinese character set conversions.
  - Fix Simplified Chinese character set conversions by switching around the
    fields of an internal struct so it corresponds with the way variables of
    this type are initialised.
  - Fix an array index out of bounds bug in iconv VIQR (Vietnamese) module.
  - Silence gcc warning about unsigned comparison with 0.
  
  Also record r258316 and r258587 as merged so they no longer show up as
  eligible.
  
  PR:		185964
  Submitted by:	Manuel Mausz <manuel-freebsd@mausz.at>

Modified:
  stable/10/lib/libc/iconv/citrus_prop.c
  stable/10/lib/libc/iconv/citrus_prop.h
  stable/10/lib/libiconv_modules/BIG5/citrus_big5.c
  stable/10/lib/libiconv_modules/HZ/citrus_hz.c
  stable/10/lib/libiconv_modules/VIQR/citrus_viqr.c
Directory Properties:
  stable/10/   (props changed)

Modified: stable/10/lib/libc/iconv/citrus_prop.c
==============================================================================
--- stable/10/lib/libc/iconv/citrus_prop.c	Tue Mar  4 10:47:35 2014	(r262730)
+++ stable/10/lib/libc/iconv/citrus_prop.c	Tue Mar  4 11:43:01 2014	(r262731)
@@ -339,7 +339,7 @@ name_found:
 
 static int
 _citrus_prop_parse_element(struct _memstream * __restrict ms,
-    const _citrus_prop_hint_t * __restrict hints, void ** __restrict context)
+    const _citrus_prop_hint_t * __restrict hints, void * __restrict context)
 {
 	int ch, errnum;
 #define _CITRUS_PROP_HINT_NAME_LEN_MAX	255
@@ -435,8 +435,7 @@ _citrus_prop_parse_variable(const _citru
 		if (ch == EOF || ch == '\0')
 			break;
 		_memstream_ungetc(&ms, ch);
-		errnum = _citrus_prop_parse_element(
-		    &ms, hints, (void ** __restrict)context);
+		errnum = _citrus_prop_parse_element(&ms, hints, context);
 		if (errnum != 0)
 			return (errnum);
 	}

Modified: stable/10/lib/libc/iconv/citrus_prop.h
==============================================================================
--- stable/10/lib/libc/iconv/citrus_prop.h	Tue Mar  4 10:47:35 2014	(r262730)
+++ stable/10/lib/libc/iconv/citrus_prop.h	Tue Mar  4 11:43:01 2014	(r262731)
@@ -42,7 +42,7 @@ typedef struct _citrus_prop_hint_t _citr
 
 #define _CITRUS_PROP_CB0_T(_func_, _type_) \
 typedef int (*_citrus_prop_##_func_##_cb_func_t) \
-    (void ** __restrict, const char *, _type_); \
+    (void * __restrict, const char *, _type_); \
 typedef struct { \
 	_citrus_prop_##_func_##_cb_func_t func; \
 } _citrus_prop_##_func_##_cb_t;
@@ -52,7 +52,7 @@ _CITRUS_PROP_CB0_T(str, const char *)
 
 #define _CITRUS_PROP_CB1_T(_func_, _type_) \
 typedef int (*_citrus_prop_##_func_##_cb_func_t) \
-    (void ** __restrict, const char *, _type_, _type_); \
+    (void * __restrict, const char *, _type_, _type_); \
 typedef struct { \
 	_citrus_prop_##_func_##_cb_func_t func; \
 } _citrus_prop_##_func_##_cb_t;

Modified: stable/10/lib/libiconv_modules/BIG5/citrus_big5.c
==============================================================================
--- stable/10/lib/libiconv_modules/BIG5/citrus_big5.c	Tue Mar  4 10:47:35 2014	(r262730)
+++ stable/10/lib/libiconv_modules/BIG5/citrus_big5.c	Tue Mar  4 11:43:01 2014	(r262731)
@@ -172,7 +172,7 @@ _citrus_BIG5_check_excludes(_BIG5Encodin
 }
 
 static int
-_citrus_BIG5_fill_rowcol(void ** __restrict ctx, const char * __restrict s,
+_citrus_BIG5_fill_rowcol(void * __restrict ctx, const char * __restrict s,
     uint64_t start, uint64_t end)
 {
 	_BIG5EncodingInfo *ei;
@@ -191,7 +191,7 @@ _citrus_BIG5_fill_rowcol(void ** __restr
 
 static int
 /*ARGSUSED*/
-_citrus_BIG5_fill_excludes(void ** __restrict ctx,
+_citrus_BIG5_fill_excludes(void * __restrict ctx,
     const char * __restrict s __unused, uint64_t start, uint64_t end)
 {
 	_BIG5EncodingInfo *ei;
@@ -237,7 +237,6 @@ static int
 _citrus_BIG5_encoding_module_init(_BIG5EncodingInfo * __restrict ei,
     const void * __restrict var, size_t lenvar)
 {
-	void *ctx = (void *)ei;
 	const char *s;
 	int err;
 
@@ -259,9 +258,9 @@ _citrus_BIG5_encoding_module_init(_BIG5E
 	}
 
 	/* fallback Big5-1984, for backward compatibility. */
-	_citrus_BIG5_fill_rowcol((void **)&ctx, "row", 0xA1, 0xFE);
-	_citrus_BIG5_fill_rowcol((void **)&ctx, "col", 0x40, 0x7E);
-	_citrus_BIG5_fill_rowcol((void **)&ctx, "col", 0xA1, 0xFE);
+	_citrus_BIG5_fill_rowcol(ei, "row", 0xA1, 0xFE);
+	_citrus_BIG5_fill_rowcol(ei, "col", 0x40, 0x7E);
+	_citrus_BIG5_fill_rowcol(ei, "col", 0xA1, 0xFE);
 
 	return (0);
 }

Modified: stable/10/lib/libiconv_modules/HZ/citrus_hz.c
==============================================================================
--- stable/10/lib/libiconv_modules/HZ/citrus_hz.c	Tue Mar  4 10:47:35 2014	(r262730)
+++ stable/10/lib/libiconv_modules/HZ/citrus_hz.c	Tue Mar  4 11:43:01 2014	(r262731)
@@ -65,8 +65,8 @@ typedef enum {
 } charset_t;
 
 typedef struct {
-	int	 end;
 	int	 start;
+	int	 end;
 	int	 width;
 } range_t;
 
@@ -505,12 +505,12 @@ _citrus_HZ_encoding_module_uninit(_HZEnc
 }
 
 static int
-_citrus_HZ_parse_char(void **context, const char *name __unused, const char *s)
+_citrus_HZ_parse_char(void *context, const char *name __unused, const char *s)
 {
 	escape_t *escape;
 	void **p;
 
-	p = (void **)*context;
+	p = (void **)context;
 	escape = (escape_t *)p[0];
 	if (escape->ch != '\0')
 		return (EINVAL);
@@ -522,14 +522,14 @@ _citrus_HZ_parse_char(void **context, co
 }
 
 static int
-_citrus_HZ_parse_graphic(void **context, const char *name, const char *s)
+_citrus_HZ_parse_graphic(void *context, const char *name, const char *s)
 {
 	_HZEncodingInfo *ei;
 	escape_t *escape;
 	graphic_t *graphic;
 	void **p;
 
-	p = (void **)*context;
+	p = (void **)context;
 	escape = (escape_t *)p[0];
 	ei = (_HZEncodingInfo *)p[1];
 	graphic = malloc(sizeof(*graphic));
@@ -591,13 +591,13 @@ _CITRUS_PROP_HINT_END
 };
 
 static int
-_citrus_HZ_parse_escape(void **context, const char *name, const char *s)
+_citrus_HZ_parse_escape(void *context, const char *name, const char *s)
 {
 	_HZEncodingInfo *ei;
 	escape_t *escape;
 	void *p[2];
 
-	ei = (_HZEncodingInfo *)*context;
+	ei = (_HZEncodingInfo *)context;
 	escape = malloc(sizeof(*escape));
 	if (escape == NULL)
 		return (EINVAL);

Modified: stable/10/lib/libiconv_modules/VIQR/citrus_viqr.c
==============================================================================
--- stable/10/lib/libiconv_modules/VIQR/citrus_viqr.c	Tue Mar  4 10:47:35 2014	(r262730)
+++ stable/10/lib/libiconv_modules/VIQR/citrus_viqr.c	Tue Mar  4 11:43:01 2014	(r262731)
@@ -433,7 +433,6 @@ static int
 _citrus_VIQR_encoding_module_init(_VIQREncodingInfo * __restrict ei,
     const void * __restrict var __unused, size_t lenvar __unused)
 {
-	const mnemonic_def_t *p;
 	const char *s;
 	size_t i, n;
 	int errnum;
@@ -457,7 +456,10 @@ _citrus_VIQR_encoding_module_init(_VIQRE
 			return (errnum);
 		}
 	}
-	for (i = 0;; ++i) {
+	/* a + 1 < b + 1 here to silence gcc warning about unsigned < 0. */
+	for (i = 0; i + 1 < mnemonic_ext_size + 1; ++i) {
+		const mnemonic_def_t *p;
+
 		p = &mnemonic_ext[i];
 		n = strlen(p->name);
 		if (ei->mb_cur_max < n)
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 9 Tijl Coosemans freebsd_committer freebsd_triage 2014-03-04 11:43:50 UTC 
State Changed
From-To: patched->closed

Merged to stable/10 in r262731.
Comment 10 Delta Regeer 2014-06-16 04:01:25 UTC 
I understand this has already been resolved and MFC'ed back to 10-STABLE, this however is causing any program compiled against the base system libiconv to segfault. In my case this is a DoS that is causing email to not be delivered in time.

For me specifically that means that certain emails to users are causing my Dovecot processes to crash in libiconv.

Is there any way to make this an errata that gets a -pX for 10.0-RELEASE? Or is there some way to have all the items in the ports tree built against converters/libiconv instead of against the base system libiconv if USES=iconv is set.
Comment 11 Tijl Coosemans freebsd_committer freebsd_triage 2014-06-17 16:28:06 UTC 
(In reply to xistence from comment #10)
> I understand this has already been resolved and MFC'ed back to 10-STABLE,
> this however is causing any program compiled against the base system
> libiconv to segfault. In my case this is a DoS that is causing email to not
> be delivered in time.
> 
> For me specifically that means that certain emails to users are causing my
> Dovecot processes to crash in libiconv.
> 
> Is there any way to make this an errata that gets a -pX for 10.0-RELEASE? Or
> is there some way to have all the items in the ports tree built against
> converters/libiconv instead of against the base system libiconv if
> USES=iconv is set.

As a workaround you can modify the Dovecot port Makefile.  Look for a line starting with "USES=" and replace "iconv" with "iconv:translit" there.  That should make it compile against converters/libiconv.