December 2009 - Wine-Devel - list.winehq.org

Re: Introducing WineTestBot

by Jeremy White

>> Let me be the first to thank Ge for this awesome piece of work. > > Seconded! It's really quite slick, for anyone that hasn't used it. It's a very nice way to quickly feel comfortable that the test you just wrote actually works in more places than just your mind. Thanks again! Cheers, Jeremy

Dec. 1, 2009

Re: [PATCH] mshtml: check for NULL return from allocations

by Jacek Caban

Hi Marcus, Marcus Meissner wrote: > Hi, > > Coverity thinks we miss NULL checks and yes, we do. > Added some. > > (...) > > - wstrbuf_init(&buf); > + if (!wstrbuf_init(&buf)) > + return E_OUTOFMEMORY; > range_to_string(This, &buf); > - if(buf.buf) > - *p = SysAllocString(buf.buf); > + *p = SysAllocString(buf.buf); > wstrbuf_finish(&buf); > > TRACE("ret %s\n", debugstr_w(*p)); > + if (!*p) > + return E_OUTOFMEMORY; > return S_OK; > } This check is not meant to check allocation failure, but to check if range_to_string set it to NULL. We even have a tests for this. More error handling is a good thing to do, but this check should remain unchanged. Thanks, Jacek

Dec. 1, 2009

Silencing RtlNtStatusToDosErrorNoTeb() a bit

by Wolfram Sang

Hi, while debugging an application (Project One, available at [1]), I noticed that wine (today's git) throws a lot of these fixmes: fixme:ntdll:RtlNtStatusToDosErrorNoTeb no mapping for c0000109 Looking around, I found this page[2] stating that ERROR_MR_MID_NOT_FOUND is an expected return value for c0000109, so that the fixme is not needed here. I updated the if clause accordingly: diff --git a/dlls/ntdll/error.c b/dlls/ntdll/error.c index 9469f6f..e125163 100644 --- a/dlls/ntdll/error.c +++ b/dlls/ntdll/error.c @@ -68,7 +68,9 @@ ULONG WINAPI RtlNtStatusToDosErrorNoTeb( NTSTATUS status ) if (status < table->end) { DWORD ret = table->table[status - table->start]; - if (ret == ERROR_MR_MID_NOT_FOUND) FIXME( "no mapping for %08x\n", status ); + if (ret == ERROR_MR_MID_NOT_FOUND && + status != STATUS_MESSAGE_NOT_FOUND) + FIXME( "no mapping for %08x\n", status ); return ret; } table++; This solution has the advantage of being not very intrusive, but the disadvantage that it might be not nicely maintainable. Another &&-block has to be added for each other status found validly returning ERROR_MR_MID_NOT_FOUND. Another solution might be to define, let's say, ERROR_WINE_FIXME for all real fixmes and convert them to ERROR_MR_MID_NOT_FOUND. Something like (just a draft): #define ERROR_WINE_FIXME 0xffffffff DWORD ret = table->table[status - table->start]; if (ret == ERROR_WINE_FIXME) { FIXME( "no mapping for %08x\n", status ); ret = ERROR_MR_MID_NOT_FOUND; } return ret; [and lots of table entries changed] Well, this is pretty intrusive, but may be better distinguishing the different cases perhaps. Then again, this additional #define is polluting the ERROR_-namespace. I think I prefer the first version for now, perhaps there is no second status validly returning ERROR_MR_MID_NOT_FOUND? Any advice on how I should proceed? Thanks in advance, Wolfram [1] ftp://ftp.scs-trc.net/pub/c64/Tools/Graphics/Project1_V0.5.zip [2] http://support.microsoft.com/kb/113996

Dec. 1, 2009

Re: [2/4] ntdll: add support for the HEAP_FREE_CHECKING_ENABLED style of heap overrun detection.

by Dan Kegel

On Tue, Dec 1, 2009 at 11:22 AM, Alexandre Julliard <julliard(a)winehq.org> wrote: > Validation should only happen when explicitly requested. Should I submit a separate patch to remove the existing small arena validation from RtlFreeHeap and RtlReAllocateHeap, then? Or is that how Windows XP behaved (validating small heap blocks but not large ones)?

Dec. 1, 2009

Re: [2/4] ntdll: add support for the HEAP_FREE_CHECKING_ENABLED style of heap overrun detection.

by Dan Kegel

On Tue, Dec 1, 2009 at 11:22 AM, Alexandre Julliard <julliard(a)winehq.org> wrote: > You should create your own heap with checking enabled so that it can be > tested in all cases. I couldn't figure out how to do that on Windows Vista, but I can sure do it at least for Wine.

Dec. 1, 2009

Re: [PATCH 2/2] jscript: Added lastIndex tests.

by Jacek Caban

Paul Vriens wrote: > On 12/01/2009 08:11 PM, Jacek Caban wrote: >> Paul Vriens wrote: >>> On 11/30/2009 09:12 PM, Jacek Caban wrote: >>>> --- >>>> dlls/jscript/tests/regexp.js | 66 >>>> ++++++++++++++++++++++++++++++++++++++++++ >>>> 1 files changed, 66 insertions(+), 0 deletions(-) >>>> >>>> >>> >>> Hi Jacek, >>> >>> This one introduces test failures on Vista+ : >>> >>> http://test.winehq.org/data/tests/jscript:run.html >>> >>> Could you have a look? >> >> That's strange, I can't reproduce it on my XP with IE8 nor IE7. The >> debug output is not enough to find, which exactly test fails. Could you >> please run it with attached patch? >> >> Thanks, >> Jacek >> > > Hi Jacek, > > I will, but ... > > Are you able to crosscompile the tests? If so you should maybe get an > account at https://winetestbot.geldorp.nl/index.pl. > Right, I forgot about it. I will give it a try. Thanks, Jacek

Dec. 1, 2009

Re: [PATCH 2/2] jscript: Added lastIndex tests.

by Paul Vriens

On 12/01/2009 08:11 PM, Jacek Caban wrote: > Paul Vriens wrote: >> On 11/30/2009 09:12 PM, Jacek Caban wrote: >>> --- >>> dlls/jscript/tests/regexp.js | 66 >>> ++++++++++++++++++++++++++++++++++++++++++ >>> 1 files changed, 66 insertions(+), 0 deletions(-) >>> >>> >> >> Hi Jacek, >> >> This one introduces test failures on Vista+ : >> >> http://test.winehq.org/data/tests/jscript:run.html >> >> Could you have a look? > > That's strange, I can't reproduce it on my XP with IE8 nor IE7. The > debug output is not enough to find, which exactly test fails. Could you > please run it with attached patch? > > Thanks, > Jacek > Hi Jacek, I will, but ... Are you able to crosscompile the tests? If so you should maybe get an account at https://winetestbot.geldorp.nl/index.pl. Here you find the results: https://winetestbot.geldorp.nl/JobDetails.pl?Key=26 -- Cheers, Paul.

Dec. 1, 2009

Re: [PATCH] kernel32: Respect the LANG environment variable on Mac OS.

by Gert van den Berg

On Tue, Dec 1, 2009 at 06:12, Ken Thomases <ken(a)codeweavers.com> wrote: > On Nov 30, 2009, at 9:34 PM, James McKenzie wrote: >> >> The idea behind most of the MacIntosh 'magic' is that the user is >> deliberately oblivous to this. > > I agree that Wine (well, anything) should meet the user's expectations without the user having to even be aware of their expectations. Unfortunately, "respecting" LANG does not lead to this... An important question is: Which environment should Wine emulate under OS X? Unix apps under OS X, Windows in the same locale as OS X or OS X-native apps? (I tend to prefer emulating OS X by default and allowing overrides using LC_ALL (in order to allow one override method on all platforms, which LANG is unsuitable for, since it is a last resort option)) In addition Wine should leave the registry settings alone if the settings on which they were based were left unchanged. This allows a user to fully customize Wine's "regional settings" without changing anything in the environment where Wine is run. >> I just ran the set command in a >> terminal.app session, of which I have several running right now, and it >> was set to en_US.utf8, which is what I expect because the 'American' >> flag is displayed in the top information bar (also called the 'Apple' >> bar.) Gert may want to run the set command in a terminal session and >> given what was said, the LANG setting should be en_ZA.utf8, which is >> what I expect for South African English. > I'm paste it in here soon... My Ubuntu settings is: mohag(a)mohagpc:~$ locale LANG=en_ZA.UTF-8 LANGUAGE= LC_CTYPE="en_ZA.UTF-8" LC_NUMERIC="en_ZA.UTF-8" LC_TIME="en_ZA.UTF-8" LC_COLLATE="en_ZA.UTF-8" LC_MONETARY="en_ZA.UTF-8" LC_MESSAGES="en_ZA.UTF-8" LC_PAPER="en_ZA.UTF-8" LC_NAME="en_ZA.UTF-8" LC_ADDRESS="en_ZA.UTF-8" LC_TELEPHONE="en_ZA.UTF-8" LC_MEASUREMENT="en_ZA.UTF-8" LC_IDENTIFICATION="en_ZA.UTF-8" LC_ALL= My OS X settings is: (Language set to "English", "Formats" set to "South Africa") MoHaGLaptop:~ mohag$ locale LANG= LC_COLLATE="C" LC_CTYPE="UTF-8" LC_MESSAGES="C" LC_MONETARY="C" LC_NUMERIC="C" LC_TIME="C" LC_ALL= I would interpret that as default language, C for numbers, currency, etc, which would not be correct. (I think a (Perl/Python if any of them support native locale under all OSes?) script to quickly test the behaviour under Wine / OS X / *nix might be handy to figure out what applications are actually seeing....) > But, more importantly for this discussion, the fact that LANG will be appropriate for you and probably Gert does not mean it will be appropriate "often enough" for Wine to rely on. As I have said, there are many ways in which users configure their language and formats settings that will not be properly reflected in LANG (and, indeed, cannot be). > > >> The Wine project could use this as a default with the Mac, which is not >> POSIX compliant. > > You are suggesting using a proxy for the real thing rather than the real thing. If we want to maintain the "magic", Wine should just use the settings from System Preferences directly (via CFLocale, CFNumberFormatter, and CFDateFormatter), with an optional way to override them for those rare users who want Wine's behavior to deviate from the system's. That makes sense to me... (If Wine tries to be a good OS X app under OS X, rather than a good POSIX app under OS X) (And what is suggested was current Wine settings of environment remained the same, LC_ALL (to allow for similar override everywhere), platform specific settings (e.g. individual settings from OS X's "Formats" settings), LC_* (which only contains specific settings, not "country" / "language"), LANG (for unset settings, and locale/country itself). On OS X this should mean that the variables other than LC_ALL only gets looked at if reading the OS X settings fail for some reason) For consistency with *nix override via LC_ALL makes sense (ignoring all other LC_*/LANG settings on OS X, in favour of OS X specific settings) > LANG doesn't help maintain the magic. It's a hindrance. > LC_ALL should be used to override settings, not LANG. LANG doesn't work (on Linux at least) if LC_ALL is defined. Telling users to use LANG if it only works in certain cases does not make sense... mohag(a)mohagpc:~$ LC_ALL="en_US.UTF-8" LANG="en_ZA.UTF-8" wine regedit /E - 'HKEY_CURRENT_USER\Control Panel\International'|egrep "Country|Locale" "iCountry"="1" "Locale"="00000409" "sCountry"="United States" mohag(a)mohagpc:~$ LC_ALL="en_ZA.UTF-8" LANG="en_US.UTF-8" wine regedit /E - 'HKEY_CURRENT_USER\Control Panel\International'|egrep "Country|Locale" "iCountry"="27" "Locale"="00001c09" "sCountry"="South Africa" I'm actually quite impressed with Wine's handling of the locale under Linux, it seem to be possible to overwrite the individual settings with LC_*, which I did not think was possible based on Vitality's comments on Wine-users. In addition it seems not to mess around with the settings if the environment stays the same. (Which is mostly the behaviour that I prefer, since it starts with defaults based on my settings (whether it is based on Windows' or Linux's idea of my settings is not easy to test, since I don't know of any obvious differences to test) and allows me to customize it under Wine to fix broken defaults / specific settings needed by applications) Under Linux it seems to emulate native apps quite nicely. I don't currently have an OS X Wine installed to test effect of different settings. The environment variables does seem to be affected by the language settings, which might be caused by OS X's decoupling of "Formats" settings and "Language" settings, which can result in some weird combinations. (e.g. Dutch language with "Formats" country set to "China" ) (Parts of this might be in need of some reordering, had internet issues and might have repeated myself) Gert

Dec. 1, 2009

Re: [2/4] ntdll: add support for the HEAP_FREE_CHECKING_ENABLED style of heap overrun detection.

by Alexandre Julliard

Dan Kegel <dank(a)kegel.com> writes: > @@ -85,6 +85,7 @@ typedef struct > > #define ARENA_INUSE_FILLER 0x55 > #define ARENA_FREE_FILLER 0xaa > +#define ARENA_PAD_FILLER(address) (((int)(address) & 1) ? 0xee : 0xfe) Don't cast addresses to int. > +static PVOID HEAP_ValidateTail( const HEAP *heap, const void *arena, const void *p, SIZE_T size, SIZE_T filler_size ) > +{ > + unsigned char *tail = ((unsigned char *)p) + size; > + int j=0; Please use correct types. j should be a SIZE_T, tail should be a const pointer. > @@ -1477,7 +1561,9 @@ BOOLEAN WINAPI RtlFreeHeap( HANDLE heap, ULONG flags, PVOID ptr ) > pInUse = (ARENA_INUSE *)ptr - 1; > if (!(subheap = HEAP_FindSubHeap( heapPtr, pInUse ))) > { > - if (!find_large_block( heapPtr, ptr )) goto error; > + ARENA_LARGE *arena = find_large_block( heapPtr, ptr ); > + if (!arena) goto error; > + if (!validate_large_arena( heapPtr, arena, NOISY )) goto error; Validation should only happen when explicitly requested. > diff --git a/dlls/ntdll/tests/heap.c b/dlls/ntdll/tests/heap.c > new file mode 100644 > index 0000000..7cfa554 > --- /dev/null > +++ b/dlls/ntdll/tests/heap.c The tests should go with the existing heap tests in kernel32. > +static void test_free_overrun(void) > +{ > + if ((pRtlGetNtGlobalFlags() & FLG_HEAP_ENABLE_FREE_CHECK) == 0) { > + skip("\ > +Skipping heap free padding overrun test. To enable:\n\ > +on Wine, use gflags to enable global heap free checking, or set the following key:\n\ > +[HKEY_LOCAL_MACHINE\\System\\CurrentControlSet\\Control\\Session Manager]\n\ > +\"GlobalFlag\"=dword:00000020\n\ > +On Windows, use gflags to enable heap free checking for ntdll_test.exe.\n"); > + return; You should create your own heap with checking enabled so that it can be tested in all cases. Also the patch is still too large, changes to sensitive areas like the heap code should be very small and focused. -- Alexandre Julliard julliard(a)winehq.org

Dec. 1, 2009

Re: [PATCH 2/2] jscript: Added lastIndex tests.

by Jacek Caban

Paul Vriens wrote: > On 11/30/2009 09:12 PM, Jacek Caban wrote: >> --- >> dlls/jscript/tests/regexp.js | 66 >> ++++++++++++++++++++++++++++++++++++++++++ >> 1 files changed, 66 insertions(+), 0 deletions(-) >> >> > > Hi Jacek, > > This one introduces test failures on Vista+ : > > http://test.winehq.org/data/tests/jscript:run.html > > Could you have a look? That's strange, I can't reproduce it on my XP with IE8 nor IE7. The debug output is not enough to find, which exactly test fails. Could you please run it with attached patch? Thanks, Jacek diff --git a/dlls/jscript/tests/regexp.js b/dlls/jscript/tests/regexp.js index bcc9aba..a1be281 100644 --- a/dlls/jscript/tests/regexp.js +++ b/dlls/jscript/tests/regexp.js @@ -302,7 +302,7 @@ re = /a/g; ok(re.lastIndex === 0, "re.lastIndex = " + re.lastIndex + " expected 0"); m = re.exec(" a "); -ok(re.lastIndex === 2, "re.lastIndex = " + re.lastIndex + " expected 2"); +ok(re.lastIndex === 2, "re.lastIndex = " + re.lastIndex + " expected 2 [1]"); ok(m.index === 1, "m.index = " + m.index + " expected 1"); m = re.exec(" a "); @@ -328,14 +328,14 @@ re.lastIndex = obj; ok(re.lastIndex === obj, "re.lastIndex = " + re.lastIndex + " expected obj"); li = 2; m = re.exec(" a a "); -ok(re.lastIndex === 2, "re.lastIndex = " + re.lastIndex + " expected 2"); +ok(re.lastIndex === 2, "re.lastIndex = " + re.lastIndex + " expected 2 [2]"); ok(m.index === 1, "m.index = " + m.index + " expected 1"); re.lastIndex = 3; re.lastIndex = "test"; ok(re.lastIndex === "test", "re.lastIndex = " + re.lastIndex + " expected 'test'"); m = re.exec(" a a "); -ok(re.lastIndex === 2, "re.lastIndex = " + re.lastIndex + " expected 2"); +ok(re.lastIndex === 2, "re.lastIndex = " + re.lastIndex + " expected 2 [3]"); ok(m.index === 1, "m.index = " + m.index + " expected 1"); re.lastIndex = 0; @@ -349,7 +349,7 @@ obj.valueOf = function() { throw 0; } re.lastIndex = obj; ok(re.lastIndex === obj, "unexpected re.lastIndex"); m = re.exec(" a a "); -ok(re.lastIndex === 2, "re.lastIndex = " + re.lastIndex + " expected 2"); +ok(re.lastIndex === 2, "re.lastIndex = " + re.lastIndex + " expected 2 [4]"); ok(m.index === 1, "m.index = " + m.index + " expected 1"); re.lastIndex = -3;

Dec. 1, 2009