From: SourceForge.net <no...@so...> - 2011-07-11 20:08:50
|
Bugs item #3362446, was opened at 2011-07-10 21:35 Message generated for change (Comment added) made by mistachkin 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: 5 Private: No Submitted By: Ashok P. Nadkarni (apnadkarni) Assigned to: Kevin B KENNY (kennykb) 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: 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 |