Closed Bug 1117580 Opened 9 years ago Closed 9 years ago

nsAtomicFileOutputStream::Init should throw an exception if PR_APPEND flag is passed

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: arai, Assigned: arai)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 3 obsolete files)

nsAtomicFileOutputStream and nsSafeFileOutputStream don't support MODE_APPEND (0x10) flag,
but currently there is no way to know it except reading the implementation.

It should be nice to throw an exception if MODE_APPEND is passed to nsAtomicFileOutputStream::Init
(or at lease show a warning message, if throwing an exception causes compatibility problem).

For example, following code should throw an exception in 'init' method, otherwise the data in the existing file will be lost, despite 'init' with MODE_APPEND succeeds:

  var file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsILocalFile);
  file.initWithPath(pathToExistingFile);
  var ostream = Cc["@mozilla.org/network/safe-file-output-stream;1"].
               createInstance(Ci.nsIFileOutputStream);
  ostream.init (file, 0x02 | 0x08 | 0x10, 0o644, 0);
  ...
  ostream.close();
Yoric, how do you think about this?
Which is better to throw an exception or show a warning message? (or any other idea?)
Flags: needinfo?(dteller)
Sorry, "MODE_APPEND" is used only in FileUtils.jsm, and it's PR_APPEND in nsFileStreams.cpp.
Summary: nsAtomicFileOutputStream::Init should throw an exception if MODE_APPEND flag is passed → nsAtomicFileOutputStream::Init should throw an exception if PR_APPEND flag is passed
I believe that it would indeed be quite reasonable to throw an exception. I just hope that this not going to break existing code.
Flags: needinfo?(dteller)
Unfortunatelly, -1 is passed as aFlags parameter in some places (including comm-central), for example:
  http://dxr.mozilla.org/mozilla-central/source/modules/libpref/Preferences.cpp#962
  http://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsCertOverrideService.cpp#333
  http://dxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgUtils.h#194

Of course, -1 means PR_TRUNCATE is also specified, so it's not required to append to existing file, but it still does not support appending by two or more file handles, since they are writing to different temporary files.

Is -1 flag commonly used in File I/O programming? (so, is -1 likely used in many Add-ons?) If so, it might be better to avoid throwing if aFlags is -1 (or both PR_APPEND and PR_TRUNCATE are specified), to reduce the risk of compatibility problem.
Flags: needinfo?(dteller)
This patch throws if PR_APPEND is passed without PR_TRUNCATE.
So we can throw an exception if PR_APPEND is explicitly specified to append to existing file.

Green on try run:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=1261094eb56d
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=57e719695378
Attachment #8545108 - Flags: feedback?(dteller)
Comment on attachment 8545108 [details] [diff] [review]
Throw an exception if PR_APPEND is passed without PR_TRUNCATE to nsAtomicFileOutputStream::Init.

Review of attachment 8545108 [details] [diff] [review]:
-----------------------------------------------------------------

I have no idea whether `-1` is currently used, but I believe that your check is sound.

::: js/xpconnect/src/xpc.msg
@@ +118,5 @@
>  XPC_MSG_DEF(NS_ERROR_FILE_NOT_FOUND                 , "File error: Not found")
>  XPC_MSG_DEF(NS_ERROR_FILE_READ_ONLY                 , "File error: Read only")
>  XPC_MSG_DEF(NS_ERROR_FILE_DIR_NOT_EMPTY             , "File error: Dir not empty")
>  XPC_MSG_DEF(NS_ERROR_FILE_ACCESS_DENIED             , "File error: Access denied")
> +XPC_MSG_DEF(NS_ERROR_FILE_APPEND_NOT_SUPPORTED      , "File error: PR_APPEND without PR_TRUNCATE is not supported")

I believe that adding a new exception type for this change is a bit overkill. I would use `NS_ERROR_INVALID_ARG`.

::: netwerk/base/src/nsFileStreams.cpp
@@ +869,5 @@
>  nsAtomicFileOutputStream::Init(nsIFile* file, int32_t ioFlags, int32_t perm,
>                               int32_t behaviorFlags)
>  {
> +    if (ioFlags & PR_APPEND && !(ioFlags & PR_TRUNCATE))
> +        return NS_ERROR_FILE_APPEND_NOT_SUPPORTED;

A comment here would be useful. Also, please add curly braces. Also, I believe it would be a little easier to read if you parenthesized as follows:

if ((ioFlags & PR_APPEND) && !(ioFlags & PR_TRUNCATE))
Attachment #8545108 - Flags: feedback?(dteller) → feedback+
Flags: needinfo?(dteller)
Thank you for your feedback :)
Okay, I'd like to land this patch. It prevents passing PR_APPEND explicitly to append to existing file, and I guess this should be sufficient for almost case.

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe29ec160af7
Attachment #8545108 - Attachment is obsolete: true
Attachment #8545756 - Flags: review?(dteller)
Comment on attachment 8545756 [details] [diff] [review]
Throw an exception if PR_APPEND is passed without PR_TRUNCATE to nsAtomicFileOutputStream::Init.

Review of attachment 8545756 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/test/unit/test_safeoutputstream_append.js
@@ +1,5 @@
> +const PR_WRONLY      = 0x02;
> +const PR_CREATE_FILE = 0x08;
> +const PR_APPEND      = 0x10;
> +const PR_TRUNCATE    = 0x20;
> +

Maybe a comment explaining what you're testing?

@@ +9,5 @@
> +  let caught = false;
> +  try {
> +    stream.init(file, flags, 0o644, 0);
> +  } catch (e) {
> +dump(e + "\n");

Generally, don't use `dump`.

@@ +16,5 @@
> +  if (!caught) {
> +    stream.close();
> +  }
> +
> +  do_check_eq(caught, throws);

Please use `Assert.throws`.

@@ +20,5 @@
> +  do_check_eq(caught, throws);
> +}
> +
> +function run_test() {
> +  var filename = "test.txt";

Please use `let` instead of `var`.

@@ +21,5 @@
> +}
> +
> +function run_test() {
> +  var filename = "test.txt";
> +  var file = Cc["@mozilla.org/file/directory_service;1"]

Generally, we use `Services.dirsvc`. You'll need to import Services.jsm.
Attachment #8545756 - Flags: review?(dteller) → feedback+
Thanks again :)
Fixed test code.

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=44ea1ed33bdb
Attachment #8545756 - Attachment is obsolete: true
Attachment #8546977 - Flags: review?(dteller)
Comment on attachment 8546977 [details] [diff] [review]
Throw an exception if PR_APPEND is passed without PR_TRUNCATE to nsAtomicFileOutputStream::Init.

Review of attachment 8546977 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there :)
Also, don't forget to add `;r=yoric` at the end of your commit message.

::: netwerk/test/unit/test_safeoutputstream_append.js
@@ +11,5 @@
> +function check_flag(file, contractID, flags, throws) {
> +  let stream = Cc[contractID].createInstance(Ci.nsIFileOutputStream);
> +
> +  if (throws) {
> +    Assert.throws(() => stream.init(file, flags, 0o644, 0));

Assert.throws supports a regexp as second argument, to make sure that we are throwing the correct exception. Could you use it?

@@ +13,5 @@
> +
> +  if (throws) {
> +    Assert.throws(() => stream.init(file, flags, 0o644, 0));
> +  }
> +  else {

Nit: `} else {`
Attachment #8546977 - Flags: review?(dteller) → feedback+
Thank you again!

(In reply to David Rajchenbach-Teller [:Yoric] (hard to reach until January 10th - use "needinfo") from comment #10)
> Comment on attachment 8546977 [details] [diff] [review]
> Throw an exception if PR_APPEND is passed without PR_TRUNCATE to
> nsAtomicFileOutputStream::Init.
> 
> Review of attachment 8546977 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Almost there :)
> Also, don't forget to add `;r=yoric` at the end of your commit message.

Oops, I thought that I should add it after the patch got r+ed.

> ::: netwerk/test/unit/test_safeoutputstream_append.js
> @@ +11,5 @@
> > +function check_flag(file, contractID, flags, throws) {
> > +  let stream = Cc[contractID].createInstance(Ci.nsIFileOutputStream);
> > +
> > +  if (throws) {
> > +    Assert.throws(() => stream.init(file, flags, 0o644, 0));
> 
> Assert.throws supports a regexp as second argument, to make sure that we are
> throwing the correct exception. Could you use it?

Thank you for letting me know that, it's a cool feature :)

Green on try run: https://tbpl.mozilla.org/?tree=Try&rev=5a4e31a304bb
Attachment #8546977 - Attachment is obsolete: true
Attachment #8548959 - Flags: review?(dteller)
Comment on attachment 8548959 [details] [diff] [review]
Throw an exception if PR_APPEND is passed without PR_TRUNCATE to nsAtomicFileOutputStream::Init. r=yoric

Review of attachment 8548959 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks for the patch.
Attachment #8548959 - Flags: review?(dteller) → review+
https://hg.mozilla.org/mozilla-central/rev/51f6b5ca2e47
Assignee: nobody → arai_a
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.