Closed Bug 672324 Opened 9 years ago Closed 8 years ago

Fix test for bug 650509 (Other apps can read Firefox profile files)

Categories

(Toolkit :: Storage, defect, major)

ARM
Android
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: emorley, Assigned: m_kato)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

+++ This bug was initially created as a clone of Bug #650509 +++

Broken out from bug 650509 per request.

Comment on attachment 532229 [details] [diff] [review]
>+  // This reflexts the permissions defined by SQLITE_DEFAULT_FILE_PERMISSIONS in
>+  // db/sqlite3/src/Makefile.in and must be kept in sync with that
>+#ifdef ANDROID
>+  do_check_true(perms == PR_IRUSR | PR_IWUSR);
>+#else
>+  do_check_true(perms == PR_IRUSR | PR_IWUSR | PR_IRGRP | PR_IWGRP | PR_IROTH | PR_IWOTH);
>+#endif

Whilst fixing test_file_perms.cpp build warnings in bug 659234, it was noticed that the do_check_true lines above were incorrect.

From bug 659234 comment 6...
> I agree with you that that looks busted -- I seriously doubt that's the
> intended behavior.
>
> blassey, mind taking a look at this?  It looks to me like there's a bug in
> the code and/or the test (in which case bug 650509 may not actually be
> fixed, or at least not tested sufficiently...)

See also bug 659234 comment 9 and bug 659234 comment 10.

At the same time as fixing the test, can the following build warning please be dealt with (if correcting the test behaviour doesn't sort it already):
{
mozilla/storage/test/test_file_perms.cpp: In function ‘void test_file_perms()’:
mozilla/storage/test/test_file_perms.cpp:64:44: warning: suggest parentheses around comparison in operand of ‘|’
}
Assignee: nobody → blassey.bugs
Blocks: 680552
Duplicate of this bug: 690693
Attached patch fix (obsolete) — Splinter Review
Attachment #563693 - Flags: review?(sdwilsh)
Assignee: blassey.bugs → m_kato
Comment on attachment 563693 [details] [diff] [review]
fix

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

::: storage/test/test_file_perms.cpp
@@ +54,4 @@
>    nsCOMPtr<nsIFile> profDir;
>    (void)NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, getter_AddRefs(profDir));
>    nsCOMPtr<nsILocalFile> sqlite_file = do_QueryInterface(profDir);
> +  sqlite_file->Append(NS_LITERAL_STRING("storage_test_db.sqlite"));

I think here you should rather use db->GetDatabaseFile and QI to nsILocalFile

@@ +65,2 @@
>  #else
> +  do_check_true(perms == (PR_IRUSR | PR_IWUSR | PR_IRGRP | PR_IWGRP | PR_IROTH | PR_IWOTH));

are PR_IWGRP and PR_IWOTH correct on other platforms? did you check that on Try?
I think the original patch changed premissions only on Android?
(In reply to Marco Bonardo [:mak] (Away 31Oct-1Nov) from comment #3)
> Comment on attachment 563693 [details] [diff] [review] [diff] [details] [review]
> fix
> 
> Review of attachment 563693 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: storage/test/test_file_perms.cpp
> @@ +54,4 @@
> >    nsCOMPtr<nsIFile> profDir;
> >    (void)NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, getter_AddRefs(profDir));
> >    nsCOMPtr<nsILocalFile> sqlite_file = do_QueryInterface(profDir);
> > +  sqlite_file->Append(NS_LITERAL_STRING("storage_test_db.sqlite"));
> 
> I think here you should rather use db->GetDatabaseFile and QI to nsILocalFile
> 
> @@ +65,2 @@
> >  #else
> > +  do_check_true(perms == (PR_IRUSR | PR_IWUSR | PR_IRGRP | PR_IWGRP | PR_IROTH | PR_IWOTH));
> 
> are PR_IWGRP and PR_IWOTH correct on other platforms? did you check that on
> Try?
> I think the original patch changed premissions only on Android?

Ahh, I should not believe original test case.  Although this will work on Android and Windows only, I don't test on Linux.  On Linux, it will be failure.
Attached patch fix v2Splinter Review
Attachment #563693 - Attachment is obsolete: true
Attachment #570625 - Flags: review?(mak77)
Attachment #563693 - Flags: review?(sdwilsh)
Comment on attachment 570625 [details] [diff] [review]
fix v2

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

::: storage/test/test_file_perms.cpp
@@ +51,5 @@
> +  nsCOMPtr<mozIStorageConnection> db(getDatabase());
> +  nsCOMPtr<nsIFile> dbFile;
> +  do_check_success(db->GetDatabaseFile(getter_AddRefs(dbFile)));
> +
> +  nsCOMPtr<nsILocalFile> sqlite_file = do_QueryInterface(dbFile);

nit: you may sanity check_true sqlite_file to ensure QI was successful.

@@ +56,2 @@
>    PRUint32 perms = 0;
> +  do_check_success(sqlite_file->GetPermissions(&perms));

nit: since there is a dbFile var, this should probably be sqliteFile, for var naming consistency. Btw, I'd probably just use "file" and "localFile", we are not preserving blame regardless of changing var names.
Attachment #570625 - Flags: review?(mak77) → review+
https://hg.mozilla.org/mozilla-central/rev/ab09b7c7a7a8
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla10
You need to log in before you can comment on or make changes to this bug.