Re: [Help-smalltalk] [PATCH] iconv: Handle iconv_open failures and substitute encoding names

[Paolo Bonzini]

Il 20/05/2013 19:36, Holger Hans Peter Freyther ha scritto:
> On AMD64/FreeBSD 9.1 the GNU libiconv library does not know about
> 'utf8' and iconv_open returned an invalid handle address. The code
> attempts to detect cases of the handle being equal to (iconv_t) -1
> but this didn't work on AMD64 systems. Add a C-Function to query
> the value of (iconv_t) -1 to make the code work for various pointer
> sizes.
> 
> Grease is using 'utf8' as codec name but GNU libiconv does not have
> support to match this to 'UTF-8'. Add some support code to replace
> the codec name before calling iconv_open.
> 
> Introduce a >>#isValid that checks the iconvHandle for isNil or
> the address for being an invalid handle. This should fix a potential
> crash when saving an invalid handle and calling release on it.
> 
> 2013-05-20  Holger Hans Peter Freyther  <address@hidden>
> 
>       * Sets.st: Add >>#isValid, >>#codecReplace: and >>#iconvInvalidHandle.
>       * iconv.c: Add iconvInvalid C-Function.
>       * iconvtests.st: Add test for 'utf8' replacement.
> ---
>  packages/iconv/ChangeLog     |    6 ++++++
>  packages/iconv/Sets.st       |   41 ++++++++++++++++++++++++++++++++++++-----
>  packages/iconv/iconv.c       |    8 +++++++-
>  packages/iconv/iconvtests.st |   14 +++++++++++++-
>  4 files changed, 62 insertions(+), 7 deletions(-)
> 
> diff --git a/packages/iconv/ChangeLog b/packages/iconv/ChangeLog
> index 2d74b98..4ede4da 100644
> --- a/packages/iconv/ChangeLog
> +++ b/packages/iconv/ChangeLog
> @@ -1,3 +1,9 @@
> +2013-05-20  Holger Hans Peter Freyther  <address@hidden>
> +
> +     * Sets.st: Add >>#isValid, >>#codecReplace: and >>#iconvInvalidHandle.
> +     * iconv.c: Add iconvInvalid C-Function.
> +     * iconvtests.st: Add test for 'utf8' replacement.
> +
>  2010-12-04  Paolo Bonzini  <address@hidden>
>  
>       * package.xml: Remove now superfluous <file> tags.
> diff --git a/packages/iconv/Sets.st b/packages/iconv/Sets.st
> index 30b28a7..f6a0d89 100644
> --- a/packages/iconv/Sets.st
> +++ b/packages/iconv/Sets.st
> @@ -7,7 +7,7 @@
>  
>  "======================================================================
>  |
> -| Copyright 2001, 2002, 2005, 2006, 2007, 2008 Free Software Foundation, Inc.
> +| Copyright 2001, 2002, 2005, 2006, 2007, 2008, 2013 Free Software 
> Foundation, Inc.
>  | Written by Paolo Bonzini.
>  |
>  | This file is part of the GNU Smalltalk class library.
> @@ -928,6 +928,19 @@ Iconv is skipped altogether and only Smalltalk 
> converters are used.'>
>           ifTrue: [self allInstancesDo: [:each | each release]]
>      ]
>  
> +    codecReplace: aName [
> +     "libiconv/glibc behave differently in terms of supporting
> +     UTF-8/utf8. This routine is patching it up."
> +     <category:'private'>
> +     ^aName = 'utf8'
> +         ifTrue: ['UTF-8'] ifFalse: [aName].
> +    ]
> +
> +    iconvInvalidHandle [
> +     <category: 'C call-outs'>
> +     <cCall: 'iconv_invalid' returning: #cObject args: #()>
> +    ]
> +
>      iconvOpen: to from: from [
>       <category: 'C call-outs'>
>       <cCall: 'iconv_open' returning: #cObject args: #(#string #string)>
> @@ -1000,6 +1013,16 @@ Iconv is skipped altogether and only Smalltalk 
> converters are used.'>
>       ^n
>      ]
>  
> +    isValid [
> +     <category: 'accessing'>
> +
> +     iconvHandle isNil
> +         ifTrue: [^false].
> +     iconvHandle address = self iconvInvalidHandle
> +         ifTrue: [^false].
> +     ^true.
> +    ]
> +
>      release [
>       <category: 'private - living across snapshots'>
>       self
> @@ -1009,17 +1032,25 @@ Iconv is skipped altogether and only Smalltalk 
> converters are used.'>
>  
>      finalize [
>       <category: 'private - living across snapshots'>
> -     iconvHandle isNil ifTrue: [^self].
> +     self isValid ifFalse: [
> +         iconvHandle := nil.
> +         ^self].
>       self iconvClose: iconvHandle.
>       iconvHandle := nil
>      ]
>  
>      iconvOpen [
> +     | convTo convFrom |
>       <category: 'private - living across snapshots'>
> +
> +     "Replace utf8 to UTF-8 or such"
> +     convTo := self codecReplace: to.
> +     convFrom := self codecReplace: from.
> +
>       iconvHandle isNil ifFalse: [self release].
> -     iconvHandle := self iconvOpen: to from: from.
> -     iconvHandle address = 4294967295 
> -         ifTrue: [^InvalidCharsetError signal: 
> +     iconvHandle := self iconvOpen: convTo from: convFrom.
> +     self isValid
> +         ifFalse: [^InvalidCharsetError signal:
>                       {from.
>                       to}].
>       self addToBeFinalized.
> diff --git a/packages/iconv/iconv.c b/packages/iconv/iconv.c
> index 80c97c6..ba2c38a 100644
> --- a/packages/iconv/iconv.c
> +++ b/packages/iconv/iconv.c
> @@ -7,7 +7,7 @@
>  
>  /***********************************************************************
>   *
> - * Copyright 2001, 2002, 2004, 2005, 2006 Free Software Foundation, Inc.
> + * Copyright 2001, 2002, 2004, 2005, 2006, 2013 Free Software Foundation, 
> Inc.
>   * Written by Paolo Bonzini.
>   *
>   * This file is part of GNU Smalltalk.
> @@ -92,10 +92,16 @@ iconvWrapper (iconv_t handle, OOP readBufferOOP, int 
> readPos,
>    return (save_errno != EILSEQ);
>  }
>  
> +iconv_t iconvInvalid(void)
> +{
> +  return (iconv_t) -1;
> +}
> +
>  void
>  gst_initModule (VMProxy * proxy)
>  {
>    vmProxy = proxy;
> +  vmProxy->defineCFunc ("iconv_invalid", iconvInvalid);
>    vmProxy->defineCFunc ("iconv_open", iconv_open);
>    vmProxy->defineCFunc ("iconv_close", iconv_close);
>    vmProxy->defineCFunc ("iconvWrapper", iconvWrapper);
> diff --git a/packages/iconv/iconvtests.st b/packages/iconv/iconvtests.st
> index 92fa2a3..90629c2 100644
> --- a/packages/iconv/iconvtests.st
> +++ b/packages/iconv/iconvtests.st
> @@ -7,7 +7,7 @@
>  
>  "======================================================================
>  |
> -| Copyright 2007 Free Software Foundation, Inc.
> +| Copyright 2007, 2013 Free Software Foundation, Inc.
>  | Written by Paolo Bonzini and Stephen Compall
>  |
>  | This file is part of GNU Smalltalk.
> @@ -243,5 +243,17 @@ TestCase subclass: IconvTest [
>       b := b copyFrom: 1 to: 1024.
>       self should: [ b asUnicodeString ] raise: IncompleteSequenceError.
>      ]
> +
> +    testCodecReplacement [
> +        | str unicode |
> +        "Grease and others use utf8 as codec name and GNU libiconv does not
> +        have support for the lookup. Check that it is working in both
> +        directions."
> +
> +        str := #[208 184].
> +        unicode := str asUnicodeString: 'utf8'.
> +        self assert: unicode first = $<16r0438>.
> +        self assert: (unicode asString: 'utf8') = str asString.
> +    ]
>  ]
>  
> 

Ok.

Paolo