375102 - fix RDFXMLDataSourceImpl::rdfXMLFlush() to use safe output streams like nsBookmarksService::WriteBookmarks() to avoid writing a partial localstore.rdf to disk

Status: RESOLVED FIXED

(Core Graveyard :: RDF, defect)

Description

[(not reading, please use [e-mail] instead)]

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 [e-mail] instead)]

Attachment: patch

vlad, since you reviewed bug #252053 back in 2004, maybe you could review this as well?

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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 [e-mail] instead)]

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)]

yeah, file streams aren't buffered.

Comment 5

[(not reading, please use [e-mail] instead)]

> 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 [e-mail] instead)]

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 [e-mail] instead)]

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]]

Probably want this for 1.8.0 as well.

Comment 9

[(not reading, please use [e-mail] instead)]

Comment on attachment 259423 [details] [diff] [review]
patch

seeking approval for the branches

Comment 10

[Scott MacGregor]

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 [e-mail] instead)]

zoinks, thanks scott!

Comment 12

[(not reading, please use [e-mail] instead)]

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]

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 [e-mail] instead)]

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]

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 [e-mail] instead)]

Attachment: supplimental fix to revert back to 0666

Comment 17

[Tony Mechelynck [:tonymec]]

(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]

> Wouldn't 0644 (aka -rw-r--r--) be a better compromise between...

The user's umask should handle that.

Comment 19

[Daniel Veditz [:dveditz]]

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 [e-mail] instead)]

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 [e-mail] instead)]

Attachment: patch for the MOZILLA_1_8_BRANCH and the MOZILLA_1_8_0_BRANCH

carrying forward r=vlad,dveditz

Comment 22

[Andrew Schultz]

This also seems to have cause bug 376173, though I do not understand how.

Comment 23

[Daniel Veditz [:dveditz]]

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 [e-mail] instead)]

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]]

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]]

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 28

[Daniel Veditz [:dveditz]]

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 [e-mail] instead)]

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 [e-mail] instead)]

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 [e-mail] instead)]

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]]

closing the tree for 1.8.0.13 soon, will you be able to land this patch?

Comment 33

[(not reading, please use [e-mail] instead)]

> 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]

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.