November 2009 - Wine-devel - list.winehq.org

Wine programs - replicating bad design/bugs
by Jerome Leclanche 28 Nov '09

28 Nov '09

Ok, posting here to get a broader feedback on the issue. Recently, another bug* was closed about an enhancement to a wine program - a minor feature not present on Windows. This is an issue I've seen brought up before, and which I've brought up myself**. As far as the win32 api goes, I can totally agree on dysfunctions that need to be replicated. However I find this unnecessary for the wine programs (notepad, wordpad, regedit, any or more). The idea behind bug 20844 was to have a better integration with the desktop, where Shift-F3 is a common keybind for "Search previous...". The only problem with this kind of mindset is "where should we stop with new features?". IMHO, the issue behind the two bugs linked below is only a matter of better integration, better design. I can also understand how some developers' goal is to close bugs, sometimes disregarding the fact there is a problem to deal with before then. * http://bugs.winehq.org/show_bug.cgi?id=20844 ** http://bugs.winehq.org/show_bug.cgi?id=18622 J. Leclanche PS. In bug 18622, I'm not talking about a case-insensitive registry, but for the key being actually renamed.

2 2

Re: Corrections for signal handlers
by Markus Elfring 28 Nov '09

28 Nov '09

> And don't ask me to actually think about what I'm doing, I am still open for further discussions. > it's too hard to figure out how to print a string and a number without using printf(). This data display (e. g. for the input command and line number) is generally not a problem. But the situation is different in the context of a signal handler because of the limited set of async-signal-safe functions. Regards, Markus

1 0

Re: Corrections for signal handlers
by Markus Elfring 28 Nov '09

28 Nov '09

> Original printed more useful information. I am aware of the desire to get some data for easier program debugging conveniently. > Just dropping it in sake of "correctness" is wrong. I have got an opposite opinion. I am curious if more software developers would like to care for complete async-signal-safety. > You'll have to come up with something that prints the same This will not happen at the moment because of conflicting expectations. > or don't touch it. I assume that the involved implementation details will be reconsidered some day in the future. Regards, Markus

2 1

Re: Corrections for signal handlers
by Vitaliy Margolen 28 Nov '09

28 Nov '09

Markus Elfring wrote: > Hello, > > Some signal handler implementations are also affected by technical details that are described in an article by Justin Pincar. > https://www.securecoding.cert.org/confluence/display/seccode/SIG30-C.+Call+… > Your code looks even worse then your arguments for "correct" codding practices. > -static int watchdog; > +static sig_atomic_t watchdog; Don't do this, you've been told to leave it alone. > - fprintf( stderr, "wineserver crashed, please enable coredumps (ulimit -c unlimited) and restart.\n"); > + char const message[] = "wineserver crashed, please enable coredumps (ulimit -c unlimited) and restart.\n"; Should be "static const char" in that exact order. > + > + (void) write(STDERR_FILENO, message, sizeof(message) - 1); What's "(void)" doing here? Yes we do ignore returned states here and in 10000s or other places. Don't do that. > + (void) fsync(STDERR_FILENO); Why do you need sync here? > abort(); > --- a/tools/wmc/wmc.c Wine fix per program please. You'll have to split this into 3 separate patches. > static void segvhandler(int sig) > { > -<----->fprintf(stderr, "\n%s:%d: Oops, segment violation\n", input_name, line_number); > -<----->fflush(stdout); > -<----->fflush(stderr); > + char const message[] = "\nOops, segment violation\n"; Same as above - should be "static const char" in that exact order. > + > + (void) write(STDERR_FILENO, message, sizeof(message) - 1); > + (void) fsync(STDOUT_FILENO); > + (void) fsync(STDERR_FILENO); Remove bogus "(void)". > <----->abort(); > } Original printed more useful information. Just dropping it in sake of "correctness" is wrong. You'll have to come up with something that prints the same or don't touch it. Original used tabs (8-spaces) your patch uses 4-space indentation. Either keep the file's formatting or stick to 4-space indentation but DO NOT mix and match both inside one function. Same applies to tools/wrc/wrc.c Vitaliy.

1 0

re: Corrections for signal handlers
by Dan Kegel 28 Nov '09

28 Nov '09

I'm not sure this change does what you think: - fflush(stdout); ... + (void) fsync(STDOUT_FILENO); The real problem with your change is that *we don't care* whether the output of wineserver is garbled, because it is low priority debugging information, and I've never seen it garbled in practice. In short, it's not worth the time reviewing the patch. So while we appreciate your effort to improve the correctness of Wine's code, we would prefer it if you chose a more useful target. Also, Wine's patch review system is reputation-based. Your best avenue for getting pedantic corrections accepted is to get some pragmatic improvements accepted first. You might then be able to use some of your earned karma points on your favorite bikeshed problem. - Dan

1 0

Whitespace cleanups
by Nate Gallaher 28 Nov '09

28 Nov '09

I'm looking at working on a new area of wine (for me) and the file I'm going to be working with is rife with whitespace issues. Literal tabs are sprinkled around and the 80-col limit has been broken badly and often in easily fixable ways. I'm wondering what the acceptable approach is to these issues. I'd like to fix these issues before I start submitting functional patches. My question is this: Is it acceptable to fix the whole file in one patch (80-cols), or should I just fix the one function I'm planning on touching? Or should I grit my teeth and ignore the style issues entirely? Thanks, ~Nate

6 9

dlls/kernel32/path.c: Fix return values for UNC pathnames in GetLongPathNameW
by Alexandre Hardy 28 Nov '09

28 Nov '09

The MSDN: http://msdn.microsoft.com/en-us/library/aa364980(VS.85).aspx states that GetLongPathNameW returns the length of the string required to hold the pathname if the buffer is too small to hold the path. In the case where the buffer is too small, the filename should not be stored in the buffer. This ensures that the buffer is not written to (for UNC pathnames) if the buffer is not large enought to store the path. Kind regards Alexandre -- -------------------------------------------------- Alexandre Hardy http://www.ahardy.za.net

2 1

Commit 05c788ac6f464b75c2a55c847a91d3a79261ca8e causes tooltip functions in e-Sword to stop functioning
by James McKenzie 27 Nov '09

27 Nov '09

All Commit 05c788ac6f464b75c2a55c847a91d3a79261ca8e richedit: Used ITextHost methods to avoid using window handle directly caused the tooltips popups to stop functioning in e-Sword. Has this been corrected in later Wine releases? I am at wine-1.1.14 with integrating the EM_FORMATRANGE patch. Thank you. James McKenzie

4 4

Re: [4/4] ntdll: partially support HEAP_DISABLE_COALESCE_ON_FREE, allows us to detect use-after-free
by Dan Kegel 27 Nov '09

27 Nov '09

Don't apply this one yet, just found a problem with it. On Tue, Nov 24, 2009 at 6:20 PM, Dan Kegel <dank(a)kegel.com> wrote: > This adds something like support for > HEAP_DISABLE_COALESCE_ON_FREE. > If the GlobalFlags registry entry has the > 200000 bit set, freed blocks are set > aside for a while rather than immediately > being released for reuse. This allows > us to poison them and check for use-after-free > or double-free errors, and report them as heap corruption. > > If you're running Valgrind, you get an immediate error > and stack trace for any of these heap corruption events, > which was the motivation for the patch series. > But the patches are useful even without valgrind. > > The number of blocks kept out of circulation is > 5000 by default, but can be set via an environment > variable. It ought to have a limit on number of bytes, > too, but this seemed enough for the moment. >

1 0

New valgrind results, this time with overrun checks and lots of new errors.
by Dan Kegel 27 Nov '09

27 Nov '09

http://kegel.com/wine/valgrind/logs/2009-11-18-21.51/ is the first full run with the heap tail check enabled. Here are the first few new problems it found. Somehow, it found a bunch of invalid reads in http://kegel.com/wine/valgrind/logs/2009-11-18-21.51/vg-advapi32_crypt.txt all in a function called test_incorrect_api_usage(). Offhand that seems like something to suppress. The invalid write I already posted about is in http://kegel.com/wine/valgrind/logs/2009-11-18-21.51/diff-advapi32_lsa.txt http://kegel.com/wine/valgrind/logs/2009-11-18-21.51/vg-advapi32_lsa.txt http://kegel.com/wine/valgrind/logs/2009-11-18-21.51/diff-advpack_files.txt http://kegel.com/wine/valgrind/logs/2009-11-18-21.51/vg-advpack_files.txt show an off-by-one buffer size issue (forgot to include space for nul char) in advpack (fix should be easy, anybody can grab this): Invalid write of size 2 at strcpyW (unicode.h:224) by lstrcpyW (string.c:104) by AdvInstallFileW (files.c:283) by AdvInstallFileA (files.c:216) by test_AdvInstallFile (files.c:513) Address 0x7f03d12e is 0 bytes after a block of size 30 alloc'd at notify_alloc (heap.c:279) by RtlAllocateHeap (heap.c:1521) by AdvInstallFileW (files.c:282) by AdvInstallFileA (files.c:216) by test_AdvInstallFile (files.c:513) http://kegel.com/wine/valgrind/logs/2009-11-18-21.51/diff-comctl32_tab.txt http://kegel.com/wine/valgrind/logs/2009-11-18-21.51/vg-comctl32_tab.txt show a more inscrutable error: Invalid write of size 4 at TAB_SetCurSel (tab.c:255) by TAB_WindowProc (tab.c:3367) by ??? (library.h:159) by call_window_proc (winproc.c:469) by WINPROC_CallProcAtoW (winproc.c:1023) by CallWindowProcA (winproc.c:2299) by tabSubclassProcess (tab.c:404) by ??? (library.h:159) by call_window_proc (winproc.c:469) by WINPROC_call_window (winproc.c:2223) by call_window_proc (message.c:1635) by send_message (message.c:2482) by SendMessageA (message.c:2627) by test_getters_setters (tab.c:752) by func_tab (tab.c:1174) by run_test (test.h:535) by main (test.h:585) Address 0x7f044098 is not stack'd, malloc'd or (recently) free'd Nikolay touched that code back in March, perhaps he should have a look. etc. etc. Lots of triage to do. It'd be nice if somebody could volunteer to go through these, file bugs, and assign provision blame using 'git blame'... - Dan

3 7