This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: windres bug with MENUEX resource.


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
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;

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]