Re: [Patch] Encode invalid chars in /proc/registry entries
Hi Christian,
On Nov 15 22:56, Christian Franke wrote:
Registry key and value names may contain chars which are not allowed within
file names ('/', '\', :). But Cygwin's /proc/registry returns these names
unchanged to the app. The obvious effect is that such entries cannot be
accessed.
But if an entry name is identical to an existing path, more interesting
results occur. Cygwin itself adds registry entries which are testcases for
this issue :-))
[...]
The attached patch encodes the critical chars with %XX to avoid such
problems.
Patch is tested with 1.5.24-2. Merge with HEAD looks good, but was not
actually tested. Therefore, no changelog provided yet.
Thanks for this patch. Apart from the missing ChangeLog I'm inclined
to apply it to the upcoming 1.5.25 release, but I don't like to have it
in HEAD as is.
The reason is that the patch introduces more usages of CYG_MAX_PATH plus
static buffers of that size. That's ok for 1.5, but that's not ok
anymore for 1.7. We're heading to support PATH_MAX = ~32K paths. The
registry also supports long paths, unfortunately with undefined max
length. The current definition in MSDN(*) is
Max name length of keys: 255 chars
Max name length of values: 16383 chars
Max tree depth: 512 levels
So, for HEAD I'd like to ask you to allow arbitrary path lengths in your
code. Personally I could live with restricting registry paths to
PATH_MAX as well.
While you're digging in registry code anyway... would you be interested
to convert the entire registry code to wide char and long path names?
I'd be glad for any help.
Corinna
(*) http://msdn2.microsoft.com/en-us/library/ms724872.aspx
--
Corinna Vinschen Please, send mails regarding Cygwin to
Cygwin Project Co-Leader cygwin AT cygwin DOT com
Red Hat
Re: [Patch] Encode invalid chars in /proc/registry entries
On Fri, Nov 16, 2007 at 08:35:48PM +0100, Christian Franke wrote: Christopher Faylor wrote: .. Patch is tested with 1.5.24-2. Merge with HEAD looks good, but was not actually tested. Therefore, no changelog provided yet. Thanks for this patch. Apart from the missing ChangeLog I'm inclined to apply it to the upcoming 1.5.25 release, but I don't like to have it in HEAD as is. I'm not so sure it's appropriate for either yet. Isn't it possible to use at least some of the managed mode functions which deal with munging characters to do some of encoding? It seems like the patch duplicates some of the functionality from path.cc. I realize that the registry is sort of the opposite of a managed mount but it seems like the encoding functions might be potentially used in reverse for this. I actually consulted path.cc before starting the patch but did not find any function which provides the required functionality OOTB. Therefore, I solved the tradeoff between reuse and do not change working code if you don't have time for thorough regression testing by the latter :-) I'm sorry but reuse is a fairly important concept in a project like this. The proc functions, in particular, have been prone to NIH and I don't want to see even more there if we can possibly help it. So, I'll reiterate my suggestion that you look at, e.g., mount_item::fnmunge and possibly think about generalizing it if it isn't quite up to the task. I'll also go on record as advocating that this not be part of a bugfix release. It seems too much like a last minute change to me. Getting it into cvs main, however, seems like a good idea. cgf
Re: [Patch] Encode invalid chars in /proc/registry entries
On Fri, Nov 16, 2007 at 12:09:01PM +0100, Corinna Vinschen wrote:
Hi Christian,
On Nov 15 22:56, Christian Franke wrote:
Registry key and value names may contain chars which are not allowed within
file names ('/', '\', :). But Cygwin's /proc/registry returns these names
unchanged to the app. The obvious effect is that such entries cannot be
accessed.
But if an entry name is identical to an existing path, more interesting
results occur. Cygwin itself adds registry entries which are testcases for
this issue :-))
[...]
The attached patch encodes the critical chars with %XX to avoid such
problems.
Patch is tested with 1.5.24-2. Merge with HEAD looks good, but was not
actually tested. Therefore, no changelog provided yet.
Thanks for this patch. Apart from the missing ChangeLog I'm inclined
to apply it to the upcoming 1.5.25 release, but I don't like to have it
in HEAD as is.
I'm not so sure it's appropriate for either yet.
Isn't it possible to use at least some of the managed mode functions
which deal with munging characters to do some of encoding? It seems
like the patch duplicates some of the functionality from path.cc.
I realize that the registry is sort of the opposite of a managed mount
but it seems like the encoding functions might be potentially used in
reverse for this.
cgf
Re: [Patch] Encode invalid chars in /proc/registry entries
Hi Corinna,
Corinna Vinschen wrote:
...
Patch is tested with 1.5.24-2. Merge with HEAD looks good, but was not
actually tested. Therefore, no changelog provided yet.
Thanks for this patch. Apart from the missing ChangeLog I'm inclined
to apply it to the upcoming 1.5.25 release, but I don't like to have it
in HEAD as is.
Thanks, I would appreciate to have this issue fixed in the bugfix release.
Here is a new version of the patch and a ChangeLog.
The names . and .. are now also encoded. Theses are also valid as
Key/Value Names and .. may result in infinite recursion.
The reason is that the patch introduces more usages of CYG_MAX_PATH plus
static buffers of that size. That's ok for 1.5, but that's not ok
anymore for 1.7. We're heading to support PATH_MAX = ~32K paths. The
registry also supports long paths, unfortunately with undefined max
length. The current definition in MSDN(*) is
Max name length of keys: 255 chars
Max name length of values: 16383 chars
Max tree depth: 512 levels
So, for HEAD I'd like to ask you to allow arbitrary path lengths in your
code. Personally I could live with restricting registry paths to
PATH_MAX as well.
Agree. Probably Cygwin should never descend paths that exceed PATH_MAX,
as an application using PATH_MAX may have no buffer overflow check.
While you're digging in registry code anyway... would you be interested
to convert the entire registry code to wide char and long path names?
I'd be glad for any help.
I will have a look at it, but be patient. Is current HEAD a reasonable
starting point or is there a better (more stable) snapshot?
Christian
2007-11-16 Christian Franke [EMAIL PROTECTED]
* fhandler_registry.cc (must_encode): New function.
(encode_regname): Ditto.
(decode_regname): Ditto.
(fhandler_registry::exists): Encode name before path compare.
(fhandler_registry::fstat): Pass decoded name to win32 registry call.
(fhandler_registry::readdir): Return encoded name to user.
(fhandler_registry::open): Store decoded name into value_name.
(open_key): Pass decoded name to win32 registry call.
--- cygwin-1.5.24-2.orig/winsup/cygwin/fhandler_registry.cc 2006-01-27 22:50:40.00100 +0100
+++ cygwin-1.5.24-2/winsup/cygwin/fhandler_registry.cc 2007-11-16 16:34:45.0 +0100
@@ -86,6 +86,69 @@ static const char *DEFAULT_VALUE_NAME =
static HKEY open_key (const char *name, REGSAM access, bool isValue);
+/* Return true if char must be encoded.
+ */
+static inline bool
+must_encode (char c)
+{
+ return (isdirsep (c) || c == ':' || c == '%');
+}
+
+/* Encode special chars in registry key or value name.
+ */
+static int
+encode_regname (char * dst, const char * src)
+{
+ int di = 0;
+ for (int si = 0; src[si]; si++)
+{
+ char c = src[si];
+ if (must_encode (c) ||
+ (c == '.' si == 0 (!src[1] || (src[1] == '.' !src[2]
+ {
+ if (di + 3 = CYG_MAX_PATH)
+ return ENAMETOOLONG;
+ __small_sprintf (dst + di, %%%02x, c);
+ di += 3;
+ }
+ else
+ dst[di++] = c;
+}
+ dst[di] = 0;
+ return 0;
+}
+
+/* Decode special chars in registry key or value name.
+ */
+static int
+decode_regname (char * dst, const char * src, int len = -1)
+{
+ if (len 0)
+len = strlen (src);
+ int di = 0;
+ for (int si = 0; si len; si++)
+{
+ char c = src[si];
+ if (c == '%')
+ {
+ if (si + 2 = len)
+ return EINVAL;
+ char s[] = {src[si+1], src[si+2], '\0'};
+ char *p;
+ c = strtoul (s, p, 16);
+ if (!(must_encode (c) ||
+ (c == '.' si == 0 (len == 3 || (src[3] == '.' len == 4)
+ return EINVAL;
+ dst[di++] = c;
+ si += 2;
+ }
+ else
+ dst[di++] = c;
+}
+ dst[di] = 0;
+ return 0;
+}
+
/* Returns 0 if path doesn't exist, 0 if path is a directory,
* 0 if path is a file.
*
@@ -166,8 +229,9 @@ fhandler_registry::exists ()
NULL, NULL))
|| (error == ERROR_MORE_DATA))
{
- if (pathmatch (buf, file) || (buf[0] == '\0'
- pathmatch (file, DEFAULT_VALUE_NAME)))
+ char enc_buf[CYG_MAX_PATH];
+ if ( (buf[0] == '\0' pathmatch (file, DEFAULT_VALUE_NAME))
+ || (!encode_regname (enc_buf, buf) pathmatch (enc_buf, file)))
{
file_type = -1;
goto out;
@@ -246,9 +310,11 @@ fhandler_registry::fstat (struct __stat6
while (!isdirsep (*value_name))
value_name--;
value_name++;
+ char dec_value_name[CYG_MAX_PATH];
DWORD dwSize;
- if (ERROR_SUCCESS ==
- RegQueryValueEx (hKey, value_name, NULL, NULL, NULL,
+ if (!decode_regname (dec_value_name, value_name)
+ ERROR_SUCCESS ==
+ RegQueryValueEx (hKey, dec_value_name, NULL, NULL, NULL,
dwSize))
buf-st_size = dwSize;
}
@@ -338,8 +404,8 @@ retry:
/* We get here if `buf' contains valid data. */
if (*buf == 0)
strcpy (de-d_name, DEFAULT_VALUE_NAME);
- else
-strcpy (de-d_name, buf);
+
Re: [Patch] Encode invalid chars in /proc/registry entries
Christopher Faylor wrote: .. Patch is tested with 1.5.24-2. Merge with HEAD looks good, but was not actually tested. Therefore, no changelog provided yet. Thanks for this patch. Apart from the missing ChangeLog I'm inclined to apply it to the upcoming 1.5.25 release, but I don't like to have it in HEAD as is. I'm not so sure it's appropriate for either yet. Isn't it possible to use at least some of the managed mode functions which deal with munging characters to do some of encoding? It seems like the patch duplicates some of the functionality from path.cc. I realize that the registry is sort of the opposite of a managed mount but it seems like the encoding functions might be potentially used in reverse for this. I actually consulted path.cc before starting the patch but did not find any function which provides the required functionality OOTB. Therefore, I solved the tradeoff between reuse and do not change working code if you don't have time for thorough regression testing by the latter :-) Christian
