SeaMonkey downloads and Unix permissions

[Dave Yeo]

Unluckily the code paths are quite complex. Possibly class TypeEaEnumerator could be extended to delete those EAs if I had the skills to do it and if they don't get added somewhere else later.
While in here, I should try to figure out where the .TYPE EA (stores the download URL) is getting eaten. Perhaps we could save it and get rid of the other EAs at the same time.
For now, it seems simpler to just change the default permissions from 0600 to perhaps 0666.

[Andreas Schnellbacher]

Possibly class TypeEaEnumerator could be extended to delete those EAs

There exists eaclean by David A. for that. See https://88watts.net/software.html.

[Dave Yeo]

Possibly class TypeEaEnumerator could be extended to delete those EAs

There exists eaclean by David A. for that. See https://88watts.net/software.html.

Yes, that would be one workaround. class TypeEaEnumerator (written by Rich) is actually in xpcom\io\nsLocalFileOS2.cpp and it seems this stuff should go into that file, it is about 70KB with 2677 lines, perhaps too complex for me right now.
Looking, it seems that it is https://bugzilla.mozilla.org/show_bug.cgi?id=120679 that is the problem and has been since before Mozilla v1 if I understand correctly. As usual, this bug died due to bikeshedding. What I've tried is this,

Code: [Select]

diff --git a/netwerk/base/BackgroundFileSaver.cpp b/netwerk/base/BackgroundFileSaver.cpp
index f3cdccdffb..ea4be39887 100644
--- a/netwerk/base/BackgroundFileSaver.cpp
+++ b/netwerk/base/BackgroundFileSaver.cpp
@@ -623,7 +623,7 @@ BackgroundFileSaver::ProcessStateChange()
   nsCOMPtr<nsIOutputStream> outputStream;
   rv = NS_NewLocalFileOutputStream(getter_AddRefs(outputStream),
                                    mActualTarget,
-                                   PR_WRONLY | creationIoFlags, 0600);
+                                   PR_WRONLY | creationIoFlags, 0666);
   NS_ENSURE_SUCCESS(rv, rv);
 
   outputStream = NS_BufferOutputStream(outputStream, BUFFERED_IO_SIZE);
diff --git a/netwerk/base/nsDownloader.cpp b/netwerk/base/nsDownloader.cpp
index 3e6583ca67..5e4d455cb2 100644
--- a/netwerk/base/nsDownloader.cpp
+++ b/netwerk/base/nsDownloader.cpp
@@ -54,7 +54,7 @@ nsDownloader::OnStartRequest(nsIRequest *request, nsISupports *ctxt)
         rv = location->AppendNative(nsDependentCString(buf, 12));
         if (NS_FAILED(rv)) return rv;
 
-        rv = location->CreateUnique(nsIFile::NORMAL_FILE_TYPE, 0600);
+        rv = location->CreateUnique(nsIFile::NORMAL_FILE_TYPE, 0666);
         if (NS_FAILED(rv)) return rv;
 
         location.swap(mLocation);
diff --git a/netwerk/base/nsIncrementalDownload.cpp b/netwerk/base/nsIncrementalDownload.cpp
index 4250481fe4..cb1a8f4870 100644
--- a/netwerk/base/nsIncrementalDownload.cpp
+++ b/netwerk/base/nsIncrementalDownload.cpp
@@ -43,7 +43,7 @@ static nsresult
 WriteToFile(nsIFile *lf, const char *data, uint32_t len, int32_t flags)
 {
   PRFileDesc *fd;
-  int32_t mode = 0600;
+  int32_t mode = 0666;
   nsresult rv;
 #if defined(MOZ_WIDGET_GONK)
   // The sdcard on a B2G phone looks like:
which here results in (note the latest files)

Code: [Select]

-rw-r--r-- 1 root root    57948 Jan 10 12:58 3DLogicMinesweeper_1-0.zip
-rw------- 1 root root    13239 Jan 10 10:17 Battle_Ship_1-0.zip
-rw-r--r-- 1 root root     7605 Jan 10 12:16 BombSquad_1-0.zip
with random files downloaded from Hobbes.
Here's a lightly tested build, https://drive.google.com/file/d/1P3jWeDzldoUcKXYiv6uJ3kaV3c9-4GTt/view?usp=drivesdk
Please test various ways of downloading and whether these permissions are good enough.

[David McKenna]

 Dave,

  Thanks for producing this new build! Everything I've tried so far produces '-rw-r--r-- root root' when downloaded whether .zip file, .exe file or .txt file. But it doesn't add up - if you are trying to use '0666' to set permissions, that should produce '-rw-rw-rw- root root'. '-rw-r--r--' = 0644, doesn't it? If you are going for a 'one size fits all' approach, maybe 0777 is the way to go to cover any possibility...

Regards,

[Dave Yeo]

Good observation, wonder if something is changing the permissions after being set where I patched? Have to experiment a bit more unless having it universally read-only is good enough. Louis figures 0777 is overkill.
Another question is email attachments.

[Rich Walsh]

wonder if something is changing the permissions after being set where I patched? Have to experiment a bit more unless having it universally read-only is good enough.

0666 is octal - are you sure you're not entering 0x666? The compiler will recognize octal if there's a leading zero (whence '0666'). FWIW... 0666 == 0x1B6.

Louis figures 0777 is overkill.

At the point where you are assigning permissions, doesn't the code know whether this is an executable or not?

Another question is email attachments.

There are no "other questions". This is OS/2: there are no users or groups, there is no security, none of the Unix considerations apply. I can look at and modify any file on my system, regardless of what someone's alien software says., These bogus Unix permissions should reflect that.

[Dave Yeo]

wonder if something is changing the permissions after being set where I patched? Have to experiment a bit more unless having it universally read-only is good enough.

0666 is octal - are you sure you're not entering 0x666? The compiler will recognize octal if there's a leading zero (whence '0666'). FWIW... 0666 == 0x1B6.

No, I was entering octal, just made a bad assumption that somewhere the code was doing a chmod

Louis figures 0777 is overkill.

At the point where you are assigning permissions, doesn't the code know whether this is an executable or not?

No, I tried where it downloaded it and where the final moveto was done. At least now I found where that final moveto is now done (nsDownloadManager.cpp) so maybe can fix your EA .subject code.

Another question is email attachments.

There are no "other questions". This is OS/2: there are no users or groups, there is no security, none of the Unix considerations apply. I can look at and modify any file on my system, regardless of what someone's alien software says., These bogus Unix permissions should reflect that.

Yes, the permissions shouldn't matter, yet as David found, when other systems get involved, they do matter. This would also included accessing HPFS on Linux, which uses the same EA's for permissions.
Anyways for now I added chmod() to nsLocalFileOS2.cpp which seems to work. Perhaps I should try replacing it with Alex's code to delete the permissions as I agree that on OS/2 they're more of a hassle then a help.

[Dave Yeo]

New test build at https://drive.google.com/file/d/1PPfJnqG_-6wgsRZq6A07fs0eHzusDXvs/view?usp=drivesdk.
For this I added chmod() to nsLocalFile::GetPermissionsOfLink() in nsLocalFileOS2.cpp. Might try Alex's suggestion as now I know where to add it I think.

[David McKenna]

 Thanks again Dave! This one seems better - everything is downloaded with -rw-rw-rw- (0666) now. Would be interested if you could try Alex's idea too (which makes sense for OS/2)..

Regards,

[Dave Yeo]

In my QE editor and a few other programs, I simply make sure that new files are created without EAs. 

More precisely, because libc creates these file permission EAs on a new file automatically, my save routines all have some logic to immediately delete them if the file was new. (I don't do that when updating an existing file because the user might actually have set them on purpose.)

e.g.

Code: [Select]

    if ( !fileExists ) {
#ifdef __OS2__
        // If this is a new file, get rid of the useless default EAs added by klibc
        deleteEA( pszFileName, (PCSZ) "UID");
        deleteEA( pszFileName, (PCSZ) "GID");
        deleteEA( pszFileName, (PCSZ) "MODE");
        deleteEA( pszFileName, (PCSZ) "INO");
        deleteEA( pszFileName, (PCSZ) "RDEV");
        deleteEA( pszFileName, (PCSZ) "GEN");
        deleteEA( pszFileName, (PCSZ) "FLAGS");
#endif
    }


Code: [Select]

unsigned long deleteEA( char *pszPathName, const char *pszEAName )
{
    PFEA2LIST pFEA2List = NULL;
    PFEA2     pFEA2 = NULL;
    EAOP2     eaop2 = {0};
    size_t    cb = 0;
    APIRET    rc = ERROR_NOT_ENOUGH_MEMORY;

    cb = sizeof( FEA2LIST ) + strlen( pszEAName );
    pFEA2List = (PFEA2LIST) calloc( cb, 1 );
    if ( pFEA2List ) {
        pFEA2List->cbList = cb;
        pFEA2 = pFEA2List->list;
        pFEA2->cbName = (BYTE) strlen( pszEAName );
        strcpy( pFEA2->szName, pszEAName );
        pFEA2->cbValue = 0;

        eaop2.fpFEA2List = pFEA2List;
        rc = DosSetPathInfo( (PCSZ) pszPathName, FIL_QUERYEASIZE,
                             (PBYTE) &eaop2, sizeof( eaop2 ), DSPI_WRTTHRU );
        free( pFEA2List );
    }
    return (unsigned long) rc;
}


Well, so far I've come up with this,

Code: [Select]

diff --git a/xpcom/io/nsLocalFileOS2.cpp b/xpcom/io/nsLocalFileOS2.cpp
index 32715473bd..bb772a8053 100644
--- a/xpcom/io/nsLocalFileOS2.cpp
+++ b/xpcom/io/nsLocalFileOS2.cpp
@@ -16,6 +16,7 @@
 #include "nsIDirectoryEnumerator.h"
 #include "nsIComponentManager.h"
 #include "prlink.h"
+#include "prmem.h"
 
 #include "nsTraceRefcnt.h"
 #include "nsReadableUtils.h"
@@ -233,6 +234,32 @@ static int isleadbyte(int c)
     return retval;
 }
 
+static nsresult deleteEA( char *pszPathName, char *pszEAName )
+  {
+      PFEA2LIST pFEA2List = NULL;
+      PFEA2     pFEA2 = NULL;
+      EAOP2     eaop2 = {0};
+      size_t    cb = 0;
+      APIRET    rc;
+
+      cb = sizeof( FEA2LIST ) + strlen( pszEAName );
+      pFEA2List = (PFEA2LIST) PR_Calloc( cb, 1 );
+      if ( pFEA2List ) {
+          pFEA2List->cbList = cb;
+          pFEA2 = pFEA2List->list;
+          pFEA2->cbName = (BYTE) strlen( pszEAName );
+          strcpy( pFEA2->szName, pszEAName );
+          pFEA2->cbValue = 0;
+
+          eaop2.fpFEA2List = pFEA2List;
+          rc = DosSetPathInfo( (PCSZ) pszPathName, FIL_QUERYEASIZE,
+                               (PBYTE) &eaop2, sizeof( eaop2 ), DSPI_WRTTHRU );
+          PR_Free( pFEA2List );
+      }
+      if (rc != NO_ERROR)
+        return ConvertOS2Error(rc);
+  }
+
 //-----------------------------------------------------------------------------
 // nsDirEnumerator
 //-----------------------------------------------------------------------------
@@ -1956,6 +1983,15 @@ nsLocalFile::SetPermissions(uint32_t aPermissions)
 
     pathInfo.attrFile ^= FILE_READONLY;
 
+//  Get rid of the useless default EAs added by klibc
+    deleteEA( mWorkingPath.get(), (PCSZ) "UID");
+    deleteEA( mWorkingPath.get(), (PCSZ) "GID");
+    deleteEA( mWorkingPath.get(), (PCSZ) "MODE");
+    deleteEA( mWorkingPath.get(), (PCSZ) "INO");
+    deleteEA( mWorkingPath.get(), (PCSZ) "RDEV");
+    deleteEA( mWorkingPath.get(), (PCSZ) "GEN");
+    deleteEA( mWorkingPath.get(), (PCSZ) "FLAGS");
+
     rc = DosSetPathInfo(mWorkingPath.get(), FIL_STANDARD,
                         &pathInfo, sizeof(pathInfo), 0UL);

But the compiler isn't happy with converting the strings, (also the return type)
...

Code: [Select]

Y:/work/cc45-git/mozilla/xpcom/io/nsLocalFileOS2.cpp:1993:31: error: invalid conversion from 'const char_type*' {aka 'const char*'} to 'char*' [-fpermissive]
 1993 |     deleteEA( mWorkingPath.get(), (PCSZ) "FLAGS");
      |               ~~~~~~~~~~~~~~~~^~
      |                               |
      |                               const char_type* {aka const char*}
Y:/work/cc45-git/mozilla/xpcom/io/nsLocalFileOS2.cpp:237:33: note:   initializing argument 1 of 'nsresult deleteEA(char*, char*)'
  237 | static nsresult deleteEA( char *pszPathName, char *pszEAName )
      |                           ~~~~~~^~~~~~~~~~~
Y:/work/cc45-git/mozilla/xpcom/io/nsLocalFileOS2.cpp:1993:35: warning: ISO C++ forbids converting a string constant to 'char*' [-Wwrite-strings]
 1993 |     deleteEA( mWorkingPath.get(), (PCSZ) "FLAGS");
      |                                   ^~~~~~~~~~~~~~
Y:/work/cc45-git/mozilla/xpcom/io/nsLocalFileOS2.cpp: In function 'nsresult deleteEA(char*, char*)':
Y:/work/cc45-git/mozilla/xpcom/io/nsLocalFileOS2.cpp:261:3: warning: control reaches end of non-void function [-Wreturn-type]
  261 |   }

Ideas?

[Lars]

Change both input params to "const char *", change function logic to ensure that you ALWAYS return a value (currently, you don't).

[Dave Yeo]

Thanks, that worked, except the EA's are still there. Probably need to strip them later when the tmp file gets moved to the permanent name in download.cpp

[Lars]

Thanks, that worked, except the EA's are still there. Probably need to strip them later when the tmp file gets moved to the permanent name in download.cpp

You should also preset the "rc" Variable with a decent value. Just like the initial example from Alexander Taylor.

[Dave Yeo]

Yes, I set it with NO_ERROR. At first I was worried about the scope of rc as it is used throughout the file.  Though thinking about it, I'm not sure what happens if there is an error, whether in the memory allocation or what DosQueryPathInfo() returns, perhaps it should have been passed somehow to the class, nsLocalFile::SetPermissions(uint32_t aPermissions).
https://github.com/bitwiseworks/mozilla-os2/blob/master/xpcom/io/nsLocalFileOS2.cpp about line 1931

[Lars]

Yes, I set it with NO_ERROR. At first I was worried about the scope of rc as it is used throughout the file.  Though thinking about it, I'm not sure what happens if there is an error, whether in the memory allocation or what DosQueryPathInfo() returns, perhaps it should have been passed somehow to the class, nsLocalFile::SetPermissions(uint32_t aPermissions).
https://github.com/bitwiseworks/mozilla-os2/blob/master/xpcom/io/nsLocalFileOS2.cpp about line 1931

In C, if you declare a variable in a block ("{" ... "}"), it is only known in that block (and effectively becomes a stack variable unless you declare it as "static" in which case it will be placed into the default data segment with a "madeup" unique name).
If you declare a variable outside of any block, then it is known throughout the whole source file (module). That way, you can have "rc" multiple times in a file but they are only declared locally.

The routine you are pointing to looks reasonable. Obviously, "NS_IMETHODIMP" is a define for something like "ULONG APIENTRY" or "unsigned long _System" or "unsigned long _cdecl" or such. In other words, a combination of return value and calling convention (you would need to search the header files for it ...).
By the way: "ConvertOS2Error" can also deal with a NO_ERROR = 0 on entry, therefore:

Code: [Select]

    if (rc)
        return ConvertOS2Error(rc);

    return NS_OK;
can just become:

Code: [Select]

     return ConvertOS2Error(rc);