flaky regression(?) - patch.py?id=1012510895681951425038278

Good day!

Something in this patch makes my app set This->bigBlockSize of its inbox
to zero when it gets a big batch of mail.  Of course, the app (and my
laola-based mailbox reader) is then unable to read the file.  AFAICT,
that is the only byte that is damaged, and it can be fixed after, but it
is annoying.

The app never did this before the patch, and hasn't since I reverted it
yesterday.

I will work on refining my observations, and I expect I will find and
fix the error eventually.  I'm just writing this in case it may be some
help to anyone.

Lawson

Good day!

I now have a crude model using a Winelib program so I can get the
problem under controlled - or at least, repeatable - conditions.

Now if I knew how to set a watchpoint from inside a Winelib program,
that might make it easy.  If a watchpoint on an upper halfword can
work.  A pox on the inventor of little-endianness!

If I have to use the debugger interface I know about, I will have to
refine the model a bit.

Lawson

Probable user head space error. - Dennis A. Moore

On Mon, 4 Feb 2002 [e-mail] wrote:
> If I have to use the debugger interface I know about, I will have to
> refine the model a bit.

putting DebugBreak() in the Winelib program seems to get me a recursive
access violation (c0000005).  Hmmm, should a Winelib program not use
malloc()?  What should it use instead (please not from a windose c
runtime dll).  Should it not use getpwuid()?  open()?  syslog()?
snprintf()?  Surely memcpy() is all right?

or is DebugBreak() wrong for a Winelib program?

Lawson

---Gone fission.---

________________________________________________________________
GET INTERNET ACCESS FROM JUNO!
Juno offers FREE or PREMIUM Internet access for less!
Join Juno today!  For your FREE software, visit:
http://dl.www.juno.com/get/web/.

[Francois Gouget]

On Tue, 5 Feb 2002 [e-mail] wrote:
> On Mon, 4 Feb 2002 [e-mail] wrote:
> > If I have to use the debugger interface I know about, I will have to
> > refine the model a bit.
>
> putting DebugBreak() in the Winelib program seems to get me a recursive
> access violation (c0000005).  Hmmm, should a Winelib program not use
> malloc()?  What should it use instead (please not from a windose c
> runtime dll).  Should it not use getpwuid()?  open()?  syslog()?
> snprintf()?  Surely memcpy() is all right?

A Winelib program should be able to use any of the above except
getpwuid() and syslog(). If they use those then they will not run on
Windows. But they can use them as long as they link with the Unix C
library...
> or is DebugBreak() wrong for a Winelib program?

DebugBreak() should work, at least on x86, it's just an asm("int3").
But most likely what happens is that there is a problem in the exception
handling.

--
Francois Gouget         [e-mail]        http://fgouget.free.fr/
                  Hiroshima '45 - Czernobyl '86 - Windows '95

On Tue, 5 Feb 2002, Francois Gouget wrote:
> A Winelib program should be able to use any of the above except
> getpwuid() and syslog(). If they use those then they will not run on
> Windows. But they can use them as long as they link with the Unix C
> library...

Thanks for that.
> > or is DebugBreak() wrong for a Winelib program?
>
> DebugBreak() should work, at least on x86, it's just an asm("int3").
> But most likely what happens is that there is a problem in the exception
> handling.

Well, one problem is that the winelib program was accustomed to run
with stdin assigned to a file or pipe.  That seems to confuse winedbg,
so I moved it for now to a different fd.  That give me one winedbg
prompt, but if I tell it to cont, it spews endless syntax errors.
I'll see if Eric's recent patches help, and/or running it on a
wineconsole instead of a linux virtual console or xterm.
> --
> Francois Gouget         [e-mail]        http://fgouget.free.fr/
>                   Hiroshima '45 - Czernobyl '86 - Windows '95

Thanks!  We'll get it eventually.

Lawson

On Tue, 5 Feb 2002 [e-mail] wrote:
> I'll see if Eric's recent patches help, and/or running it on a
> wineconsole instead of a linux virtual console or xterm.

The patches I was thinking of were already committed.  DebugBreak()
works fine when the Winelib program is run with wineconsole (as long as
stdin is a terminal, not a file or a pipe), but I got so lost in
ICOM_THIS that I couldn't lead winedbg to the variable I wanted it to
watch.  Fortunately, If I take on faith that comments do not cause
errors (even with ICOM), that patch doesn't actually _do_ very much,
and I hit it on the second guess.

This fixes the problem I reported.  Maybe we should make the filename
longer to accomodate the trailing \0, maybe because it is a fixed
length, the trailing \0 is gratuitous, but as it was, that stepped all
over bigBlockSizeBits, generally disrupted my app, and looks sort of
like an off by one error.  I'll attach a copy in case your mailer folds
it.

diff -ur was/dlls/ole32/storage32.c is/dlls/ole32/storage32.c
--- was/dlls/ole32/storage32.c	Mon Feb  4 17:31:22 2002
+++ is/dlls/ole32/storage32.c	Tue Feb  5 22:11:20 2002
@@ -5615,7 +5615,7 @@
   /* prepare the file name string given in lieu of the root property name */
   GetFullPathNameW(pwcsName, MAX_PATH, fullname, NULL);
   memcpy(newStorage->filename, fullname, PROPERTY_NAME_BUFFER_LEN);
-  newStorage->filename[PROPERTY_NAME_BUFFER_LEN] = '\0';
+  newStorage->filename[PROPERTY_NAME_BUFFER_LEN-1] = '\0';

/*
    * Get an "out" pointer for the caller.

Lawson

Installation is performed in the reverse order of removal.
                    - British Leyland shop manual

[Francois Gouget]

On Tue, 5 Feb 2002 [e-mail] wrote:
[...]
> This fixes the problem I reported.  Maybe we should make the filename
> longer to accomodate the trailing \0, maybe because it is a fixed
> length, the trailing \0 is gratuitous, but as it was, that stepped all
> over bigBlockSizeBits, generally disrupted my app, and looks sort of
> like an off by one error.  I'll attach a copy in case your mailer folds
> it.

I don't know what the code is supposed to do so I cannot comment much
on your patch. But it looks reasonable. But this file contains quite a
few other cases that are definitely not reasonable:

Context:
struct StorageImpl { ...
    WCHAR filename[PROPERTY_NAME_BUFFER_LEN];
.. };

* in StorageBaseImpl_RenameElement()
   StorageBaseImpl_CreateStream()
   StorageImpl_CreateStorage()

renamedProperty.sizeOfNameString =
      ( lstrlenW(pwcsNewName)+1 ) * sizeof(WCHAR);

if (renamedProperty.sizeOfNameString > PROPERTY_NAME_BUFFER_LEN)
      return STG_E_INVALIDNAME;

strcpyW(renamedProperty.name, pwcsNewName);

Should not cause crashes but shouldn't the test be:
renamedProperty.sizeOfNameString > PROPERTY_NAME_BUFFER_LEN*sizeof(WCHAR)

Although this is a contrieved way to put it.

* StorageImpl_ReadProperty()

WCHAR *propName = (index == This->rootPropertySetIndex) ?
        		This->filename : (WCHAR *)currentProperty+OFFSET_PS_NAME;
    memset(buffer->name, 0, sizeof(buffer->name));
    memcpy(
      buffer->name,
      propName,
      PROPERTY_NAME_BUFFER_LEN );

- Do we really have a garantee that there will be
     PROPERTY_NAME_BUFFER_LEN bytes available for memcpy to copy?
     (grrr, just saw that OFFSET_PS_NAME==0, still you either suppose
     it's 0 or not)
   - Furthermore since this is bytes, there could be as much as
     2*PROPERTY_NAME_BUFFER_LEN bytes to copy!
   - What's the point of the memset by the way?

-> a unicode strncpy seems indicated

* StorageImpl_WriteProperty
   -> pretty much the same problem

--
Francois Gouget         [e-mail]        http://fgouget.free.fr/
           Cahn's Axiom: When all else fails, read the instructions.

[Eric Pouech]

> Well, one problem is that the winelib program was accustomed to run
> with stdin assigned to a file or pipe.  That seems to confuse winedbg,
> so I moved it for now to a different fd.

as of today, the debugger uses the standard input / output handles the
following way:
- if you do nothing, it'll use the one inherited from stdin and stdout.
if those aren't hooked up to a console, we're in bad shape. that's not
a debugger issue, it's a console issue
- if you run the debugger thru the wineconsole, then you're fine, you'll
get a brand new pair of input/output handles
- if the debugger UseXTerm is set to true, you're fine too, because the
debugger will fire a new console (whatever the state you're in), and 
you're back to previous square

what still remains to be fixed is adding support for console handles 
based on unix consoles (today, it's still a bit hacky), and implement
a (n)curse (or xterm as Marcus once did) back end for wineconsole

A+

On Wed, 6 Feb 2002, Eric Pouech wrote:
> what still remains to be fixed is adding support for console handles
> based on unix consoles (today, it's still a bit hacky), and implement
> a (n)curse (or xterm as Marcus once did) back end for wineconsole
>
> A+

I didn't mean to criticize the debugger.  It can't display COM objects
in a form I can understand, but I doubt anything could.  In general, I'd
rather have it on a unix console, but if stdin is occupied, it is
reasonable that it needs a winecosole or separate xterm.  In fact, that
is an elegant solution to the general problem of debuggers and terminal
input/output.

I would like to know if/how it is possible to set a watchpoint from
inside, say, a Winelib program (without any UI involved until the
watchpoint trips), but don't waste a lot of your time on it if it's not.

The ability to recall and edit the last command (the one I just
fat-fingered) would be nice.  :-)

Lawson

winedbg is my friend.

[Eric Pouech]

> I would like to know if/how it is possible to set a watchpoint from
> inside, say, a Winelib program (without any UI involved until the
> watchpoint trips), but don't waste a lot of your time on it if it's not.

normally yes. start your program from the debugger, then set your
watchpoint:
watch * <addr>
or
watch <id>
if id is known (it could be complex expression), then size of watchpoint
(1, 2, 4) should be set according to size of <id>
then run your program (you may need to set temporary bp:s to be able
to know where to set the watchpoint)
> The ability to recall and edit the last command (the one I just
> fat-fingered) would be nice.  :-)

if run though wineconsole (at least), most emacs commands work.
So Ctrl-P recalls last command and allows you to edit it (it would
be possible to use the regular windows arrows and such, but the
trigger isn't set in the debugger)

A+