[Tcl-bugs] [ tcl-Bugs-3362446 ] registry keys command fails with 8.5/8.6

SourceForge Headquarters 225 Broadway Suite 1600 San Diego, CA 92101 +1 (858) 454-5900

Bugs item #3362446, was opened at 2011-07-10 21:35
Message generated for change (Comment added) made by apnadkarni
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=110894&aid=3362446&group_id=10894

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: 32. registry Package
Group: None
Status: Open
Resolution: None
Priority: 7
Private: No
Submitted By: Ashok P. Nadkarni (apnadkarni)
Assigned to: Jan Nijtmans (nijtmans)
Summary: registry keys command fails with 8.5/8.6

Initial Comment:
Use of the [registry keys] command fails under some circumstances on 8.5/8.6. This failure was not present in 8.4

With 8.5
% package require Tcl
8.5.9
% package require registry
1.2.1
% registry keys {HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Services\Eventlog}
unable to enumerate subkeys of "HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Services\Eventlog": More data is available.

With 8.6, identical failure
(tcl) 49 % package require Tcl
8.6b1.2
(tcl) 50 % package require registry
1.3
(tcl) 51 % registry keys {HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Services\Eventlog}
unable to enumerate subkeys of "HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Services\Eventlog": More data is available.

With 8.4, this failure does not happen
(tools) 1 % package require Tcl
8.4
(tools) 2 % package require registry
1.1.3
(tools) 3 % registry keys {HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Services\Eventlog}
Application {Internet Explorer} ODiag OSession Security System

Logging bug as placeholder, Will follow up with additional info/trace

/Ashok

----------------------------------------------------------------------

>Comment By: Ashok P. Nadkarni (apnadkarni)
Date: 2012-06-20 22:07

Message:
Jan, I'm a bit confused by your last comment about not merging into
core-8-4-branch and up. Does that mean you don't plan on merging into 8.6 ?
Note from my original report that this problem only shows up with 8.6 since
that is where the change to Unicode calls was made.

/Ashok

----------------------------------------------------------------------

Comment By: Jan Nijtmans (nijtmans)
Date: 2012-06-20 00:41

Message:
The only reason I am hesitating to merge this to
core-8-4-branch and up, is because there is no test
case which produces the error message "unable to
query key ...".  Maybe it was impossible from
the start to get this error message ever: If the
OpenKey() call succeeded alreay (so we know that
the key exists) why would the following
RegQueryInfoKey fail? msdn doesn't say anything
about that, other than that RegQueryInfoKey can
produce a system error message. But in the Tcl
code, it seems impossible.

Added test-case 6.21 now, seems that very long
value names and values work fine.

----------------------------------------------------------------------

Comment By: Ashok P. Nadkarni (apnadkarni)
Date: 2012-06-19 19:13

Message:
Seems reasonable to me. 

----------------------------------------------------------------------

Comment By: Jan Nijtmans (nijtmans)
Date: 2012-06-19 06:22

Message:
See:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms724862%28v=vs.85%29.aspx

This recommends just calling RegEnumKeyEx  until the
function returns ERROR_NO_MORE_ITEMS. Further on,
it's not worth to query the buffersize, if the maximum length
is not more than 256. This eliminates the need for
RegQueryInfoKey altogether, and it eliminates the
possibility for race consitions.

This suggestion is commited to branch
bug-3362446 now. All test-cases pass.

Agreed with this solution?

----------------------------------------------------------------------

Comment By: Jan Nijtmans (nijtmans)
Date: 2012-06-19 04:59

Message:
> - There is buffer overflow, I believe, if the code is compiled without
> _UNICODE defined (this may not matter anymore). The call to
> Tcl_WinTCharToUtf should use sizeof(TCHAR), not sizeof(WCHAR) I believe.

Good catch! That should be fixed now in trunk.

----------------------------------------------------------------------

Comment By: Ashok P. Nadkarni (apnadkarni)
Date: 2011-07-11 19:08

Message:
I dunno about serious but I hate it when operations fail for no apparent
reason. The glob example I gave I think illustrates the point - you don't
want glob failing because someone deletes/renames a file in the glob
target. Of course, registry enum is likely to be far less common than
globbing files but still...

There are two other calls - to delete a key (tree) and enumerate values.
The first I don't think needs fixing because when deleting a tree, the
caller must always expect failure modes caused by other processes. The
latter case probably does need fixing (but not the same fix since the value
names are not limited to just 256 bytes.

If you want, I can look at producing a patch for that as well.

/Ashok

----------------------------------------------------------------------

Comment By: Joe Mistachkin (mistachkin)
Date: 2011-07-11 13:08

Message:
If these race conditions are truly a serious problem, perhaps other parts
of tclWinReg.c need to be rewritten as well?  For example, I see several
other calls to RegQueryInfoKey.

----------------------------------------------------------------------

Comment By: Ashok P. Nadkarni (apnadkarni)
Date: 2011-07-11 04:46

Message:
I won't get a hold of the failing machine again until tomorrow so I will
try it then.

Also, winReg_v2.diff does not handle the race condition when one of the
keys is deleted. In this case, the command currently simply fails, whereas
it should return a valid key list (either including or exculding the
deleted key). It is akin to [glob] failing because someone deleted a file
at the same time.

Personal opinion, after making changes similar to yours, and then adding
the handling of ERROR_NO_MORE_ITEMS to avoid the race, I thought it cleaner
to rewrite the function, although I do understand the philosophy behind
minimizing changes. Also, Tcl does MAX_PATH stack allocations in many
places so a stack var of 257 saves unnecessary allocations.

It's of course your call! I'll let you know tomorrow once I get hold of the
failing system. I don't see why it would not work

/Ashok

----------------------------------------------------------------------

Comment By: Joe Mistachkin (mistachkin)
Date: 2011-07-11 03:18

Message:
Attaching a more robust patch that fixes the Tcl_Obj leak as well.

----------------------------------------------------------------------

Comment By: Joe Mistachkin (mistachkin)
Date: 2011-07-11 02:45

Message:
Thanks for the patch.  I'm wondering if something a bit more minimal would
work in your case?  Could you try that attached patch and see if the issue
persists?

----------------------------------------------------------------------

Comment By: Ashok P. Nadkarni (apnadkarni)
Date: 2011-07-11 01:33

Message:
(Sigh - SF seems to have thrown my previous patch update into a bitbucket
somewhere, reattaching)

Added patch - getkeynames.c - replace GetKeyNames in tclWinReg.c with the
corresponding function in attached getkeynames.c file. Tested registry.test
against 8.6 head.

/Ashok

----------------------------------------------------------------------

Comment By: Ashok P. Nadkarni (apnadkarni)
Date: 2011-07-11 01:29

Message:
Patch is attached. Replaces the GetKeyName() function in win/tclWinReg.c.
Tested against 8.6 head registry.test, all pass except 16 tests skipped
(english/nonportable)

----------------------------------------------------------------------

Comment By: Ashok P. Nadkarni (apnadkarni)
Date: 2011-07-11 00:07

Message:
Tracing through the code, the failure is in tclWinReg.c:GetKeyNames. The
code calls RegQueryInfoKey to get number of keys and max length, allocates
a buffer and iterates through the number of keys.

There are several problems in this function:

- race condition - the max length AND/OR number of keys can change during
the iteration (if another thread/process modified/adds/deletes a key),
causing the function to fail with either ERROR_MORE_DATA or
ERROR_NO_MORE_ITEMS
- There is a memory leak in case of errors
- There is buffer overflow, I believe, if the code is compiled without
_UNICODE defined (this may not matter anymore). The call to
Tcl_WinTCharToUtf should use sizeof(TCHAR), not sizeof(WCHAR) I believe.

Ironially enough, none of the above is the cause of the observed failure.
The apparent reason is that RegQueryInfoKey does not always return the
correct information when the Unicode version is used and the original key
was stored using a MBCS call (this is just scuttlebutt but seems to match
the circumstantial evidence). So the max length assumed by the function is
in fact too small. I verified this in Visual Studio - the "Internet
Explorer" key is 17 chars, but the returned max by RegQueryInfoKey is only
12. This is really then an MS bug, but their position is that
RegQueryInfoKey return values cannot be relied upon and the sample
iterative method in the sdk should be followed.

Patch to follow...

/Ashok

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=110894&aid=3362446&group_id=10894

[Tcl-bugs] [ tcl-Bugs-3362446 ] registry keys command fails with 8.5/8.6

The Tool Command Language implementation

[Tcl-bugs] [ tcl-Bugs-3362446 ] registry keys command fails with 8.5/8.6