The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla1.8.1

Status

()

Core
RDF
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: (not reading, please use seth@sspitzer.org instead), Assigned: (not reading, please use seth@sspitzer.org instead))

Tracking

(Blocks: 2 bugs, {fixed1.8.1.5})

1.8 Branch
mozilla1.8.1
fixed1.8.1.5
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1.5 +
blocking1.8.0.13 -
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

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.)
Created attachment 259423 [details] [diff] [review]
patch

vlad, since you reviewed bug #252053 back in 2004, maybe you could review this as well?
Attachment #259423 - Flags: review?(vladimir)
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...
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
Assignee: nobody → sspitzer
yeah, file streams aren't buffered.
> yeah, file streams aren't buffered.

thanks Christian.

Vlad, give this (and my comment about 0600), can you re-review?
Status: NEW → ASSIGNED
Attachment #259423 - Flags: review?(vladimir) → review+
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)
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: blocking1.8.1.4?
Resolution: --- → FIXED
Whiteboard: [fixed landed on trunk, wanted for 2.0.0.x, too]
Component: General → RDF
Product: Firefox → Core
QA Contact: general → rdf
Version: 2.0 Branch → 1.8 Branch
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.
Probably want this for 1.8.0 as well.
Flags: blocking1.8.1.4?
Flags: blocking1.8.1.4+
Flags: blocking1.8.0.12+
Comment on attachment 259423 [details] [diff] [review]
patch

seeking approval for the branches
Attachment #259423 - Flags: approval1.8.1.4?
Attachment #259423 - Flags: approval1.8.0.12?

Comment 10

10 years ago
Seth, there's a post in .seamonkey today that suggests this may have caused a regression. Just an FYI.
zoinks, thanks scott!
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

10 years ago
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.
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

10 years ago
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?
Created attachment 260138 [details] [diff] [review]
supplimental fix to revert back to 0666
Attachment #260138 - Flags: review?(dveditz)
Attachment #259423 - Flags: approval1.8.1.4?
Attachment #259423 - Flags: approval1.8.0.12?
(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

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

The user's umask should handle that.
Comment on attachment 260138 [details] [diff] [review]
supplimental fix to revert back to 0666

r=dveditz in the interest of status quo.
Attachment #260138 - Flags: review?(dveditz) → review+
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 
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
Attachment #260255 - Flags: review+
Attachment #260255 - Flags: approval1.8.1.4?
Attachment #260255 - Flags: approval1.8.0.12?
Flags: in-testsuite?
Blocks: 376136

Updated

10 years ago
Depends on: 375847

Updated

10 years ago
Depends on: 376173

Comment 22

10 years ago
This also seems to have cause bug 376173, though I do not understand how.
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
Attachment #260255 - Flags: approval1.8.1.4?
Attachment #260255 - Flags: approval1.8.1.4+
Attachment #260255 - Flags: approval1.8.0.12?
Attachment #260255 - Flags: approval1.8.0.12+
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.)
Whiteboard: [fixed landed on trunk, wanted for 2.0.0.x, too] → [fixed landed on trunk, wanted for 2.0.0.x, waiting on #376173 before landing on the branches]
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.
Whiteboard: [fixed landed on trunk, wanted for 2.0.0.x, waiting on #376173 before landing on the branches] → [at risk][fixed landed on trunk, wanted for 2.0.0.x, waiting on #376173 before landing on the branches]
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
Attachment #260255 - Flags: approval1.8.1.5?
Attachment #260255 - Flags: approval1.8.1.4+
Attachment #260255 - Flags: approval1.8.0.13?
Attachment #260255 - Flags: approval1.8.0.12+
Flags: blocking1.8.1.5?
Flags: blocking1.8.1.4+
Flags: blocking1.8.0.13?
Flags: blocking1.8.0.12+

Updated

10 years ago
Duplicate of this bug: 359472
Flags: blocking1.8.1.5?
Flags: blocking1.8.1.5+
Flags: blocking1.8.0.13?
Flags: blocking1.8.0.13-
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
Attachment #260255 - Flags: approval1.8.1.5?
Attachment #260255 - Flags: approval1.8.1.5+
Attachment #260255 - Flags: approval1.8.0.13?
Attachment #260255 - Flags: approval1.8.0.13+
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.
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
Keywords: fixed1.8.1.5
Whiteboard: [at risk][fixed landed on trunk, wanted for 2.0.0.x, waiting on #376173 before landing on the branches]
Target Milestone: --- → mozilla1.8.1
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.
closing the tree for 1.8.0.13 soon, will you be able to land this patch?
> 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

10 years ago
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.
Attachment #260255 - Flags: approval1.8.0.13+ → approval1.8.0.13-

Updated

10 years ago
Blocks: 389136

Updated

8 years ago
Duplicate of this bug: 109706

Updated

8 years ago
Duplicate of this bug: 145558

Updated

8 years ago
Blocks: 123929
You need to log in before you can comment on or make changes to this bug.