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.
Maybe I add also Jelmer to this bug.
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.
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.
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].
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.
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].
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.
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].
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.
Jelmer, what is here left to do?
Recursive deletes.
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.
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"
Created attachment 3152 [details] Here the patch
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.
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?
We've discussed it at SambaXP: Firstly, we need to support "NULL" values in the registry LDB for strings.
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.
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.
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?
> 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.
Created attachment 3400 [details] GDB LOG Here the promised GDB log. I hope it helps.
Jelmer, have you a clue how we could fix this?
Comment on attachment 3108 [details] Properly check return values from ldb_*() functions. Okay, this was applied.
Comment on attachment 3109 [details] Allow changing data for existing values. Applied in a slightly different form.
Comment on attachment 3110 [details] Allow creating, deleting, and renaming values. Mostly applied.
Comment on attachment 3111 [details] Allow creating, deleting, and renaming keys. Mostly applied.
Comment on attachment 3112 [details] Update the caches when adding/deleting keys/values. This hasn't been applied. Why?
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 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.
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.
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.
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.
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.
Created attachment 3474 [details] Patch to introduce the REG_BINARY datatype This finally introduces correctly the REG_BINARY datatype. The patch depends on #3473.
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)
Created attachment 3480 [details] Patch to fix up the WINREG RPC pipe Another small error found and fixed (discovered when implementing the default attribute).
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*.
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.
Here also an URL to a git repository where I applied the patches.
Created attachment 3572 [details] Latest patch of "ldb.c" This includes my latest version of ldb.c
I mark my attached patches as obsolete because I've added the link to my GIT repository.
Should all be fixed.