Last Comment Bug 375102 - fix RDFXMLDataSourceImpl::rdfXMLFlush() to use safe output streams like nsBookmarksService::WriteBookmarks() to avoid writing a partial localstore.rdf to disk
: fix RDFXMLDataSourceImpl::rdfXMLFlush() to use safe output streams like nsBoo...
Status: RESOLVED FIXED
: fixed1.8.1.5
Product: Core
Classification: Components
Component: RDF (show other bugs)
: 1.8 Branch
: All All
: -- normal (vote)
: mozilla1.8.1
Assigned To: (not reading, please use seth@sspitzer.org instead)
:
Mentors:
: 109706 145558 359472 (view as bug list)
Depends on: 375847 376173
Blocks: profile-corrupt 376136 389136
  Show dependency treegraph
 
Reported: 2007-03-23 10:17 PDT by (not reading, please use seth@sspitzer.org instead)
Modified: 2009-04-28 08:16 PDT (History)
31 users (show)
dveditz: blocking1.8.1.5+
dveditz: blocking1.8.0.13-
jwalden+bmo: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (2.46 KB, patch)
2007-03-23 11:11 PDT, (not reading, please use seth@sspitzer.org instead)
vladimir: review+
Details | Diff | Splinter Review
supplimental fix to revert back to 0666 (1.29 KB, patch)
2007-03-30 09:07 PDT, (not reading, please use seth@sspitzer.org instead)
dveditz: review+
Details | Diff | Splinter Review
patch for the MOZILLA_1_8_BRANCH and the MOZILLA_1_8_0_BRANCH (2.44 KB, patch)
2007-03-31 20:36 PDT, (not reading, please use seth@sspitzer.org instead)
moco: review+
dveditz: approval1.8.1.5+
mscott: approval1.8.0.13-
Details | Diff | Splinter Review

Description (not reading, please use seth@sspitzer.org instead) 2007-03-23 10:17:32 PDT
last night, Juan mentioned that he and Carsten were going to be working finding a way to reproduce bug #319196.  On my end, I did some debugging of the code that writes bookmarks out to disk to check what we did on a restart due to software update.  (I had some naive ideas about what might be going on, but upon debugging the theories I had fell apart.)

But since bug #319196 is also about a corrupt localstore.rdf, I looked at some of that code.  I think we should fix RDFXMLDataSourceImpl::rdfXMLFlush() to use safe output streams like nsBookmarksService::WriteBookmarks() to avoid writing a partial localstore.rdf to disk.

comments?

I'm hoping this might help with bug #319196, but that could just be wishful thinking (which is why I did not comment in that bug.)
Comment 1 (not reading, please use seth@sspitzer.org instead) 2007-03-23 11:11:01 PDT
Created attachment 259423 [details] [diff] [review]
patch

vlad, since you reviewed bug #252053 back in 2004, maybe you could review this as well?
Comment 2 Vladimir Vukicevic [:vlad] [:vladv] 2007-03-24 23:02:28 PDT
Comment on attachment 259423 [details] [diff] [review]
patch

The original code created the file with 0666 permissions (which would then be modified by the user's umask)... the new code always uses 0600.  Is that intentional?

Also, is it necessary to use the buffered output stream?  I thought that the file streams are buffered by default, but I could just be making that up...
Comment 3 (not reading, please use seth@sspitzer.org instead) 2007-03-26 18:22:15 PDT
vlad, thanks for the review.

> The original code created the file with 0666 permissions (which would then be
> modified by the user's umask)... the new code always uses 0600.  Is that
> intentional?

it was intentional, following previous review comments from dveditz.  (See https://bugzilla.mozilla.org/show_bug.cgi?id=364599#c70)

> Also, is it necessary to use the buffered output stream?  I thought that the
> file streams are buffered by default, but I could just be making that up...

I don't think nsFileOutputStream::Write() is buffered.  It will call through to PR_Write(), and then on to the per-platform write call.

http://lxr.mozilla.org/seamonkey/source/netwerk/base/src/nsFileStreams.cpp#409
http://lxr.mozilla.org/seamonkey/source/netwerk/base/src/nsBufferedStreams.cpp#525
http://lxr.mozilla.org/seamonkey/source/nsprpub/pr/src/io/priometh.c#144
Comment 4 Christian :Biesinger (don't email me, ping me on IRC) 2007-03-26 23:14:03 PDT
yeah, file streams aren't buffered.
Comment 5 (not reading, please use seth@sspitzer.org instead) 2007-03-26 23:16:57 PDT
> yeah, file streams aren't buffered.

thanks Christian.

Vlad, give this (and my comment about 0600), can you re-review?
Comment 6 (not reading, please use seth@sspitzer.org instead) 2007-03-27 22:22:30 PDT
fixed on the trunk.

Checking in base/src/nsRDFXMLDataSource.cpp;
/cvsroot/mozilla/rdf/base/src/nsRDFXMLDataSource.cpp,v  <--  nsRDFXMLDataSource.
cpp
new revision: 1.155; previous revision: 1.154
done

thanks to dwitte for his original patch in bug #252053 for bookmarks.

I'm going to mark as blocking?, as I think we want this for 2.0.0.x (not sure about the next 1.5.0.x)
Comment 7 (not reading, please use seth@sspitzer.org instead) 2007-03-28 15:16:21 PDT
david and scott point out that mimeTypes.rdf is (obviously) rdf on disk.

but tbird might use rdf on disk for more than firefox, such as for RSS feed reading.
Comment 8 Daniel Veditz [:dveditz] 2007-03-29 11:11:35 PDT
Probably want this for 1.8.0 as well.
Comment 9 (not reading, please use seth@sspitzer.org instead) 2007-03-29 15:45:52 PDT
Comment on attachment 259423 [details] [diff] [review]
patch

seeking approval for the branches
Comment 10 Scott MacGregor 2007-03-29 15:49:45 PDT
Seth, there's a post in .seamonkey today that suggests this may have caused a regression. Just an FYI.
Comment 11 (not reading, please use seth@sspitzer.org instead) 2007-03-29 15:56:36 PDT
zoinks, thanks scott!
Comment 12 (not reading, please use seth@sspitzer.org instead) 2007-03-29 16:00:02 PDT
here's a link to the post concerning the seamonkey issue.

http://groups.google.com/group/mozilla.dev.apps.seamonkey/browse_thread/thread/466c0039e431d6e5/d85c6b28081f8d5f?lnk=st&q=seamonkey+nightlies+broken+on+linux&rnum=1&hl=en#d85c6b28081f8d5f
Comment 13 Andrew Schultz 2007-03-29 16:19:40 PDT
SeaMonkey has 3 generated RDF files that live in the app's chrome directory (chrome.rdf, stylesheet.rdf and overlayinfo.rdf).  With this change, they are all generated with 0600 permissions.  On linux, the app directory is often owned by root (this improves security), but that means that the files are unreadable by users who run seamonkey and the app crashes.  The situation might be similar on windows if the app was installed with admin privs.  I can't think of anything at this point that we can do other than to generate the files and the manually change the permissions from a script during packaging (or post-install).

I guess my question for this bug is why this method should assume that anything it writes is going into the profile directory?  Things written outside the profile directory should use the user's umask.
Comment 14 (not reading, please use seth@sspitzer.org instead) 2007-03-29 17:24:36 PDT
thanks for the details, andrew.  

I didn't know that SeaMonkey was writing out to the app directory like that.  Yes, this scenario would be a problem on windows, too.  

Even with perms of 0666, you are going to have problems on Vista as your process (firefox) would need elevated privs to write to C:\Program Files.  (Note, when software update does it, updater.exe, we prompt the user to elevate.)

Let me check with dveditz to see what he feels about reverting from 0600 to 0666.  I have to admit it is scary to me that firefox, when running as a non-root user, is writing to the application directory (owned by root.)
Comment 15 Robert Kaiser 2007-03-29 17:35:53 PDT
Actually, we might not necessarily require to write those files, but we probably fail if we find they are there but cannot read them (I don't know the code specifically but that's what I suspect).

I actually wonder why we might want to write files with tight security measures by default. It's a good idea to do that within the profile, I don't understand it when writing files to random locations. we should definately use the user's default permissions (umask) there. Who says that all users of rdfXMLFlush() write security-sensitive, private data?
Comment 16 (not reading, please use seth@sspitzer.org instead) 2007-03-30 09:07:28 PDT
Created attachment 260138 [details] [diff] [review]
supplimental fix to revert back to 0666
Comment 17 Tony Mechelynck [:tonymec] 2007-03-30 09:31:59 PDT
(In reply to comment #16)
> Created an attachment (id=260138) [details]
> supplimental fix to revert back to 0666
> 

Wouldn't 0644 (aka -rw-r--r--) be a better compromise between "read-write by anyone" and "neither read nor write by anyone other than root"?
Comment 18 Andrew Schultz 2007-03-30 11:00:15 PDT
> Wouldn't 0644 (aka -rw-r--r--) be a better compromise between...

The user's umask should handle that.
Comment 19 Daniel Veditz [:dveditz] 2007-03-31 10:51:34 PDT
Comment on attachment 260138 [details] [diff] [review]
supplimental fix to revert back to 0666

r=dveditz in the interest of status quo.
Comment 20 (not reading, please use seth@sspitzer.org instead) 2007-03-31 19:00:17 PDT
supplimental fix landed on the trunk.

Checking in nsRDFXMLDataSource.cpp;
/cvsroot/mozilla/rdf/base/src/nsRDFXMLDataSource.cpp,v  <--  nsRDFXMLDataSource.
cpp
new revision: 1.156; previous revision: 1.155
done

will attach branch versions of the complete patch for 1.8.x and 1.8.0.x 
Comment 21 (not reading, please use seth@sspitzer.org instead) 2007-03-31 20:36:03 PDT
Created attachment 260255 [details] [diff] [review]
patch for the MOZILLA_1_8_BRANCH and the MOZILLA_1_8_0_BRANCH

carrying forward r=vlad,dveditz
Comment 22 Andrew Schultz 2007-04-01 13:45:14 PDT
This also seems to have cause bug 376173, though I do not understand how.
Comment 23 Daniel Veditz [:dveditz] 2007-04-02 10:27:50 PDT
Comment on attachment 260255 [details] [diff] [review]
patch for the MOZILLA_1_8_BRANCH and the MOZILLA_1_8_0_BRANCH

approved for 1.8.0.12 and 1.8.1.4, a=dveditz for release-drivers
Comment 24 (not reading, please use seth@sspitzer.org instead) 2007-04-02 13:28:52 PDT
even though I have branch approval, I'm going to wait until I hear more about bug #376173 from andrew/neil/kairo.  (note, there are details in that bug already.)
Comment 25 Daniel Veditz [:dveditz] 2007-04-24 11:31:14 PDT
At risk given no fix for bug 376173. Since we've suffered this problem a long time we could bump it out if we had to.
Comment 26 Daniel Veditz [:dveditz] 2007-04-30 18:38:45 PDT
Comment on attachment 260255 [details] [diff] [review]
patch for the MOZILLA_1_8_BRANCH and the MOZILLA_1_8_0_BRANCH

Too late for 1.8.1.4
Comment 27 Axel Hecht 2007-05-24 05:29:16 PDT
*** Bug 359472 has been marked as a duplicate of this bug. ***
Comment 28 Daniel Veditz [:dveditz] 2007-06-22 10:35:02 PDT
Comment on attachment 260255 [details] [diff] [review]
patch for the MOZILLA_1_8_BRANCH and the MOZILLA_1_8_0_BRANCH

approved for 1.8.1.5 and 1.8.0.13, a=dveditz for release-drivers
Comment 29 (not reading, please use seth@sspitzer.org instead) 2007-06-26 16:31:48 PDT
Manish from flock writes:

"The patch in https://bugzilla.mozilla.org/attachment.cgi?id=260255 has been in our tree for about a month, with no problems, and before we had a similar patch in there for months.  (see bug #359472)"

Kudos to Ian for finding this (and having a fix) back on 11/2006.
Comment 30 (not reading, please use seth@sspitzer.org instead) 2007-07-04 13:30:03 PDT
fixed landing on the MOZILLA_1_8_BRANCH

Checking in base/src/nsRDFXMLDataSource.cpp;
/cvsroot/mozilla/rdf/base/src/nsRDFXMLDataSource.cpp,v  <--  nsRDFXMLDataSource.
cpp
new revision: 1.153.4.1; previous revision: 1.153
done
Comment 31 (not reading, please use seth@sspitzer.org instead) 2007-07-04 13:41:59 PDT
This fix could help with any firefox 2.0.0.x corruption of

downloads.rdf
extensions.rdf
localstore.rdf
mimeTypes.rdf
search.rdf

And could help tbird with corruption of:

./downloads.rdf
./extensions.rdf
./localstore.rdf
./Mail/News & Blogs/feeditems.rdf
./Mail/News & Blogs/feeds.rdf
./mimeTypes.rdf

But unfortunately, I don't have a test case to reproduce the corruption.
Comment 32 Daniel Veditz [:dveditz] 2007-08-07 11:38:38 PDT
closing the tree for 1.8.0.13 soon, will you be able to land this patch?
Comment 33 (not reading, please use seth@sspitzer.org instead) 2007-08-07 14:06:33 PDT
> closing the tree for 1.8.0.13 soon, will you be able to land this patch?

sent mail to scott and david to see if they want it for the next version of tbird to be built off of that branch.
Comment 34 Scott MacGregor 2007-08-07 16:25:18 PDT
Comment on attachment 260255 [details] [diff] [review]
patch for the MOZILLA_1_8_BRANCH and the MOZILLA_1_8_0_BRANCH

we decided not to take this on the branch so I'm going to minus this.
Comment 35 timeless 2009-04-28 00:12:05 PDT
*** Bug 109706 has been marked as a duplicate of this bug. ***
Comment 36 timeless 2009-04-28 00:12:22 PDT
*** Bug 145558 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.