Closed
Bug 672324
Opened 14 years ago
Closed 13 years ago
Fix test for bug 650509 (Other apps can read Firefox profile files)
Categories
(Core :: SQLite and Embedded Database Bindings, defect)
Tracking
()
RESOLVED
FIXED
mozilla10
People
(Reporter: emorley, Assigned: m_kato)
References
Details
Attachments
(1 file, 1 obsolete file)
1.66 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
+++ 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 ‘|’
}
Reporter | ||
Updated•14 years ago
|
Assignee: nobody → blassey.bugs
Assignee | ||
Comment 2•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #563693 -
Flags: review?(sdwilsh)
Reporter | ||
Updated•13 years ago
|
Assignee: blassey.bugs → m_kato
Comment 3•13 years ago
|
||
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?
Assignee | ||
Comment 4•13 years ago
|
||
(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.
Assignee | ||
Comment 5•13 years ago
|
||
Attachment #563693 -
Attachment is obsolete: true
Attachment #570625 -
Flags: review?(mak77)
Attachment #563693 -
Flags: review?(sdwilsh)
Comment 6•13 years ago
|
||
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+
Assignee | ||
Comment 7•13 years ago
|
||
Whiteboard: [inbound]
Comment 8•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla10
Updated•6 months ago
|
Product: Toolkit → Core
You need to log in
before you can comment on or make changes to this bug.
Description
•