samba - Jun 2006 - iconv.c / static charset prototype/assembler bug?

Hi!

I think there is a little bug in the current samba release (3.0.22).

Take a look at cp850.c at the last line and you will see following 
definition:

        SMB_GENERATE_CHARSET_MODULE_8_BIT_GAP(CP850)

Using the macros from charset.c, the preprocessor expands this to:

        NTSTATUS charset_CP850_init(void) \
        {       \
                return smb_register_charset(&CP850_functions);          \
        }       \

NTSTATUS is defined in nt_status.h as follows:
 
        typedef struct {uint32 v;} NTSTATUS;
        #define NT_STATUS(x) ((NTSTATUS) { x })
        #define NT_STATUS_V(x) ((x).v)


Ok, when cp850/cp437 is compiled as static, config.h has following macro:
        #define static_init_charset { charset_CP850_init(); 
charset_CP437_init();} 

This static_init_charset macro gets called in iconv.c. But at this point 
in the file there is no function prototype for charset_CP850_init and 
charset_CP437_init. Thus gcc, doesn't know that it has to reserve space on 
the stack for this 4 byte return value which actually gets returned AND 
pushed to the stack by charset_CP850_init.

After adding following prototype to config.h, everything works as 
expected:
NT_STATUS  charset_CP850_init(void);


You may look at this disassembly which shows whats going wrong exactely:

iconv.c: smb_iconv_open(): (With the NT_STATUS  charset_CP850_init(void); 
prototype:)

 807c173:       85 c0                   test   %eax,%eax
 807c175:       75 e9                   jne    807c160 
<smb_iconv_open+0x3a>
 807c177:       8d 45 ec                lea    0xffffffec(%ebp),%eax
 807c17a:       83 ec 0c                sub    $0xc,%esp
 807c17d:       50                      push   %eax // here gcc reserves 
space on the stack
 807c17e:       e8 b9 96 00 00          call   808583c 
<charset_CP850_init>
 807c183:       83 c4 0c                add    $0xc,%esp
 807c186:       e8 7d 98 00 00          call   8085a08 
<charset_CP437_init> 

iconv.c: smb_iconv_open(): (Without the NT_STATUS 
charset_CP850_init(void); prototype:)

 807c173:       85 c0                   test   %eax,%eax
 807c175:       75 e9                   jne    807c160 
<smb_iconv_open+0x3a>
 807c177:       e8 b8 96 00 00          call   8085834 
<charset_CP850_init>    // no space reserved
 807c17c:       e8 7f 98 00 00          call   8085a00 
<charset_CP437_init> 

cp850.c : charset_CP850_init
 808583c:       55                      push   %ebp
 808583d:       89 e5                   mov    %esp,%ebp
 808583f:       56                      push   %esi
 8085840:       53                      push   %ebx
 8085841:       83 ec 18                sub    $0x18,%esp
 8085844:       e8 00 00 00 00          call   8085849 
<charset_CP850_init+0xd>
 8085849:       5b                      pop    %ebx
 808584a:       81 c3 8f 1a 03 00       add    $0x31a8f,%ebx
 8085850:       8b 75 08                mov    0x8(%ebp),%esi // read 
NT_STATUS
 8085853:       8d 55 f4                lea    0xfffffff4(%ebp),%edx
 8085856:       8d 83 ac 60 00 00       lea    0x60ac(%ebx),%eax
 808585c:       50                      push   %eax
 808585d:       52                      push   %edx
 808585e:       e8 73 67 ff ff          call   807bfd6 
<smb_register_charset>
 8085863:       8b 45 f4                mov    0xfffffff4(%ebp),%eax
 8085866:       89 06                   mov    %eax,(%esi) // write 
NT_STATUS
 8085868:       89 f0                   mov    %esi,%eax
 808586a:       8d 65 f8                lea    0xfffffff8(%ebp),%esp
 808586d:       5b                      pop    %ebx
 808586e:       5e                      pop    %esi
 808586f:       5d                      pop    %ebp
 8085870:       c2 04 00                ret    $0x4
 8085873:       90                      nop 



As you can see,
 8085866:       89 06                   mov    %eax,(%esi) // write 
NT_STATUS
tries to write the error code to the address pushed on the stack before, 
which actually never happened when the prototype is missing.

Btw, this is compiled with GCC 4.1.1 / ELF / PIC

May I be wrong here? Do you need any additional information?

Thanks!
Robert!
http://www.skyos.org

On Tue, Jun 27, 2006 at 02:25:28PM +0200, Robert Szeleney
wrote:
> Hi!
> 
> I think there is a little bug in the current samba release (3.0.22).
> 
> Take a look at cp850.c at the last line and you will see following 
> definition:
> 
>         SMB_GENERATE_CHARSET_MODULE_8_BIT_GAP(CP850)
> 
> Using the macros from charset.c, the preprocessor expands this to:
> 
>         NTSTATUS charset_CP850_init(void) \
>         {       \
>                 return smb_register_charset(&CP850_functions);         
\
>         }       \
> 
> NTSTATUS is defined in nt_status.h as follows:
>  
>         typedef struct {uint32 v;} NTSTATUS;
>         #define NT_STATUS(x) ((NTSTATUS) { x })
>         #define NT_STATUS_V(x) ((x).v)
> 
> 
> Ok, when cp850/cp437 is compiled as static, config.h has following macro:
>         #define static_init_charset { charset_CP850_init(); 
> charset_CP437_init();} 
> 
> This static_init_charset macro gets called in iconv.c. But at this point 
> in the file there is no function prototype for charset_CP850_init and 
> charset_CP437_init. Thus gcc, doesn't know that it has to reserve space
on
> the stack for this 4 byte return value which actually gets returned AND 
> pushed to the stack by charset_CP850_init.
Ok, I've looked into this in the current SAMBA_3_0 codebase
(the code that's in 3.0.23RC3) and this problem shouldn't
happen.

When you configure with :

./configure.developer --with-static-modules=charset_CP850

You get :

/* Decl of Static init functions */
#define static_decl_charset extern NTSTATUS charset_CP850_init(void);

followed by :

#define static_init_charset {  charset_CP850_init();}

defined in include/config.h

in lib/iconv.c we have :

static_decl_charset;

at the top of the file (after #include "includes.h")
followed by :

static void lazy_initialize_iconv(void)
{
        static BOOL initialized;
        int i;

        if (!initialized) {
                initialized = True;
                for(i = 0; builtin_functions[i].name; i++)
                        smb_register_charset(&builtin_functions[i]);
                static_init_charset;
        }
}

- this should cause the correct declaration and definition to
be used. Follow up this email if you think I'm incorrect on
this but I don't think it's a problem in 3.0.23 RC3.

Jeremy.