Bug 6195 - Migrating from 3.0.x to 3.3.x can fail to update passdb.tdb correctly.
Summary: Migrating from 3.0.x to 3.3.x can fail to update passdb.tdb correctly.
Status: RESOLVED FIXED
Alias: None
Product: Samba 3.3
Classification: Unclassified
Component: User & Group Accounts (show other bugs)
Version: unspecified
Hardware: Other Linux
: P3 regression
Target Milestone: ---
Assignee: Jeremy Allison
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-03-18 16:23 UTC by Jeremy Allison
Modified: 2009-07-27 05:32 UTC (History)
2 users (show)

See Also:


Attachments
Fix for 3.3.2. (10.20 KB, patch)
2009-03-18 18:42 UTC, Jeremy Allison
no flags Details
Extra patch for 3.2. (7.42 KB, patch)
2009-03-27 12:42 UTC, Jeremy Allison
no flags Details
Easy patch on top of current code. (1.76 KB, patch)
2009-03-27 13:47 UTC, Jeremy Allison
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Allison 2009-03-18 16:23:03 UTC
From Alexander Zagrebin <alexz@visp.ru>

I have tried to migrate from samba version 3.0.32 to 3.3.2 and found
the serious problem: samba can't convert my passdb.tdb from version 3 to
version 4.

For example, when running "pdbedit -L" I receive:

# pdbedit -L
tdbsam_open: Converting version 3 database to version 4.
Could not store the new record: NT_STATUS_UNSUCCESSFUL
Converting records failed
tdbsam_open: Error when trying to convert tdbsam
[/usr/local/etc/samba/passdb.tdb]
tdbsam_getsampwnam: failed to open /usr/local/etc/samba/passdb.tdb!
Could not start searching users
Segmentation fault (core dumped)

With debugging (-d5) the output contains:
...
Finding user ????
Trying _Get_Pwnam(), username as lowercase is ????
Get_Pwnam_internals did find user [????]!
Could not store the new record: NT_STATUS_UNSUCCESSFUL
Converting records failed
...

This problem exists only for user names, containing characters with codes
from
range 128...255 (with high bit set).
After doing some debugging, I have found that:

1. Possible the "break" is missed at source/passdb/pdb_tdb.c:

--- source/passdb/pdb_tdb.c.orig        2009-02-24 10:38:16.000000000 +0300
+++ source/passdb/pdb_tdb.c             2009-03-14 23:47:41.000000000 +0300
@@ -102,6 +102,7 @@
                ret = init_samu_from_buffer(user, SAMU_BUFFER_V3,
                                            (uint8 *)rec->value.dptr,
                                            rec->value.dsize);
+               break;
        case 4:
                ret = init_samu_from_buffer(user, SAMU_BUFFER_V4,
                                            (uint8 *)rec->value.dptr,

2. The reason of the above described problem is in default_tdb_hash
(source/lib/tdb/common/open.c)
This function returns different results on 3.0.* and 3.3.* for same data,
containing non-ascii
characters. The code is the same, but definition of TDB_DATA
(source/lib/tdb/include/tdb.h)
was changed. Early the TDB_DATA.dptr was signed, but now it is unsigned. So
the hash calculation
makes a different result now and pdbedit can't store the new (upgraded)
record.
Comment 1 Jeremy Allison 2009-03-18 16:24:08 UTC
This is a really nasty one to fix as in order to successfully update the passdb.tdb we must do the equivalent of a tdbbackup to move to the new hash values before we do the upgrade.
Jeremy.
Comment 2 Jeremy Allison 2009-03-18 18:42:04 UTC
Created attachment 3998 [details]
Fix for 3.3.2.
Comment 3 Michael Adam 2009-03-19 19:57:38 UTC
Hi Jeremy,

this code make samba panic when running against ctdb:
The file /etc/samba/passdb.tdb is simply not there (since
the tdb calls are handed to ctdb by dbwrap and so the
actual databases are placed elsewhere), so the rename fails
and a panic is triggered.

It is too late at night for me now to think of a proper solution.
But renaming is not an option sice the db's are under control
of ctdb.

Cheers - Michael
Comment 4 Jeremy Allison 2009-03-27 12:42:31 UTC
Created attachment 4031 [details]
Extra patch for 3.2.

Extra patch that goes on top of current 3.2.x git. If this tests out ok I'll forward port to 3.3.x and beyond.
Jeremy.
Comment 5 Jeremy Allison 2009-03-27 13:06:27 UTC
Patch still needs some work. This still really sucks as if someone is moving from 3.0.x to 3.2 the passdb version is still the same (3), but the hashes are different. I can't think of a good way to deal with that. Moving to 3.3.x and above is better as we're always changing passdb versions from 3 -> 4 in this case.
Jeremy.
Comment 6 Jeremy Allison 2009-03-27 13:47:58 UTC
Created attachment 4032 [details]
Easy patch on top of current code.

The extra patch fails, due to a subtle problem :-).

The delete in the traverse fails due to the fact
that the hash changed between the old and new
tdb libraries :-(. So you can't delete from
the old db at all, you'd need to do a CLEAR_IF_FIRST
or some other way of blowing away the old db.

I then realized I was just going about this
the wrong way :-).

The problem is that the backup -> rename
strategy fails for clustered setups.

But clustered setups have only ever used
the unsigned version of TDB_DATA in the
first place so they can't be in this mess :-).

So here is a *much* simpler patch that will
fix the issue on top of our current code :-).

Jeremy.
Comment 7 Michael Adam 2009-04-01 06:52:58 UTC
The patch made smbd child processes pannic in 3.2.9:
when the main smbd did the conversion, then a client connect
would make the child smbd panci in reinit_after_fork():
Because the tdbsam_convert_backup() call did the rename
without closing the dbs, the db context still had the 
passdb.tdb.tmp file name so the reinit would try to open
not passdb.tdb but passdb.tdb.tmp. Hence panic.

I have pushed a fix to v3-2-test, v3-3-test, v3-4-test and master

Karolin is going to release 3.2.10 with this fix today.

Cheers - Michael
Comment 8 Michael Adam 2009-04-01 06:55:14 UTC
Jeremy, by the way, as Volker has just pointed out to me:

You said you did not want to do the conversion in-memory.
But you wrap the actions in tdbsam_convert_backup() in a
transaction -- this effectively keeps stuff in memory as well.

Like, this, you could just do it directly in-memory right away
without the extra tdb.

Cheers - Michael
Comment 9 Karolin Seeger 2009-07-27 05:32:37 UTC
I think this one was fixed with Michael's patch.
Closing out bug report.

Thanks!