This is the mail archive of the cygwin mailing list for the Cygwin 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: cat /proc/registry/HKEY_PERFOMANCE_DATA/@ hangs


Since Dave is not subscribed to cygwin-developers anyway, I'll continue
this here.  More below.

On Wed, 14 Jul 2004, Dave Korn wrote:

> > -----Original Message-----
> > From: cygwin-owner On Behalf Of Igor Pechtchanski
> > Sent: 14 July 2004 04:22
>
> > Ok, the theory washed out.  The code above is actually simply buggy.
> > When RegQueryValueEx is called (2 lines below the arrow), the "size"
> > parameter is uninitialized, so, in effect, it keeps thinking that the
> > buffer has some random size and reallocating (which, of course,
> > doesn't change the size, hence the infinite loop).
>
>   I concur; that is bad code.  The variable unambiguously needs
> initialising, and since RegQueryValueEx damages it, it needs to be
> re-set each time round the loop.

Not quite true.  Turns out RegQueryValueEx doesn't touch the value of size
if the buffer is too small (for HKEY_PERFORMANCE_DATA).  So size stayed at
0.

>   I disagree with your analysis, though.  After the first time round the
> loop, size is no longer uninitialised.  After all, the call to
> RegQueryValueEx sets size to the amount of space needed.

That is not true for HKEY_PERFORMANCE_DATA.  See MSDN
(<http://msdn.microsoft.com/library/en-us/sysinfo/base/regqueryvalueex.asp>):

	If hKey specifies HKEY_PERFORMANCE_DATA and the lpData buffer is
	not large enough to contain all of the returned data,
	RegQueryValueEx returns ERROR_MORE_DATA and the value returned
	through the lpcbData parameter is undefined.  This is because the
	size of the performance data can change from one call to the next.
	In this case, you must increase the buffer size and call
	RegQueryValueEx again passing the updated buffer size in the
	lpcbData parameter.  Repeat this until the function succeeds.
	You need to maintain a separate variable to keep track of the
	buffer size, because the value returned by lpcbData is
	unpredictable.

> Second time round the loop, we set bufalloc to 2000, realloc that much
> space, then call RegQueryValueEx, passing it the pointer to this 2000
> byte buffer, and the size variable is still whatever RegQueryValueEx set
> it to last time, i.e. the full size of the value's data.  So when we
> call RegQueryValueEx the second time, it thinks the buffer is loads
> bigger than it actually is - in fact, it thinks the buffer is exactly
> big enough for the value's data - and it copies waaaay more data into
> the buffer than it has room for.  Bang! Heap corruption!

Hmm, did you verify this in gdb?  Mine showed size=0 until I added the
assignment.

> > However, the fix is not as simple as inserting a "size = bufalloc;"
> > just before the RegQueryValueEx.  When I do that, I get a SIGSEGV in
> > the guts of iasperf.dll, which I have yet to track down.  This happens
> > on the second iteration, FWIW, with buffer increment of 1000.
>
>   That's funny.  It WFMHRN.

"Works for me here right now"?

> Are you completely sure that you put that line in the right place?

Yes.

> Are you sure you remembered to rebuild the dll after editing the source?

Yes.

> Are you sure you tested the new version of the dll rather than some old
> one?

Yes.

> I know some of these things sound daft, but since the fix is a)
> obviously correct, and b) tested and working, I suspect that you made
> some little error in the procedure that invalidated your test.

...or we have some differences in user permissions, operating systems,
environments, setup, etc, etc, etc.  In any case, the above does *NOT*
work for me.

> You wouldn't believe how often I ALT+TAB to another window and type
> "make all" only to realise I haven't saved all the changed files that I
> have in my editor window, only some of them!

I'm reasonably sure I've rebuilt everything from scratch (I even did a
"make clean" - ouch).

> In particular, the fact that you see a SEGV the second time round -
> which is what my analysis above demonstrates should happen if the size
> variable is NOT reinited each time round the loop - makes me think you
> didn't actually test the right code.  You better double-check.

Triple-checked.  Your analysis would have been correct for any key but
HKEY_PERFORMANCE_DATA.

>   Just to show that the bug really is fixed by that one-line patch,
> here's a dump of it WFMHRNing:
> [snip]

I believe you.  It just doesn't work for me. :-(  In fact, I'm getting a
segfault whenever the RegQueryValueEx call attempts to write anything to
a buffer, whether realloc()'d or automatic (stack-allocated).  I haven't
tried a static array, that's next on my list.  I'll try to get to the
bottom of this eventually.

>   Here's the patch, against current CVS.  I'll let Igor supply the
> ChangeLog entry, since it was his fix.

Can't for a while.  Can you do me a favor and submit this as a fix, if you
have a copyright assignment for Cygwin?  If it makes you feel better, feel
free to mention me in the ChangeLog as well.

FWIW, there's another buglet (redundant piece of code, actually): "type"
is never used, so doesn't need to be passed in.  Might as well fix that
(just pass NULL), and the typo in the file name (HKEY_PERFO*R*MANCE_DATA),
in this patch.
	Igor

> Index: winsup/cygwin/fhandler_registry.cc
> ===================================================================
> RCS file: /cvs/src/src/winsup/cygwin/fhandler_registry.cc,v
> retrieving revision 1.24
> diff -p -u -r1.24 fhandler_registry.cc
> --- winsup/cygwin/fhandler_registry.cc  9 Feb 2004 04:04:23 -0000       1.24
> +++ winsup/cygwin/fhandler_registry.cc  14 Jul 2004 16:47:53 -0000
> @@ -575,6 +575,7 @@ fhandler_registry::fill_filebuf ()
>         {
>           bufalloc += 1000;
>           filebuf = (char *) realloc (filebuf, bufalloc);
> +          size = bufalloc;
>           error = RegQueryValueEx (handle, value_name, NULL, &type,
>                                    (BYTE *) filebuf, &size);
>           if (error != ERROR_SUCCESS && error != ERROR_MORE_DATA)

-- 
				http://cs.nyu.edu/~pechtcha/
      |\      _,,,---,,_		pechtcha@cs.nyu.edu
ZZZzz /,`.-'`'    -.  ;-;;,_		igor@watson.ibm.com
     |,4-  ) )-,_. ,\ (  `'-'		Igor Pechtchanski, Ph.D.
    '---''(_/--'  `-'\_) fL	a.k.a JaguaR-R-R-r-r-r-.-.-.  Meow!

"I have since come to realize that being between your mentor and his route
to the bathroom is a major career booster."  -- Patrick Naughton

--
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple
Problem reports:       http://cygwin.com/problems.html
Documentation:         http://cygwin.com/docs.html
FAQ:                   http://cygwin.com/faq/


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