bugPSPP - Bugs: bug #21280, gui truncates system files.

 
 

bug #21280: gui truncates system files.

Submitted by:  John Darrington <jmd>
Submitted on:  Sun 07 Oct 2007 09:26:23 AM UTC  
 
Category:  Graphical User Interface Severity:  7 - Major
Status:  Fixed Assigned to:  Ben Pfaff <blp>
Open/Closed:  Closed Release:  Before 0.6.0
Effort:  0.00

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)

Sat 03 Nov 2007 10:00:20 PM UTC, comment #17: 

>This seems to work fine.


Thanks, I checked it in.

Ben Pfaff <blp>
Project AdministratorIn charge of this item.
Sat 03 Nov 2007 08:49:59 PM UTC, comment #16: 

>The only concern I have is that there are potential localisation
>problems.


Definitely.  I cross-posted a question about this to bug-gnu-gettext and pspp-dev.  Maybe we'll get some good suggestions.

Ben Pfaff <blp>
Project AdministratorIn charge of this item.
Sat 03 Nov 2007 07:58:39 AM UTC, comment #15: 

This seems to work fine.

The only concern I have is that there are potential localisation problems.  Constructs like

msg (SE, _("Can't read from %s as a %s because it is "
                       "already being read as a %s."),
                 fh_get_name (h), type, a->type);

will cause complaints from translators, because it may well prove to be impossible to form correct sentences in their language.
In particular, languages which have noun gender will balk at the English "a %s", because the pronoun  has to agree with the noun, but the translator can't possibly know what the noun will be.

John Darrington <jmd>
Project Administrator
Sat 03 Nov 2007 02:28:56 AM UTC, comment #14: 

I noticed that I posted a slightly old version of overwrite-input-file-allow.patch.  Here is an updated version.

(file #14285)

Ben Pfaff <blp>
Project AdministratorIn charge of this item.
Fri 02 Nov 2007 05:54:07 AM UTC, comment #13: 

OK, at long last, here is what should be a real fix.

The first patch, overwrite-input-file-allow.patch, is the straightforward part: make reading and writing separate for locking and do atomic replacement of files.

Unfortunately, this doesn't really work, because the file handle architecture is based on inode number, with the assumption that a file won't get deleted and then a new file created under a different name with the same inode number.  This is a bad assumption, and at least on my system with an ext3 file system it causes the match-files.sh test to consistently fail.

Thus, the second patch and much larger patch, patches/identity-stability.patch, becomes necessary.  In this patch, we drop the association between a file handle and an inode number.  Instead, a file handle just represents a file name, and we do inode number lookup and locking on-demand using a separate table of new "struct fh_lock" structures.  This fixes the problem and allows everything to pass.

Comments?

(file #14275, file #14276)

Ben Pfaff <blp>
Project AdministratorIn charge of this item.
Sun 21 Oct 2007 06:00:02 AM UTC, comment #12: 

I now have a fix for this problem.  Unfortunately it breaks other things.  I'm working on a fix for that :-)

Ben Pfaff <blp>
Project AdministratorIn charge of this item.
Sat 13 Oct 2007 05:07:58 AM UTC, comment #11: 

>You're right. It probably won't gain us anything. And it would be
>an uncommon situation anyway.


OK.  The solution should be straightforward.  I will work on it.

Ben Pfaff <blp>
Project AdministratorIn charge of this item.
Fri 12 Oct 2007 10:03:46 AM UTC, comment #10: 

You're right.  It probably won't gain us anything. And it would be an uncommon situation anyway.

John Darrington <jmd>
Project Administrator
Thu 11 Oct 2007 03:41:31 PM UTC, comment #9: 

>There's no guarantee however that you'll have permission to create
>arbitrary files in the directory of the input file. Perhaps we
>should fall back to /tmp, if the first attempt fails, and hope
>that it's on the same filesystem.


Does that solve anything?  To create a new file we need write permission in the new file's directory.  To rename over an existing file, we need write permission in the existing file's directory.  No?

The only way we could avoid needing write permission in the file's directory, I think, is by opening the existing file and truncating it to length 0.  But then we'd lose the contents that we need to read.  I guess we could copy the file's contents to a temporary file and then truncate and overwrite it, but I am not sure that that is really a winner for performance.

I do not think it is a common situation to want to write to a file in a directory in which one does not have write permission.  Do you have a scenario in mind?

Ben Pfaff <blp>
Project AdministratorIn charge of this item.
Thu 11 Oct 2007 10:30:48 AM UTC, comment #8: 

It sounds plausible.  There's no guarantee however that you'll have permission to create arbitrary files in the directory of the input file.  Perhaps we should fall back to /tmp, if the first attempt fails, and hope that it's on the same filesystem.

John Darrington <jmd>
Project Administrator
Wed 10 Oct 2007 03:29:44 PM UTC, comment #7: 

OK.  This is do-able.  There is an issue to be resolved, however: when does the file get replaced?  This comes up when an error occurs while the operation is running.  In that case, one would hope that the original file would not be deleted.

One solution is: when a file is opened for writing, create it under a temporary name other than the one that the user chose.  Then, when the file is closed, rename it to the final name.  (The temporary and final files should be in the same directory to ensure that rename does not fail because it is a cross-file system operation.)

Does this sound reasonable to you?

Ben Pfaff <blp>
Project AdministratorIn charge of this item.
Wed 10 Oct 2007 10:16:44 AM UTC, comment #6: 

The situation is ....

System files no longer get truncated.  This is good.

Your test shows that the system behaves as you intend, but not as I expected.  I expected the syntax

 get file='foo.sav'.
 save outfile='foo.sav'.

to run without error, whereas your test expects it to produce an error.  This syntax runs without error under spss as does the slightly more useful example:

 get file='foo.sav'.
 compute y = x.
 save outfile='foo.sav'.

In any case, I imagine that most gui users would expect to be able to: open a file, add some data and/or variables, click "Save" and have the new data written back to the existing file without error.

John Darrington <jmd>
Project Administrator
Tue 09 Oct 2007 01:20:27 PM UTC, comment #5: 

>A simple syntax...where foo.sav is an existant non-empty system
>file shows there is still a problem.


I can't reproduce the problem by hand and, furthermore, the test that I checked in that tests for it passes for me (as long as I chmod +x by hand).

Ben Pfaff <blp>
Project AdministratorIn charge of this item.
Tue 09 Oct 2007 12:52:24 PM UTC, comment #4: 

>It seems to do the business.


I have to withdraw that comment.

A simple syntax

 GET /FILE='foo.sav'.
 SAVE /OUTFILE='foo.sav'.

where foo.sav is an existant non-empty system file shows there is still a problem.

John Darrington <jmd>
Project Administrator
Tue 09 Oct 2007 03:50:21 AM UTC, comment #3: 

>It seems to do the business.


Glad to hear it.  I checked this in.

Ben Pfaff <blp>
Project AdministratorIn charge of this item.
Mon 08 Oct 2007 11:08:23 AM UTC, comment #2: 

Thanks Ben.

It seems to do the business.

John Darrington <jmd>
Project Administrator
Mon 08 Oct 2007 01:04:49 AM UTC, comment #1: 

It's actually a bug in the system file and portable file writers.  Here's a fix and a test.

(file #14112)

Ben Pfaff <blp>
Project AdministratorIn charge of this item.
Sun 07 Oct 2007 09:26:23 AM UTC, original submission:  

1. Open an existing *.sav file.

2. Modify it.

3. Click Save.

You'll get a error message "Can't open file x for writing because it's already open for reading".  And then the file has a length of zero.

John Darrington <jmd>
Project Administrator

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #14276:  identity-stability.patch added by blp (64KiB - text/x-diff)
file #14112:  overwrite-input-file.patch added by blp (10KiB - text/x-diff)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by blp (Updated the item)
  • -email is unavailable- added by jmd (Submitted the item)
  •  

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

    Only logged-in users can vote.

     

     

     

    Follow 15 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2007-11-03 blp StatusReady for Test/Review => Fixed
        Open/ClosedOpen => Closed
    2007-11-03 blp Attached File- => Added overwrite-input-file-allow.patch, #14285
    2007-11-03 blp Attached File#14275 => Removed
    2007-11-02 blp Attached File- => Added overwrite-input-file-allow.patch, #14275
        Attached File- => Added identity-stability.patch, #14276
        StatusIn Progress => Ready for Test/Review
    2007-10-10 jmd StatusFixed => In Progress
        Assigned toNone => blp
    2007-10-09 jmd Open/ClosedClosed => Open
    2007-10-09 blp StatusReady for Test/Review => Fixed
        Open/ClosedOpen => Closed
    2007-10-08 blp Attached File- => Added overwrite-input-file.patch, #14112
        StatusNone => Ready for Test/Review
    2007-10-07 jmd ReleaseNone => Before 0.6.0

    Back to the top


    Powered by Savane 3.5