windres bug with MENUEX resource.

windres bug with MENUEX resource.

[Stefan `Sec` Zehl]

Hi,

I believe I have found a bug in windres (appears to be part of the
binutils-20060817-1 package).

If I define POPUP Submenus of a MENUEX, the resulting resource is
corrupt.

I have constructed a small example to show the error:

| karoshi:~/demo>make
| windres -i res.rc -o res.o
| cc -g -O -Wall -mno-cygwin -o demo demo.c res.o 
| karoshi:~/demo>./demo.exe 
| LoadMenu failed with error 13: Die Daten sind unzulässig.

(sorry for the german error message, it is "The Data is invalid")

I have found an (old) mail with a patch for the very same problem. It
can be viewed at:

http://sources.redhat.com/ml/binutils/2004-06/msg00609.html

Unfortunately I don't know how to recompile parts of cygwin, so I can't
test that patch, but it would be really great if somebody could look at
this.

----------------------------------------------------------------------
Example resource file "res.rc":
#include <windows.h>

123 MENUEX
BEGIN
    POPUP "Dummy", 144
    BEGIN
	MENUITEM "&About...", 1
    END
END
----------------------------------------------------------------------
Example c program "demo.c":
#include <stdio.h>
#include <string.h>

#include <windows.h>

void ErrorExit(char *);

int APIENTRY WinMain(HINSTANCE hInst,
                     HINSTANCE hPrevInstance,
                     LPSTR     lpCmdLine,
                     int       nCmdShow){
HMENU hCtxMenu;

        hCtxMenu=LoadMenu (hInst, MAKEINTRESOURCE(123));
        if(hCtxMenu==NULL){
            ErrorExit("LoadMenu");
        };
        return 0;
}

void ErrorExit(char *text) { 
        TCHAR szBuf[80]; 
        LPVOID lpMsgBuf;
        DWORD dw = GetLastError(); 

        FormatMessage(  FORMAT_MESSAGE_ALLOCATE_BUFFER | 
                        FORMAT_MESSAGE_FROM_SYSTEM,
                        NULL,
                        dw,
                        MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
                        (LPTSTR) &lpMsgBuf,
                        0, NULL );
        wsprintf(szBuf, "%s failed with error %d: %s", text, dw, lpMsgBuf); 
        printf("%s",szBuf);
        LocalFree(lpMsgBuf);
        ExitProcess(dw); 
};
----------------------------------------------------------------------
Example Makefile "Makefile":
CFLAGS+=-g -O -Wall -mno-cygwin
LIBS=

PRG=demo

${PRG}: ${PRG}.c res.o
	${CC} ${CFLAGS} -o ${PRG} ${PRG}.c res.o ${LIBS}

res.o: res.rc
	windres -i $< -o $@
----------------------------------------------------------------------

Thanks,
    Sec

P.S.: if you remove the ",144" part from the POPUP statement, the
program will run fine. This is because windres then compiles it as
"MENU" instead of "MENUEX", which can be verified with "windres -i res.o"

P.P.S.: This is a resend from my posting to the cygwin mailinglist from
where I was pointed to the binutils mailinglist.

-- 
The most successful method of programming is to begin a program as
simply as possible, test it, and then add to the program until it
performs the required job."              -- PDP8 handbook, Pg 9-64

Re: windres bug with MENUEX resource.

[Brian Dessent]

[-- Attachment #1: Type: text/plain, Size: 2263 bytes --]

Stefan `Sec` Zehl wrote:

> I believe I have found a bug in windres (appears to be part of the
> binutils-20060817-1 package).
> 
> If I define POPUP Submenus of a MENUEX, the resulting resource is
> corrupt.
> 
> I have constructed a small example to show the error:
> 
> | karoshi:~/demo>make
> | windres -i res.rc -o res.o
> | cc -g -O -Wall -mno-cygwin -o demo demo.c res.o
> | karoshi:~/demo>./demo.exe
> | LoadMenu failed with error 13: Die Daten sind unzulässig.
> 
> (sorry for the german error message, it is "The Data is invalid")

Yes, I can confirm that windres from current CVS HEAD miscompiles this. 
If you run the resultant demo.exe through a third party resource
decompiler, you get the following, which is clearly bogus:

123 MENUEX
LANGUAGE LANG_ENGLISH, SUBLANG_ENGLISH_US
{
MENUITEM "ummy", 144, MFT_STRING, MFS_ENABLED
POPUP "?&About...", 0, MFT_STRING, MFS_ENABLED, 0
{
}
}

> I have found an (old) mail with a patch for the very same problem. It
> can be viewed at:
> 
> http://sources.redhat.com/ml/binutils/2004-06/msg00609.html
> 
> Unfortunately I don't know how to recompile parts of cygwin, so I can't
> test that patch, but it would be really great if somebody could look at
> this.

As pointed out on the other list, binutils is its own project, it's not
part of Cygwin (your email to binutils <at> cygwin.com happend to work
by accident because cygwin.com is the same physical machine as
sourceware.org, host of many projects.)  It is a standard GNU-style
software package, buildable by "./configure && make".

I can confirm that the patch (rediffed to current CVS attached) does in
fact fix the problem at hand, verified both by running the testcase and
by correct output of decompiling the resource.

However, it seems that it is a necessary but not sufficient fix for the
bug, because windres now cannot read its own output:

$ /build/combined/binutils/windres -i res.o
/build/combined/binutils/windres: null terminated unicode string: not
enough binary data

So there's probably some remaining code somewhere using 12 instead of 14
for the length of MENUEX.  Sigh.  Littered with dozens of of these bare,
uncommented magic values, this code is really a mess.

Brian

[-- Attachment #2: windres_menuex.patch --]
[-- Type: text/plain, Size: 1975 bytes --]

Index: resbin.c
===================================================================
RCS file: /cvs/src/src/binutils/resbin.c,v
retrieving revision 1.10
diff -u -p -r1.10 resbin.c
--- resbin.c	29 Mar 2006 00:24:28 -0000	1.10
+++ resbin.c	20 Apr 2007 17:58:51 -0000
@@ -380,7 +380,7 @@ bin_to_res_menuexitems (const unsigned c
       unsigned int itemlen;
       struct menuitem *mi;
 
-      if (length < 14)
+      if (length < 16)
 	toosmall (_("menuitem header"));
 
       mi = (struct menuitem *) res_alloc (sizeof *mi);
@@ -388,17 +388,17 @@ bin_to_res_menuexitems (const unsigned c
       mi->state = get_32 (big_endian, data + 4);
       mi->id = get_16 (big_endian, data + 8);
 
-      flags = get_16 (big_endian, data + 10);
+      flags = get_16 (big_endian, data + 12);
 
-      if (get_16 (big_endian, data + 12) == 0)
+      if (get_16 (big_endian, data + 14) == 0)
 	{
 	  slen = 0;
 	  mi->text = NULL;
 	}
       else
-	mi->text = get_unicode (data + 12, length - 12, big_endian, &slen);
+	mi->text = get_unicode (data + 12, length - 14, big_endian, &slen);
 
-      itemlen = 12 + slen * 2 + 2;
+      itemlen = 14 + slen * 2 + 2;
       itemlen = (itemlen + 3) &~ 3;
 
       if ((flags & 1) == 0)
@@ -1874,21 +1874,21 @@ res_to_bin_menuexitems (const struct men
       dword_align_bin (&pp, &length);
 
       d = (struct bindata *) reswr_alloc (sizeof *d);
-      d->length = 12;
+      d->length = 14;
       d->data = (unsigned char *) reswr_alloc (d->length);
 
-      length += 12;
+      length += 14;
 
       put_32 (big_endian, mi->type, d->data);
       put_32 (big_endian, mi->state, d->data + 4);
-      put_16 (big_endian, mi->id, d->data + 8);
+      put_16 (big_endian, mi->id, d->data + 10);
 
       flags = 0;
       if (mi->next == NULL)
 	flags |= 0x80;
       if (mi->popup != NULL)
 	flags |= 1;
-      put_16 (big_endian, flags, d->data + 10);
+      put_16 (big_endian, flags, d->data + 12);
 
       *pp = d;
       pp = &d->next;

Re: windres bug with MENUEX resource.

[Kai Tietz]

[-- Attachment #1: Type: text/plain, Size: 4303 bytes --]

binutils-owner@sourceware.org wrote on 20.04.2007 17:26:40:

> Hi,
> 
> I believe I have found a bug in windres (appears to be part of the
> binutils-20060817-1 package).
> 
> If I define POPUP Submenus of a MENUEX, the resulting resource is
> corrupt.
> 
> I have constructed a small example to show the error:
> 
> | karoshi:~/demo>make
> | windres -i res.rc -o res.o
> | cc -g -O -Wall -mno-cygwin -o demo demo.c res.o 
> | karoshi:~/demo>./demo.exe 
> | LoadMenu failed with error 13: Die Daten sind unzulässig.
> 
> (sorry for the german error message, it is "The Data is invalid")
> 
> I have found an (old) mail with a patch for the very same problem. It
> can be viewed at:
> 
> http://sources.redhat.com/ml/binutils/2004-06/msg00609.html
> 
> Unfortunately I don't know how to recompile parts of cygwin, so I can't
> test that patch, but it would be really great if somebody could look at
> this.
> 
> ----------------------------------------------------------------------
> Example resource file "res.rc":
> #include <windows.h>
> 
> 123 MENUEX
> BEGIN
>     POPUP "Dummy", 144
>     BEGIN
>    MENUITEM "&About...", 1
>     END
> END
> ----------------------------------------------------------------------
> Example c program "demo.c":
> #include <stdio.h>
> #include <string.h>
> 
> #include <windows.h>
> 
> void ErrorExit(char *);
> 
> int APIENTRY WinMain(HINSTANCE hInst,
>                      HINSTANCE hPrevInstance,
>                      LPSTR     lpCmdLine,
>                      int       nCmdShow){
> HMENU hCtxMenu;
> 
>         hCtxMenu=LoadMenu (hInst, MAKEINTRESOURCE(123));
>         if(hCtxMenu==NULL){
>             ErrorExit("LoadMenu");
>         };
>         return 0;
> }
> 
> void ErrorExit(char *text) { 
>         TCHAR szBuf[80]; 
>         LPVOID lpMsgBuf;
>         DWORD dw = GetLastError(); 
> 
>         FormatMessage(  FORMAT_MESSAGE_ALLOCATE_BUFFER | 
>                         FORMAT_MESSAGE_FROM_SYSTEM,
>                         NULL,
>                         dw,
>                         MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
>                         (LPTSTR) &lpMsgBuf,
>                         0, NULL );
>         wsprintf(szBuf, "%s failed with error %d: %s", text, dw, 
lpMsgBuf); 
>         printf("%s",szBuf);
>         LocalFree(lpMsgBuf);
>         ExitProcess(dw); 
> };
> ----------------------------------------------------------------------
> Example Makefile "Makefile":
> CFLAGS+=-g -O -Wall -mno-cygwin
> LIBS=
> 
> PRG=demo
> 
> ${PRG}: ${PRG}.c res.o
>    ${CC} ${CFLAGS} -o ${PRG} ${PRG}.c res.o ${LIBS}
> 
> res.o: res.rc
>    windres -i $< -o $@
> ----------------------------------------------------------------------
> 
> Thanks,
>     Sec
> 
> P.S.: if you remove the ",144" part from the POPUP statement, the
> program will run fine. This is because windres then compiles it as
> "MENU" instead of "MENUEX", which can be verified with "windres -i 
res.o"
> 
> P.P.S.: This is a resend from my posting to the cygwin mailinglist from
> where I was pointed to the binutils mailinglist.

I agree, that the MENUEX resource is not proper.
AFAIC read in the MS specification of rc files, it seems to be that the 
"id" is a dword, as to see in the MENUITEM spec for the "result" field. 
Therefore I assume, that the following patch should be more correct, than 
to simply to pad the structure.

ChangeLog:

2007-04-23  Kai Tietz  <Kai.Tietz@onevision.com>

        * binutils/resbin.c: (bin_to_res_menuexitems,
        res_to_bin_menuexitems): Read id as 32-bit word.




Regards,
 i.A. Kai Tietz

------------------------------------------------------------------------------------------
  OneVision Software Entwicklungs GmbH & Co. KG
  Dr.-Leo-Ritter-Straße 9 - 93049 Regensburg
  Tel: +49.(0)941.78004.0 - Fax: +49.(0)941.78004.489 - www.OneVision.com
  Commerzbank Regensburg - BLZ 750 400 62 - Konto 6011050
  Handelsregister: HRA 6744, Amtsgericht Regensburg
  Komplementärin: OneVision Software Entwicklungs Verwaltungs GmbH
  Dr.-Leo-Ritter-Straße 9 – 93049 Regensburg
  Handelsregister: HRB 8932, Amtsgericht Regensburg - Geschäftsführer: 
Ulrike Döhler, Manuela Kluger


[-- Attachment #2: menuitemex_align.txt --]
[-- Type: text/plain, Size: 2016 bytes --]

Index: binutils/resbin.c
===================================================================
RCS file: /cvs/src/src/binutils/resbin.c,v
retrieving revision 1.10
diff -b -u -r1.10 resbin.c
--- binutils/resbin.c	29 Mar 2006 00:24:28 -0000	1.10
+++ binutils/resbin.c	23 Apr 2007 12:36:31 -0000
@@ -380,25 +380,25 @@
       unsigned int itemlen;
       struct menuitem *mi;
 
-      if (length < 14)
+      if (length < 16)
 	toosmall (_("menuitem header"));
 
       mi = (struct menuitem *) res_alloc (sizeof *mi);
       mi->type = get_32 (big_endian, data);
       mi->state = get_32 (big_endian, data + 4);
-      mi->id = get_16 (big_endian, data + 8);
+      mi->id = get_32 (big_endian, data + 8);
 
-      flags = get_16 (big_endian, data + 10);
+      flags = get_16 (big_endian, data + 12);
 
-      if (get_16 (big_endian, data + 12) == 0)
+      if (get_16 (big_endian, data + 14) == 0)
 	{
 	  slen = 0;
 	  mi->text = NULL;
 	}
       else
-	mi->text = get_unicode (data + 12, length - 12, big_endian, &slen);
+	mi->text = get_unicode (data + 14, length - 14, big_endian, &slen);
 
-      itemlen = 12 + slen * 2 + 2;
+      itemlen = 14 + slen * 2 + 2;
       itemlen = (itemlen + 3) &~ 3;
 
       if ((flags & 1) == 0)
@@ -1874,21 +1874,21 @@
       dword_align_bin (&pp, &length);
 
       d = (struct bindata *) reswr_alloc (sizeof *d);
-      d->length = 12;
+      d->length = 14;
       d->data = (unsigned char *) reswr_alloc (d->length);
 
-      length += 12;
+      length += 14;
 
       put_32 (big_endian, mi->type, d->data);
       put_32 (big_endian, mi->state, d->data + 4);
-      put_16 (big_endian, mi->id, d->data + 8);
+      put_32 (big_endian, mi->id, d->data + 8);
 
       flags = 0;
       if (mi->next == NULL)
 	flags |= 0x80;
       if (mi->popup != NULL)
 	flags |= 1;
-      put_16 (big_endian, flags, d->data + 10);
+      put_16 (big_endian, flags, d->data + 12);
 
       *pp = d;
       pp = &d->next;
=