1427276 - sdb_init fails if the database path contains a non-ASCII character

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Masatoshi Kimura [:emk]]

SQLite will expect that database file paths are always encoded in UTF-8[1] regardless of the current system locale while PR_Access uses ANSI encoding. So sdb_init will always fail if the profile path contains a non-ASCII character.[2]

[1] https://sqlite.org/c3ref/open.html
[2] https://dxr.mozilla.org/mozilla-central/rev/f5a1cb52c12e8fbcf2e3b5a675fe2a84d43507a7/security/nss/lib/softoken/sdb.c#1742,1747

Comment 1

[Masatoshi Kimura [:emk]]

Attachment: Fix sdb to handle UTF-8 paths correctly on Windows

I can't request review on Phabricator because I do not set up 2FA yet. Hopefully dome NSS developers will take this...

Comment 2

[Masatoshi Kimura [:emk]]

Attachment: Fix NSS sdb to handle UTF-8 paths correctly on Windows

A local proof-of-concept change to NSS.

Comment 3

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8939135 [details] [diff] [review]
Fix sdb to handle UTF-8 paths correctly on Windows

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

While this looks like it would work it needs a test to make sure it actually does work.
I suggest extending softoken_gtest.cc to do so.

::: lib/softoken/sdb.c
@@ +199,5 @@
> +/*
> + * Covert a UTF8 string to Unicode wide character
> + */
> +static LPWSTR
> +sdb_win32_UTF8ToWide(char *buf)

make this const char* and cast away the const later.

@@ +203,5 @@
> +sdb_win32_UTF8ToWide(char *buf)
> +{
> +    DWORD size;
> +    LPWSTR wide;
> +

check that buf != NULL

@@ +204,5 @@
> +{
> +    DWORD size;
> +    LPWSTR wide;
> +
> +    if ((char *)NULL == buf) {

No need to cast

@@ +212,5 @@
> +    size = MultiByteToWideChar(CP_UTF8, 0, buf, -1, NULL, 0);
> +    if (size == 0) {
> +        return (LPWSTR)NULL;
> +    }
> +    wide = PORT_Alloc(sizeof(WCHAR) * size);

Check wide for NULL

@@ +222,5 @@
> +    return wide;
> +}
> +
> +static int
> +sdb_win32_access(const char *path, int mode)

Result should be PRStatus

@@ +225,5 @@
> +static int
> +sdb_win32_access(const char *path, int mode)
> +{
> +    int result;
> +

Check path for NULL

@@ +240,5 @@
> +static int
> +sdb_win32_chmod(const char *filename, int pmode)
> +{
> +    int result;
> +

Check filename for NULL

Comment 4

[Masatoshi Kimura [:emk]]

Attachment: Fix sdb to handle UTF-8 paths correctly on Windows

Fixed review comments and test failures.

Try run:
https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=b9782f923142911434320624cae1e3c868371f1d

Comment 5

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8940397 [details] [diff] [review]
Fix sdb to handle UTF-8 paths correctly on Windows

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

Looks generally good, but I'd like to avoid code duplication (see inline comments).

::: gtests/softoken_gtest/softoken_gtest.cc
@@ +45,5 @@
>    static void GenerateRandomName(/*in/out*/ std::string &prefix);
>    static bool TryMakingDirectory(/*in/out*/ std::string &prefix);
>  
>    std::string mPath;
> +  std::string mSQLPath;

maybe rename this something like utf8Path, this name is a little confusing

::: lib/softoken/sdb.c
@@ +196,5 @@
> + * sqlite3 expects.
> + */
> +
> +/*
> + * Covert a UTF8 string to Unicode wide character

typo Convert

@@ +205,5 @@
> +    DWORD size;
> +    LPWSTR wide;
> +
> +    if (!buf) {
> +        return (LPWSTR)NULL;

No need to cast NULL returns

::: lib/softoken/sftkdb.c
@@ +2523,5 @@
> +{
> +    wchar_t *confdirWide;
> +    DWORD size;
> +    char *nconfdir;
> +    BOOL unmappable;

Where's BOOL defined? WideCharToMultiByte takes LPBOOL.

@@ +2533,5 @@
> +    if (!confdirWide) {
> +        return NULL;
> +    }
> +
> +    size = WideCharToMultiByte(CP_ACP, WC_NO_BEST_FIT_CHARS, confdirWide, -1,

We probably need some includes here.

"Starting with Windows 8: WideCharToMultiByte is declared in Stringapiset.h. Before Windows 8, it was declared in Winnls.h."

@@ +2690,5 @@
>              } else if (newInit) {
>                  /* if the new format DB was also a newly created DB, and we
>                   * succeeded, then need to update that new database with data
>                   * from the existing legacy DB */
> +                if ((nconfdir = sftk_legacyPathFromSDBPath(confdir)) &&

please move the nconfdir assignment out of the if (same above)

::: lib/util/utilmod.c
@@ +42,5 @@
>  #define os_open_permissions_default _S_IREAD | _S_IWRITE
>  #define os_stat_type struct _stat
> +
> +/*
> + * Covert a UTF8 string to Unicode wide character

Convert

@@ +45,5 @@
> +/*
> + * Covert a UTF8 string to Unicode wide character
> + */
> +static LPWSTR
> +nssutil_UTF8ToWide(const char *buf)

These functions are copies from sdb.c. You can't use the sdb.c code in here but I think you should be able to use the code from here in sdb.c. I'd like to avoid duplicating this code.
Ideally all this code should go into NSPR then none of this would be necessary. But I leave it up to you if you want to patch NSS or NSPR.

Comment 6

[Masatoshi Kimura [:emk]]

Attachment: Fix sdb to handle UTF-8 paths correctly on Windows

Comment 7

[Masatoshi Kimura [:emk]]

Comment on attachment 8940858 [details] [diff] [review]
Fix sdb to handle UTF-8 paths correctly on Windows

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #5)
> ::: lib/softoken/sftkdb.c
> @@ +2523,5 @@
> > +{
> > +    wchar_t *confdirWide;
> > +    DWORD size;
> > +    char *nconfdir;
> > +    BOOL unmappable;
> 
> Where's BOOL defined? WideCharToMultiByte takes LPBOOL.

LPBOOL is defined as BOOL*. This parameter takes an address of a BOOL variable.

> @@ +2533,5 @@
> > +    if (!confdirWide) {
> > +        return NULL;
> > +    }
> > +
> > +    size = WideCharToMultiByte(CP_ACP, WC_NO_BEST_FIT_CHARS, confdirWide, -1,
> 
> We probably need some includes here.
> 
> "Starting with Windows 8: WideCharToMultiByte is declared in Stringapiset.h.
> Before Windows 8, it was declared in Winnls.h."

These files are automatically included from <windows.h>.

Other points are addressed.

Comment 8

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8940858 [details] [diff] [review]
Fix sdb to handle UTF-8 paths correctly on Windows

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

lgtm with the two nits fixed

::: gtests/softoken_gtest/softoken_gtest.cc
@@ +37,5 @@
>    // NB: the directory must be empty upon destruction
>    ~ScopedUniqueDirectory() { assert(rmdir(mPath.c_str()) == 0); }
>  
>    const std::string &GetPath() { return mPath; }
> +  const std::string &GetSQLPath() { return mUTF8Path; }

GetUTF8Path

@@ +66,5 @@
>      }
>    }
>    assert(mPath.length() > 0);
> +#if defined(_WIN32)
> +  // sqldb always uses UTF-8 regradless of the current system locale.

regardless

::: lib/util/utilmod.c
@@ +51,5 @@
> +    DWORD size;
> +    LPWSTR wide;
> +
> +    if (!buf) {
> +        return (LPWSTR)NULL;

no need to cast NULL returns

Comment 9

[Masatoshi Kimura [:emk]]

Attachment: Fix sdb to handle UTF-8 paths correctly on Windows

Comment 10

[Masatoshi Kimura [:emk]]

Fixed nits. I have no commit priv. on the nss repo. Please push the change to the nss repo. (3.35 and trunk)

Comment 11

[Franziskus Kiefer [:franziskus]]

trunk: https://hg.mozilla.org/projects/nss/rev/54954c2668f5

Comment 12

[Kai Engert (:KaiE:)]

3.35 branch: https://hg.mozilla.org/projects/nss/rev/e34893b446f9

Comment 13

[Kai Engert (:KaiE:)]

uplift to mozilla-inbound will be done in bug 1420060