bugrdiff-backup - Bugs: bug #21202, Crash : AssertionError: no...

 
 

bug #21202: Crash : AssertionError: no ownership of ...

Submitted by:  None
Submitted on:  Mon 01 Oct 2007 04:29:46 AM UTC  
Votes:  1  
 
Category: NoneSeverity: 3 - Normal
Item Group: NoneStatus: Fixed
Privacy: PublicAssigned to: None
Open/Closed: Closed

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Mon 28 Jan 2008 04:51:32 AM UTC, comment #6:

Ok, I have done some tests and found that:
1) Doing try/except costs about 3-4x if/else
2) No noticeable penalty is paid for setting up try/except if you don't take the except branch (because no exception was raised)

Thus, I have simply changed the previous except: block to re-try the chmod and operation. If it fails again, then it says that permission is denied.

For AFS, this means that the chmod() is tried in the error handler, and will work in setups like Marc's. For other filesystems, it means that no penalty is paid for this behavior, since the original if clause is making the only effective chmod() statement.

In summary:
- AFS pays the performance penalty that Marc proposed
- Non-AFS pays a 2x penalty, but only when the file cannot be opened, otherwise it pays no penalty. Since the former case is presumably rare, the overall penalty is minimized.

See version 1.45 of backup.py in CVS.

Andrew Ferguson <owsla>
Project Administrator
Wed 26 Dec 2007 06:23:20 PM UTC, comment #5:

Alrighty, I'll reopen this so we can discuss.

Let me get this straight: rdiff-backup runs as UID X, writes files as owned by UID Y, which have permission 000, but it is able to chmod those files? Wouldn't it have to chmod them to 004 (or better, 044) in order to read them?

You need to narrow your except statements so that you don't catch errors you don't know how to handle. If we take your new patch, it should look like:

sig = Null
try:
sig = Rdiff.get_signature(dest_rp)
except IOError, e:
if (e.errno == errno.EPERM):
try:
dest_rp.chmod(0400 | dest_rp.getperms())
except IOError, e:
if (e.errno == errno.EPERM):
log.Log.FatalError("Could not open...")
else:
raise
sig = Rdiff.get_signature(dest_rp)
else:
raise

return sig

Now, I imagine that the above code is a good deal slower than the code currently in rdiff-backup. Also, the first except clause is going to be hit fairly often because it replaces the original if statement that used to handle the chmod. The try/except machinery is slower than an if statement.

This code is along a critical path: get_one_sig_fp() gets hit once for every regular file. AFS sounds like a pretty sophisticated filesystem that made the mistake of trying to live in a Unix environment. :-)

I noticed you also submited a patch in Debian for AFS. If I'm reading that one correctly, ideally you would want to see the high_perms test broken into high_perms_files and high_perms_dirs tests, so that rdiff-backup could set those independently. Right?

Andrew Ferguson <owsla>
Project Administrator
Wed 26 Dec 2007 07:19:23 AM UTC, comment #4:

oops. I mis-unmangled what savannah did to the python code below. I took a look at what's in CVS, and I'd like to change my comments a little.

The code in rev 1.41 will work with a mode 600 file, but it will fail with a mode 000 file, since the code assumes that the file can be chmod'ed only if the process uid matches the file owner. In AFS, this is not true. It ends up printing permission denied, since the chmod step is skipped.

In fact, I'm leaning further towards the patch in my previous comment. In AFS, even root cannot read the mode 000 file, so the check for process_uid != 0 is incorrect; we want to try to chmod the file even as root. And the readable() test is redundant. If the file was readable, the code would never reach the except: block, so even if the file appears readable, the chmod should be attempted.

Marc Horowitz <mhorowitz>
Wed 26 Dec 2007 07:05:31 AM UTC, comment #3:

I don't think your patch will do the right thing in my environment. AFS is weird. It's possible to have a process which is authenticated to AFS in such a way that the files it writes have a different uid than the process's uid. If you think of NFS's root/anon mapping, you'll get the right idea, although the mechanism is different, and the process is not owned by root. In addition, AFS has directory acls which are more powerful that stat() can represent, so they are invisible to stat().

In my situation, I have a rdiff-backup process running as uid X which is writing files owned by uid Y. This process will also be able to chmod the files it creates. If the file is mode 600, then the the new code will not assert, but because dest_rp.readable() is false, get_one_sig will return None, which I think will cause those files not to be backed up (this is inconvenient for me to test).

In general, I think code which presumes filesystem semantics is always going to be more fragile than simply performing the necessary operations, and handling exceptions if they occur.
Taken to the logical extreme, this would argue for an even simpler method body than in my original patch:

try:
return Rdiff.get_signature(dest_rp)
except:
# This branch can happen with root source and non-root
# destination. Permissions are changed permanently, which
# should propogate to the diffs
dest_rp.chmod(0400 | dest_rp.getperms())
return Rdiff.get_signature(dest_rp)

Marc Horowitz <mhorowitz>
Mon 24 Dec 2007 10:38:05 PM UTC, comment #2:

Ok, I see why this is happening. Marc, I based the following fix off your patch:

diff -r1.40 backup.py
205c205,206
< if Globals.process_uid != 0 and not dest_rp.readable():
---

> if (Globals.process_uid != 0 and not dest_rp.readable() and
> dest_rp.isowner()):

209d209
< assert dest_rp.isowner(), 'no ownership of %s' % (dest_rp.path,)
211,212c211,219
< return Rdiff.get_signature(dest_rp)
<
---

> try:
> return Rdiff.get_signature(dest_rp)
> except IOError, e:
> if (e.errno == errno.EPERM):
> log.Log.FatalError("Could not open %s for reading. Check "
> "permissions on file." % (dest_rp.path,))
> else:
> raise
>

Andrew Ferguson <owsla>
Project Administrator
Fri 09 Nov 2007 06:05:09 AM UTC, comment #1:

I ran into the same problem, where my target filesystem was AFS, which does weird things with permissions. I eliminated the assert too, but by reordering the code to try first and handle an exception. My patch is attached.

(file #14342)

Marc Horowitz <mhorowitz>
Mon 01 Oct 2007 04:29:46 AM UTC, original submission:

Hi

I'm a new user and try to use rdiff-backup in my environnement.
I got the foillowing problem with 1.05 and reproduce it with 1.1.14

The first rdiff backup is ok, but the following rdiff crash with traceback.

I made some tracking, but i'm new in python. This patch seems working, but i don't know the reason of the 'assert'

--- backup.py.old 2007-10-01 06:22:17.000000000 +0200
+++ backup.py 2007-09-30 22:30:51.000000000 +0200
@@ -206,7 +206,8 @@
# This branch can happen with root source and non-root
# destination. Permissions are changed permanently, which
# should propogate to the diffs
- assert dest_rp.isowner(), 'no ownership of %s' % (dest_rp.path,)
+ #assert dest_rp.isowner(), 'no ownership of %s' % (dest_rp.path,)
+ dest_rp.isowner()
dest_rp.chmod(0400 | dest_rp.getperms())
return Rdiff.get_signature(dest_rp)

My environnement :
fedora 7 lanch a rdiff on a nfs mounted directory (/net/d2r2/BACKUP/...)
The nfs has the particularity to force mapping of user :

/BACKUP *(rw,all_squash,anonuid=99,anongid=99)

Here is the end of the log (level 9)

Sun Sep 30 12:32:13 2007 Regressing attributes of /net/d2r2/BACKUP/nx/rdiff/Private/Thunderbird/extensions/{e9a9c0c3-8673-ae31-8a09-b52878ed1e}/uninstall
Sun Sep 30 12:32:13 2007 Copying attributes from ('Thunderbird', 'extensions', '{e9a9c0c3-8673-ae31-8a09-b52878ed1e}', 'uninstall') to /net/d2r2/BACKUP/nx/rdiff/Private/Thunderbird/extensions/{e9a9c0c3-8673-ae31-8a09-b52878ed1e}/uninstall
Sun Sep 30 12:32:13 2007 Setting time of /net/d2r2/BACKUP/nx/rdiff/Private/Thunderbird/extensions/{e9a9c0c3-8673-ae31-8a09-b52878ed1e}/uninstall to 1113044062
Sun Sep 30 12:32:13 2007 Regressing attributes of /net/d2r2/BACKUP/nx/rdiff/Private/Thunderbird/extensions/{e9a9c0c3-8673-ae31-8a09-b52878ed1e}
Sun Sep 30 12:32:13 2007 Copying attributes from ('Thunderbird', 'extensions', '{e9a9c0c3-8673-ae31-8a09-b52878ed1e}') to /net/d2r2/BACKUP/nx/rdiff/Private/ThunderbiSun Sep 30 12:32:14 2007 Exception 'no ownership of /net/d2r2/BACKUP/nx/rdiff/Private/Admin/Admin/Redhat.pdf' raised of class '<type 'exceptions.AssertionError'>':
File "/usr/lib/python2.5/site-packages/rdiff_backup/Main.py", line 302, in error_check_Main
try: Main(arglist)
File "/usr/lib/python2.5/site-packages/rdiff_backup/Main.py", line 322, in Main
take_action(rps)
File "/usr/lib/python2.5/site-packages/rdiff_backup/Main.py", line 278, in take_action
elif action == "backup": Backup(rps[0], rps[1])
File "/usr/lib/python2.5/site-packages/rdiff_backup/Main.py", line 341, in Backup
backup.Mirror_and_increment(rpin, rpout, incdir)
File "/usr/lib/python2.5/site-packages/rdiff_backup/backup.py", line 51, in Mirror_and_increment
DestS.patch_and_increment(dest_rpath, source_diffiter, inc_rpath)
File "/usr/lib/python2.5/site-packages/rdiff_backup/backup.py", line 227, in patch_and_increment
for diff in rorpiter.FillInIter(source_diffiter, dest_rpath):
File "/usr/lib/python2.5/site-packages/rdiff_backup/rorpiter.py", line 169, in FillInIter
first_rp = rpiter.next() # StopIteration gets passed upwards
File "/usr/lib/python2.5/site-packages/rdiff_backup/backup.py", line 103, in get_diffs
for dest_sig in dest_sigiter:
File "/usr/lib/python2.5/site-packages/rdiff_backup/backup.py", line 178, in get_sigs
src_rorp, dest_rorp)
File "/usr/lib/python2.5/site-packages/rdiff_backup/backup.py", line 193, in get_one_sig
sig_fp = cls.get_one_sig_fp(dest_rp)
File "/usr/lib/python2.5/site-packages/rdiff_backup/backup.py", line 209, in get_one_sig_fp
assert dest_rp.isowner(), 'no ownership of %s' % (dest_rp.path,)

rd/extensions/{e9a9c0c3-8673-ae31-8a09-b52878ed1e}
Sun Sep 30 12:32:13 2007 Setting time of /net/d2r2/BACKUP/nx/rdiff/Private/Thunderbird/extensions/{e9a9c0c3-8673-ae31-8a09-b52878ed1e} to 1113044062
Sun Sep 30 12:32:13 2007 Regressing attributes of /net/d2r2/BACKUP/nx/rdiff/Private/Thunderbird/extensions
Sun Sep 30 12:32:13 2007 Copying attributes from ('Thunderbird', 'extensions') to /net/d2r2/BACKUP/nx/rdiff/Private/Thunderbird/extensions
Sun Sep 30 12:32:13 2007 Setting time of /net/d2r2/BACKUP/nx/rdiff/Private/Thunderbird/extensions to 1190921064
Sun Sep 30 12:32:13 2007 Regressing attributes of /net/d2r2/BACKUP/nx/rdiff/Private/Thunderbird
Sun Sep 30 12:32:13 2007 Copying attributes from ('Thunderbird',) to /net/d2r2/BACKUP/nx/rdiff/Private/Thunderbird
Sun Sep 30 12:32:13 2007 Setting time of /net/d2r2/BACKUP/nx/rdiff/Private/Thunderbird to 1191087496
Sun Sep 30 12:32:13 2007 Regressing attributes of /net/d2r2/BACKUP/nx/rdiff/Private
Sun Sep 30 12:32:13 2007 Copying attributes from () to /net/d2r2/BACKUP/nx/rdiff/Private
Sun Sep 30 12:32:13 2007 Setting time of /net/d2r2/BACKUP/nx/rdiff/Private to 1183649797
Sun Sep 30 12:32:13 2007 Deleting /net/d2r2/BACKUP/nx/rdiff/Private/rdiff-backup-data/current_mirror.2007-09-30T11:59:19+02:00.data
Sun Sep 30 12:32:13 2007 Reading globbing filelist /tmp/tmp.VHGbvC3005
Sun Sep 30 12:32:13 2007 Reading globbing filelist /tmp/tmp.qzUhzA3003
Sun Sep 30 12:32:13 2007 Writing mirror marker /net/d2r2/BACKUP/nx/rdiff/Private/rdiff-backup-data/current_mirror.2007-09-30T12:26:53+02:00.data
Sun Sep 30 12:32:13 2007 Starting increment operation /home/laurent/Private to /net/d2r2/BACKUP/nx/rdiff/Private
Traceback (most recent call last):
File "/usr/bin/rdiff-backup", line 23, in <module>
rdiff_backup.Main.error_check_Main(sys.argv[1:])
File "/usr/lib/python2.5/site-packages/rdiff_backup/Main.py", line 302, in error_check_Main
try: Main(arglist)
File "/usr/lib/python2.5/site-packages/rdiff_backup/Main.py", line 322, in Main
take_action(rps)
File "/usr/lib/python2.5/site-packages/rdiff_backup/Main.py", line 278, in take_action
elif action == "backup": Backup(rps[0], rps[1])
File "/usr/lib/python2.5/site-packages/rdiff_backup/Main.py", line 341, in Backup
backup.Mirror_and_increment(rpin, rpout, incdir)
File "/usr/lib/python2.5/site-packages/rdiff_backup/backup.py", line 51, in Mirror_and_increment
DestS.patch_and_increment(dest_rpath, source_diffiter, inc_rpath)
File "/usr/lib/python2.5/site-packages/rdiff_backup/backup.py", line 227, in patch_and_increment
for diff in rorpiter.FillInIter(source_diffiter, dest_rpath):
File "/usr/lib/python2.5/site-packages/rdiff_backup/rorpiter.py", line 169, in FillInIter
first_rp = rpiter.next() # StopIteration gets passed upwards
File "/usr/lib/python2.5/site-packages/rdiff_backup/backup.py", line 103, in get_diffs
for dest_sig in dest_sigiter:
File "/usr/lib/python2.5/site-packages/rdiff_backup/backup.py", line 178, in get_sigs
src_rorp, dest_rorp)
File "/usr/lib/python2.5/site-packages/rdiff_backup/backup.py", line 193, in get_one_sig
sig_fp = cls.get_one_sig_fp(dest_rp)
File "/usr/lib/python2.5/site-packages/rdiff_backup/backup.py", line 209, in get_one_sig_fp
assert dest_rp.isowner(), 'no ownership of %s' % (dest_rp.path,)
AssertionError: no ownership of /net/d2r2/BACKUP/nx/rdiff/Private/Admin/Admin/Redhat.pdf
====>>>>> Error backup rdiff

Anonymous

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #14342:  diff added by mhorowitz (1KiB - application/octet-stream - another possible patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by owsla (Posted a comment)
  • -unavailable- added by mhorowitz (Updated the item)
  • -unavailable- added by mhorowitz (Voted in favor of this item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 1 encouragement so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 8 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon 28 Jan 2008 04:51:32 AM UTCowslaStatusIn Progress=>Fixed
      Open/ClosedOpen=>Closed
    Wed 26 Dec 2007 06:23:20 PM UTCowslaStatusFixed=>In Progress
      Open/ClosedClosed=>Open
    Mon 24 Dec 2007 10:38:05 PM UTCowslaStatusNone=>Fixed
      Open/ClosedOpen=>Closed
    Fri 09 Nov 2007 06:05:09 AM UTCmhorowitzAttached File-=>Added diff, #14342
      Carbon-Copy-=>Added mhorowitz

    Back to the top


    Powered by Savane 3.1-cleanup1