659234 - Remove unused variable |rv| from test_file_perms.cpp:52

Status: RESOLVED FIXED

(Core :: SQLite and Embedded Database Bindings, defect)

Description

[Daniel Holbert [:dholbert]]

New build warning introduced recently in
  http://hg.mozilla.org/mozilla-central/rev/6105fb36f613
{
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 ‘|’
mozilla/storage/test/test_file_perms.cpp:52:12: warning: unused variable ‘rv’
}

We probably don't care about |rv| and can just get rid of it...?

Comment 1

[Daniel Holbert [:dholbert]]

(and might as well add the suggested parentheses while we're at it)

Comment 2

[Ed Morley [:emorley]]

Attachment: Fix build warnings

Comment 3

[Ed Morley [:emorley]]

Attachment: Fix build warnings v1.1

The previous patch failed try: http://tinderbox.mozilla.org/showlog.cgi?log=Try/1308556811.1308559722.5901.gz#err0

TEST-UNEXPECTED-FAIL | /builds/slave/try-lnx64-dbg/build/storage/test/test_file_perms.cpp | Expected true, got false at line 63

Due to this change:
-  do_check_true(perms == PR_IRUSR | PR_IWUSR | PR_IRGRP | PR_IWGRP | PR_IROTH | PR_IWOTH);
+  do_check_true(perms == (PR_IRUSR | PR_IWUSR | PR_IRGRP | PR_IWGRP | PR_IROTH | PR_IWOTH));

Whereas for the logic to remain the same it operator precedence says it should have been:
+  do_check_true( (perms == PR_IRUSR) | PR_IWUSR | PR_IRGRP | PR_IWGRP | PR_IROTH | PR_IWOTH );

I've updated the patch to add the parenthesis but still keep the logic the same as before - however, was the existing code correct? Only comparing PR_IRUSR to perms and not the others doesn't make sense, unless I'm missing something? This file was created in bug 650509, which is still hidden, so not sure what the intention was.

Comment 4

[Ed Morley [:emorley]]

http://dev.philringnalda.com/tbpl/?tree=Try&rev=0f7f8aa6e296

Comment 5

[Ed Morley [:emorley]]

Ping for review / comment 3 question. Thanks :-)

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to comment #3)
> Whereas for the logic to remain the same it operator precedence says it
> should have been:
> +  do_check_true( (perms == PR_IRUSR) | PR_IWUSR | PR_IRGRP | PR_IWGRP |
> PR_IROTH | PR_IWOTH );

I agree with you that that looks busted -- I seriously doubt that's the intended behavior.

> This file was created in bug 650509, which is still hidden, so
> not sure what the intention was.

(I just glanced at the bug -- it doesn't have any more information about this test.)

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

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 540431 [details] [diff] [review]
Fix build warnings v1.1

(If I'm reading correctly, that do_check_true statement is trivially always true.)

Requesting feedback?blassey on the latest (broken-behavior-preserving) patch, to be sure this is on his radar.

Comment 8

[Serge Gautherie (:sgautherie)]

Comment on attachment 540431 [details] [diff] [review]
Fix build warnings v1.1

I concur with previous comments: this patch has to be wrong.

Comment 9

[Serge Gautherie (:sgautherie)]

Comment on attachment 540356 [details] [diff] [review]
Fix build warnings

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

http://mxr.mozilla.org/mozilla-central/search?string=SQLITE_DEFAULT_FILE_PERMISSIONS&case=on

Iiuc, rights are 0644, except on Android where they are 0600.
To be explicit, this could be added as a comment.

::: storage/test/test_file_perms.cpp
@@ +57,5 @@
>  
>    // 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));

(This seems just right to me.)

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

Parens are right, but PR_IWGRP and PR_IWOTH should be removed.

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 540431 [details] [diff] [review]
Fix build warnings v1.1

the problem here (or at least one of them) is that the places.sqlite file doesn't exist in the test profile for unit tests.

The point of this test is to ensure that the permissions of the files containing sensitive data are not too permissive on Android. Its obviously completely busted on other platforms and was only passing due to the incorrect parenthesis.

Does anyone know how to make the unit tests generate a places.sqlite file so it can be tested?

Comment 11

[Shawn Wilsher :sdwilsh]

(I'd have never seen this bug because it was in the wrong component)

Comment 12

[Daniel Holbert [:dholbert]]

(fwiw, the original component choice was to match the bug that added test_file_perms.cpp. The new classification makes more sense, though - thanks for fixing it.)

Comment 13

[Ed Morley [:emorley]]

(In reply to comment #8)
> I concur with previous comments: this patch has to be wrong.

The patch preserves the existing (broken) behaviour, so isn't wrong per se, the original code is.

Anyway, I'm going to morph this bug back to being just about the unused variable build warning and file a new one for the broken behaviour in test_file_perms.cpp, so the work on that doesn't hold this up (and given that I can't really fix the rest).

Comment 14

[Ed Morley [:emorley]]

Attachment: Remove unused variable rv

Updated patch that only removes the unused variable.

The other build warning/sorting out the broken behaviour in this test has been left for (the now reopened) bug 650509.

Comment 15

[Ed Morley [:emorley]]

Shawn, ping for review (one line change that removes unused variable). Thanks :-)

Comment 16

[Ed Morley [:emorley]]

Shawn, ping for one line unused variable removal review.

Comment 17

[Shawn Wilsher :sdwilsh]

Sorry, I had over 1000 bugmail threads to read.  I should get to this review either today or some time this week.

Comment 18

[Shawn Wilsher :sdwilsh]

Comment on attachment 546299 [details] [diff] [review]
Remove unused variable rv

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

So sorry.  This patch used to be more complicated which is why I kept punting on it because it required more time to fact-check.  Clearly, with this version, that isn't needed.  Sorry!

r=sdwilsh

::: storage/test/test_file_perms.cpp
@@ +48,5 @@
>  void
>  test_file_perms()
>  {
>    nsCOMPtr<nsIFile> profDir;
> +  NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, getter_AddRefs(profDir));

nit: Storage style guide says to cast to (void) here since you are not checking the return value.

Comment 19

[Ed Morley [:emorley]]

Attachment: Patch for checkin

No worries :-)

Only change is the addition of the void cast per comment 18 / storage style guide; carrying forwards r+.

Thanks!

Comment 20

[Ed Morley [:emorley]]

Oh and the other problems found in this file were broken out to bug 672324.

Comment 21

[Marco Bonardo [:mak]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/a98208ca4e14

Comment 22

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/a98208ca4e14