Bug 5178 - Registry backend: Problems when editing with regedit
Registry backend: Problems when editing with regedit
Status: RESOLVED FIXED
Product: Samba 4.0
Classification: Unclassified
Component: Other
unspecified
All All
: P3 normal
: ---
Assigned To: Andrew Bartlett
Samba QA Contact
http://repo.or.cz/w/Samba/mdw.git?a=l...
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-01-08 15:41 UTC by Matthias Dieter Wallnöfer
Modified: 2013-07-03 03:25 UTC (History)
1 user (show)

See Also:


Attachments
Stacktrace of the problem (9.21 KB, text/plain)
2008-01-08 16:02 UTC, Matthias Dieter Wallnöfer
no flags Details
Properly check return values from ldb_*() functions. (1.46 KB, patch)
2008-01-17 14:49 UTC, Andrew Kroeger
no flags Details
Allow changing data for existing values. (843 bytes, patch)
2008-01-17 14:51 UTC, Andrew Kroeger
no flags Details
Allow creating, deleting, and renaming values. (2.10 KB, patch)
2008-01-17 14:54 UTC, Andrew Kroeger
no flags Details
Allow creating, deleting, and renaming keys. (1.98 KB, patch)
2008-01-17 14:56 UTC, Andrew Kroeger
no flags Details
Update the caches when adding/deleting keys/values. (1.86 KB, patch)
2008-01-17 14:58 UTC, Andrew Kroeger
no flags Details
Prevent deletion of keys that have subkeys or values. (1.27 KB, patch)
2008-01-17 14:59 UTC, Andrew Kroeger
no flags Details
Here the patch (1.50 KB, patch)
2008-02-27 07:47 UTC, Matthias Dieter Wallnöfer
no flags Details
GDB LOG (16.73 KB, text/plain)
2008-07-08 16:47 UTC, Matthias Dieter Wallnöfer
no flags Details
Patch to fix the empty string problem (653 bytes, patch)
2008-08-11 12:42 UTC, Matthias Dieter Wallnöfer
no flags Details
Patch to fix the "QueryValue" RPC call (757 bytes, patch)
2008-08-12 14:35 UTC, Matthias Dieter Wallnöfer
no flags Details
Patch to fix up the WINREG RPC pipe (3.70 KB, patch)
2008-08-13 14:01 UTC, Matthias Dieter Wallnöfer
no flags Details
Patch to fix up the WINREG RPC pipe (4.08 KB, patch)
2008-08-13 14:35 UTC, Matthias Dieter Wallnöfer
no flags Details
Patch to introduce the REG_BINARY datatype (915 bytes, patch)
2008-08-13 14:36 UTC, Matthias Dieter Wallnöfer
no flags Details
Patch to fix up the WINREG RPC pipe (4.62 KB, patch)
2008-08-15 06:29 UTC, Matthias Dieter Wallnöfer
no flags Details
Patch to introduce the default attribute (6.98 KB, patch)
2008-08-15 06:33 UTC, Matthias Dieter Wallnöfer
no flags Details
Patch to fix empty string problem (653 bytes, patch)
2008-08-20 12:08 UTC, Matthias Dieter Wallnöfer
no flags Details
Latest patch of "ldb.c" (6.83 KB, patch)
2008-09-12 10:48 UTC, Matthias Dieter Wallnöfer
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Matthias Dieter Wallnöfer 2008-01-08 15:41:07 UTC
Until "regedit" is used to browse in readonly mode the SAMBA 4 registry backend there aren't any problems. However if I try to change something, f.e. to create a new key or value this is quitted by strange error results and messages in "regedit". But the worst is, that it can lead to the complete crash of the SAMBA daemon.
Comment 1 Matthias Dieter Wallnöfer 2008-01-08 15:59:50 UTC
Maybe I add also Jelmer to this bug.
Comment 2 Matthias Dieter Wallnöfer 2008-01-08 16:02:11 UTC
Created attachment 3090 [details]
Stacktrace of the problem

Here now a concrete example: I tried to add a string value under a key and the SAMBA daemon broke. Here the stacktrace from gdb.
Comment 3 Andrew Kroeger 2008-01-17 14:49:38 UTC
Created attachment 3108 [details]
Properly check return values from ldb_*() functions.

There were a few cases left that attempted to detect errors from ldb_*() function calls using "(ret < 0)".  As all LDB_* error codes are greater than zero, there was no chance any errors would be detected.  Changed all such tests to use "(ret != LDB_SUCCESS)".

This patch does not depend on any of the others.
Comment 4 Andrew Kroeger 2008-01-17 14:51:48 UTC
Created attachment 3109 [details]
Allow changing data for existing values.

When modifying the data for an existing value, the ldb_add() is meant to fail in ldb_set_value() and then fall through to call ldb_modify().  The ldb_modify() call was failing with LDB_ERR_PROTOCOL_ERROR because there were no flags set on the message elements.  Setting the flags to LDB_FLAG_MOD_REPLACE allows the data to be modified on existing values.

This patch depends on the patch in attachment 3108 [details].
Comment 5 Andrew Kroeger 2008-01-17 14:54:24 UTC
Created attachment 3110 [details]
Allow creating, deleting, and renaming values.

When Windows initially creates a new value, the value name is "New Value #1".  The '#' character was causing problems, as it was not being escaped for the dn, but the failure returned by ldb_dn_add_child_fmt() was not being caught.  This was causing the new value to be added on the parent key, not the current key.  When attempting to delete the new value (now on the parent key) the same escaping error was returned by ldb_dn_add_child_fmt(), causing the delete to delete the key and not the value.

When attempting to rename a value, Windows first tries to ensure the new name does not already exist.  When a value does not exist, Windows expects a return value of WERR_BADFILE, but WERR_NOT_FOUND was being returned instead.  Providing the WERR_BADFILE that Windows expects allows values to be renamed.

This patch does not depend on any prior patches, but may generate some fuzz if they are not applied first.
Comment 6 Andrew Kroeger 2008-01-17 14:56:43 UTC
Created attachment 3111 [details]
Allow creating, deleting, and renaming keys.

When Windows attempts to create a new key, it looks for an available key name starting with "New Key #1" and iterating up to "New Key #99" before giving up.  ldb_open_key() calls reg_path_to_ldb() to build the appropriate dn from the key name.  reg_path_to_ldb() was not catching the error returned by ldb_dn_add_base_fmt() due to the unescaped '#' character, causing the returned dn to be that of the parent key, not the potential new key.  Additionally, Windows expects a return value of WERR_BADFILE when a key does not exist, but WERR_NOT_FOUND was being returned instead.  Correcting the building of the dn and the providing the expected return value allows new key creation to succeed.

When attempting to delete a key, Windows passes the complete path to the key, not just the name of the child key to be deleted.  Using reg_path_to_ldb() to build the correct dn allows key deletion to succeed.

This patch depends on the patch in attachment 3110 [details].
Comment 7 Andrew Kroeger 2008-01-17 14:58:03 UTC
Created attachment 3112 [details]
Update the caches when adding/deleting keys/values.

When adding a new key at the top level of the hive, Windows successfully adds the key, but it does not show.  Attempting to refresh the display with F5 does not correct the situation.  Debug logs showed that the *_by_id() functions were being called, but the caches already existed and were not being refreshed.  Updating the appropriate cache when adding or deleting corrected the issue.

Code was also added to talloc_free() and the previous cache when rebuilding the cache, and also to create the caches when opening a key in ldb_open_key().

This patch does not depend on any prior patches, but may generate some fuzz if they are not applied first.
Comment 8 Andrew Kroeger 2008-01-17 14:59:33 UTC
Created attachment 3113 [details]
Prevent deletion of keys that have subkeys or values.

Orphaned subkeys or values can re-appear when a key is created with the same name as a previously deleted key that contained subkeys or values.  Similar to what is done in Samba 3, we return WERR_ACCESS_DENIED to prevent the key from being deleted if it contains subkeys or values.

If we want our behavior to be consistent with the Windows behavior, we should look to provide depth-first recursive deletion.

This patch depends on the patch in attachment 3111 [details].
Comment 9 Jelmer Vernooij 2008-01-17 20:21:17 UTC
Thanks, I have applied most of your patches. I've not applied the delete one, because I think we should rather just implement the recursive delete bit consistent with windows. 
Comment 10 Matthias Dieter Wallnöfer 2008-01-20 11:59:47 UTC
Jelmer, what is here left to do?
Comment 11 Jelmer Vernooij 2008-01-20 12:05:38 UTC
Recursive deletes.
Comment 12 Matthias Dieter Wallnöfer 2008-01-24 09:24:44 UTC
Not all problems seem to be resolved:
Recently I tried to add a key which is then by default called "Neuer Schlüssel #1" under german versions of the Registry Editor. Then I wanted to be able to add a new string under it and the SAMBA daemon crashed with a stacktrace.
Comment 13 Matthias Dieter Wallnöfer 2008-02-27 07:45:00 UTC
Now I've prepared a patch to fix the creation of empty strings with regedit. The problem is caused by the routine "convert_string_talloc" who doesn't accept a NULL-string. Another problem is the inability of LDB to create attributes with NULL values (I'm right?).
So my workaround creates every time a " " string when it gets a NULL value from regedit. I tested it only under the registry LDB backend, not local, not RPC.
Then I tried to bring up also the type REG_BINARY. Here we've the same case. Should we handle it similar? I used there code from "util.c"
Comment 14 Matthias Dieter Wallnöfer 2008-02-27 07:47:09 UTC
Created attachment 3152 [details]
Here the patch
Comment 15 Andrew Kroeger 2008-02-27 12:40:33 UTC
Once my initial set of patches was applied, I have not had any problems creating empty string (REG_SZ) values using regedit.

Are you still working under the German version of regedit?  I wonder if there are additional issues related to your comment #12.  I am starting to look into those, but I had initially thought your issues in comment #12 were related to differences between the English and German character sets, and the character escaping that is necessary for special characters in the German (and other) character sets.  Maybe there are more issues with non-English character sets.  I know that there has not been much focus on i18n in the S4 development to date.

Good catch with the REG_BINARY values.  I had not tried those.  I also get errors on those even with all of my patches applied.
Comment 16 Matthias Dieter Wallnöfer 2008-03-02 04:08:43 UTC
Jelmer, how should we proceed to fix it?
Andrew: I don't fully understand why you doesn't get problems with NULL attributes in LDB. How do you handle NULL REG_SZ entries?
Comment 17 Matthias Dieter Wallnöfer 2008-04-27 06:37:41 UTC
We've discussed it at SambaXP: Firstly, we need to support "NULL" values in the registry LDB for strings.
Comment 18 Andrew Kroeger 2008-04-27 08:31:02 UTC
Thanks for the ping on this one Matthias.

I had done some initial research when you reported your issue with empty ("NULL") REG_SZ values, but then I got tied up with work obligations and totally forgot about it.  I have revisited my initial research and done some more to try and determine what is happening that lets empty REG_SZ values work for me.

Although I am not sure what is happening, I can see the end result that is allowing the empty REG_SZ values to work for me.  When I create a new REG_SZ value using regedit (tested with Windows XP Pro, Windows Server 2008 English, & Windows Server 2008 German), something happens in the conversion process and the value ends up stored as what appears to be base64-encoded data.

When I create an empty REG_SZ value using regedit, I see the following data in the LDB store (using ldbedit) (note that even though this represents an empty string, the data is not empty and does not cause the "empty attribute" error within LDB):

data:: AA==

When I create a REG_SZ value of "Test", it appears as:

data:: VGVzdAA=

However, if I look at one of the REG_SZ values created during the provision (e.g. SYSTEM\CurrentControlSet\Control\ProductOptions\ProductType), it appears as (note the lack of the double-colon and the clear string, not an encoded value):

data: LanmanNT

As I mentioned earlier, I am not sure what is causing this.  I do not know enough about the conversions that are being done.  I will look into this some more, but am posting the differences that I have seen so far in hopes that the solution may be obvious to someone like Jelmer who is more familiar with the conversions that are happening.
Comment 19 Jelmer Vernooij 2008-05-12 16:03:32 UTC
We should always store the UTF16 encoded version of the string - which is why you're seeing base64-encoded bits. Looks like the provision is doing things wrong.
Comment 20 Andrew Kroeger 2008-07-07 01:52:31 UTC
Jelmer:

Storing strings as UTF16 makes sense, and explains why I see an attribute value of "AA==" for an empty REG_SZ value.  But this does not explain the empty attribute error that Matthias is seeing.

Is it possible that something in the Matthias' system configuration is different and affects the conversion (or maybe causes there to be no conversion whatsoever)?  I am not familiar with the inner workings of the conversions in the registry code, but the first thought I had is the LANG environment variable.  On my systems, it is set to:

LANG=en_US.UTF-8

Could a different value of LANG cause the error seen by Matthias?

Matthias:  What is the value of the LANG environment variable for the root user on your S4 machine?  Are there any other variables you can see that are related to language, character set, etc. that may affect the conversion on your system?
Comment 21 Matthias Dieter Wallnöfer 2008-07-08 16:29:44 UTC
> Matthias:  What is the value of the LANG environment variable for the root user
> on your S4 machine?  Are there any other variables you can see that are related
> to language, character set, etc. that may affect the conversion on your system?

I've a Ubuntu 6.06 Server installation in German. So my language environment variables are set to:
LANG=de_DE.UTF-8
LANGUAGE=de_DE:de:en_GB:en

I note, that something changed regarding the fault when adding a string through regedit. I get now some "conversion error". I'm going to upload a gdb trace.
Comment 22 Matthias Dieter Wallnöfer 2008-07-08 16:47:20 UTC
Created attachment 3400 [details]
GDB LOG

Here the promised GDB log. I hope it helps.
Comment 23 Matthias Dieter Wallnöfer 2008-08-02 08:56:42 UTC
Jelmer, have you a clue how we could fix this?
Comment 24 Matthias Dieter Wallnöfer 2008-08-11 11:20:40 UTC
Comment on attachment 3108 [details]
Properly check return values from ldb_*() functions.

Okay, this was applied.
Comment 25 Matthias Dieter Wallnöfer 2008-08-11 11:22:05 UTC
Comment on attachment 3109 [details]
Allow changing data for existing values.

Applied in a slightly different form.
Comment 26 Matthias Dieter Wallnöfer 2008-08-11 11:25:48 UTC
Comment on attachment 3110 [details]
Allow creating, deleting, and renaming values.

Mostly applied.
Comment 27 Matthias Dieter Wallnöfer 2008-08-11 11:29:05 UTC
Comment on attachment 3111 [details]
Allow creating, deleting, and renaming keys.

Mostly applied.
Comment 28 Matthias Dieter Wallnöfer 2008-08-11 11:31:21 UTC
Comment on attachment 3112 [details]
Update the caches when adding/deleting keys/values.

This hasn't been applied. Why?
Comment 29 Matthias Dieter Wallnöfer 2008-08-11 11:33:25 UTC
Comment on attachment 3113 [details]
Prevent deletion of keys that have subkeys or values.

This hasn't been applied, but I think, another approach has been taken to fix the subtree deletes (see "ldb_del_key").
Comment 30 Matthias Dieter Wallnöfer 2008-08-11 11:34:33 UTC
Comment on attachment 3152 [details]
Here the patch

Okay, my workaround hasn't been so good, so we have to take another way to fix this.
Comment 31 Matthias Dieter Wallnöfer 2008-08-11 12:42:19 UTC
Created attachment 3464 [details]
Patch to fix the empty string problem

Now I've prepared a good patch, who fixes finally this issue. I enhanced the "utf8_push" function, who finally accepts now also the char sequence "" with length 1 as valid UTF8 string.
Comment 32 Matthias Dieter Wallnöfer 2008-08-12 14:35:45 UTC
Created attachment 3466 [details]
Patch to fix the "QueryValue" RPC call

The "QueryValue" call of SAMBA 4 doesn't behave completely like Windows (2000 in my case) in case of errors.
Comment 33 Matthias Dieter Wallnöfer 2008-08-13 14:01:49 UTC
Created attachment 3472 [details]
Patch to fix up the WINREG RPC pipe

I enhanced the latest patch because also other calls weren't completely Windows compatible.
In fact, the WINREG pipe from a Windows system sends back the client request informations when a query fails. SAMBA 4, however, sends every time NULL's back.
Comment 34 Matthias Dieter Wallnöfer 2008-08-13 14:35:40 UTC
Created attachment 3473 [details]
Patch to fix up the WINREG RPC pipe

And another update of the previous patch. This is needed to allow the REG_BINARY type.
Comment 35 Matthias Dieter Wallnöfer 2008-08-13 14:36:50 UTC
Created attachment 3474 [details]
Patch to introduce the REG_BINARY datatype

This finally introduces correctly the REG_BINARY datatype. The patch depends on #3473.
Comment 36 Matthias Dieter Wallnöfer 2008-08-13 14:39:44 UTC
Summary:
- The recursive deletes seem to work on my environment
- All datatypes work
- The last thing to implement is the "(Standard)" attribute (it can't save data)
Comment 37 Matthias Dieter Wallnöfer 2008-08-15 06:29:07 UTC
Created attachment 3480 [details]
Patch to fix up the WINREG RPC pipe

Another small error found and fixed (discovered when implementing the default attribute).
Comment 38 Matthias Dieter Wallnöfer 2008-08-15 06:33:00 UTC
Created attachment 3481 [details]
Patch to introduce the default attribute

This contains all previous patches for "lib/registry/ldb.c" and introduces the default attribute. I save the data for it in the hive object in the LDB database.
With the three patches applied (#3464, #3480 and this) we should have the whole registry backend with the in the RPC server implemented calls *working*.
Comment 39 Matthias Dieter Wallnöfer 2008-08-20 12:08:47 UTC
Created attachment 3497 [details]
Patch to fix empty string problem

This is an update of the patch, because in a recent checkin the iconv.c file has been changed. It should fix the handling with the empty strings on the Win2k (maybe also other versions) registry editor.
Comment 40 Matthias Dieter Wallnöfer 2008-09-09 14:35:26 UTC
Here also an URL to a git repository where I applied the patches.
Comment 41 Matthias Dieter Wallnöfer 2008-09-12 10:48:48 UTC
Created attachment 3572 [details]
Latest patch of "ldb.c"

This includes my latest version of ldb.c
Comment 42 Matthias Dieter Wallnöfer 2008-09-12 13:56:17 UTC
I mark my attached patches as obsolete because I've added the link to my GIT repository.
Comment 43 Matthias Dieter Wallnöfer 2008-10-21 17:31:06 UTC
Should all be fixed.