We have to use either 1. nsIFile::Load if nsIFile is available, 2. PR_LoadLibraryWithFlags with PR_LibSpec_PathnameU specified, or 3. Or operating system LoadLibrary(Ex)W directly if it is a platform-depedent code. Otherwise Unicode file paths will break on Windows. See bug 1330529 about a live example.

We should also disallow: 1. PR_LibSpec_Pathname (but note that PR_LibSpec_PathnameU is Windows-specific), 2. LoadLibraryA and LoadLibraryExA, and 3. Unsuffixed LoadLibrary and LoadLibraryEx.

So let me see if i get this streight: On Windows platform disallow: PR_LibSpec_Pathname On All platforms disallow: LoadLibraryA, LoadLibraryExA, LoadLibrary, LoadLibraryEx. If this is the case, the implementation is pretty straight forward, it can be done exclusively from the matcher perspective.

(In reply to Andi-Bogdan Postelnicu [:andi] from comment #2) > So let me see if i get this streight: > > On Windows platform disallow: PR_LibSpec_Pathname and PR_LoadLibrary. > On All platforms disallow: LoadLibraryA, LoadLibraryExA, LoadLibrary, > LoadLibraryEx. LoadLibraryA, LoadLibraryExA, LoadLibrary, LoadLibraryEx are all Windows-specific, but it would not hurt even if they are disabled on all platforms.

I've dded this revision as a wip, it only needs tests that i will add after 1443438 lands. If you want to test please do on windows otherwise you will get a bounch of fp regarding PR_LoadLibrary. If you wish to test it please apply first the patch from 1443438, since the current patch depends on that. The dependency will be resolved after 1443438 lands.

Would be great to have a test for that!

How to test this patch? No error happens with this patch (and the patch from 1443438) applied, but I don't think static analysis is running.

Please see here: https://developer.mozilla.org/en-US/docs/Mozilla/Testing/Clang_static_analysis You’re interested in build phase static analysis. If you have issues setting up the env let me know.

As 1443438 has been merged into m-c you no longer need the patch from the respective bug if you do a 'hg pull -u'. I will restrict thought this patch on windows since the current analysis if ran on anything different than windows will trigger a bunch of FP due to PR_LoadLibrary.

Right now i'm doing a build on windows with the clang-plugin and this patch activated to see how may occurrences we have in our code. I'm gonna post the result here.

First thing that triggered the checker was this definition: https://dxr.mozilla.org/mozilla-central/source/js/src/vtune/ittnotify_config.h#279 >>#if ITT_PLATFORM==ITT_PLATFORM_WIN >>#define __itt_get_proc(lib, name) GetProcAddress(lib, name) >>#define __itt_mutex_init(mutex) InitializeCriticalSection(mutex) >>#define __itt_mutex_lock(mutex) EnterCriticalSection(mutex) >>#define __itt_mutex_unlock(mutex) LeaveCriticalSection(mutex) >>#define __itt_load_lib(name) LoadLibraryA(name) >>#define __itt_unload_lib(handle) FreeLibrary(handle) >>#define __itt_system_error() (int)GetLastError() >>#define __itt_fstrcmp(s1, s2) lstrcmpA(s1, s2) >>#define __itt_fstrnlen(s, l) strnlen_s(s, l) >>#define __itt_fstrcpyn(s1, b, s2, l) strncpy_s(s1, b, s2, l) >>#define __itt_fstrdup(s) _strdup(s) >>#define __itt_thread_id() GetCurrentThreadId() >>#define __itt_thread_yield() SwitchToThread() And it gets called from: https://dxr.mozilla.org/mozilla-central/source/js/src/vtune/ittnotify_static.c#1125 >> if (DL_SYMBOLS && (groups != __itt_group_none || lib_name != NULL)) >> { >> _N_(_ittapi_global).lib = __itt_load_lib((lib_name == NULL) ? ittnotify_lib_name : lib_name); >> >> if (_N_(_ittapi_global).lib != NULL) I wonder why on windows is specified to be Ascii and not Wide char. Sean can you please shed some light here?

These are the results of running the current static analysis on our tree. Can you please take a look and tell me what do you think. Are these potential issues legit and how should we move forward with the fixes?

(In reply to Andi-Bogdan Postelnicu [:andi] from comment #14) > These are the results of running the current static analysis on our tree. > Can you please take a look and tell me what do you think. Are these > potential issues legit and how should we move forward with the fixes? I'm working on bug 1442275 to fix most issues, but I won't change third-party code and upstream the change. Is it possible to whitelist some directories?

Sure, but if we whitelist some directories other checker will no check into that directories. But where do you see issues detected by this checker in 3rd party code?

Hm, I could not find third-party code in the result. Is it possible to allow something like LoadLibraryA(<ASCII string literal>)? It should be safe.

I though that we should blacklist LoadLibraryA, but we should permit that only when it calls with an ASCII String literal like: LoadLibraryA("Some Path")?

Exactly.

(In reply to Masatoshi Kimura [:emk] from comment #19) > Exactly. Sure I'm gonna update the patch, do you want to do this only for LoadLibraryA? Do you still this be the case when you call LoadLibrary when UNICODE is not defined? Since in this case the underlying function that gets called it will also be LoadLibraryA

Same for all non-Unicode functions (PR_LoadLibrary, LoadLibraryA, LoadLibraryExA and LoadLibrary and LoadLibraryEx without UNICODE) defined.

(In reply to Masatoshi Kimura [:emk] from comment #21) > Same for all non-Unicode functions (PR_LoadLibrary, LoadLibraryA, > LoadLibraryExA and LoadLibrary and LoadLibraryEx without UNICODE) defined. I've updated the patch with your suggestions. Do you manage to run the analysis or do you want me to run it?

I can not yet run the analysis locally. I'd appreciate if you run it.

(In reply to Andi-Bogdan Postelnicu [:andi] from comment #13) > I wonder why on windows is specified to be Ascii and not Wide char. > > Sean can you please shed some light here? I really don't know. These files were provided by Intel, and we imported them without modification or review. They are used for allowing Firefox's JS JIT to be profiled by the Intel VTune Profiler.

With the latest version of the patch where we ignore functions that have the first argument as string literals, the number of issues was areduced a little bit.

If this patch suits our needs please let me know and I'll submit it for review.

Looks good to me.

Comment on attachment 8956410 [details] Bug 1440886 - Implement a static analysis checker to detect usage of PR_LoadLibrary and LoadLibraryA/LoadLibraryExA/LoadLibrary/LoadLibraryEx. https://reviewboard.mozilla.org/r/225300/#review233298 ::: build/clang-plugin/LoadLibraryUsageChecker.h:5 (Diff revision 4) > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#ifndef FopenUssageChecker_h__ Wrong include guard ::: build/clang-plugin/LoadLibraryUsageChecker.h:10 (Diff revision 4) > +#ifndef FopenUssageChecker_h__ > +#define FopenUssageChecker_h__ > + > +#include "plugin.h" > + > +class LoadLibraryUsageChecker : public BaseCheck { Please add a comment to this file explaining why these functions are forbidden so that this fact is documented somewhere in-tree. ::: build/clang-plugin/LoadLibraryUsageChecker.h:18 (Diff revision 4) > + : BaseCheck(CheckName, Context) {} > + void registerMatchers(MatchFinder *AstMatcher) override; > + void check(const MatchFinder::MatchResult &Result) override; > +}; > + > +#endif Can you add a // !defined(include_guard_h) here? ::: build/clang-plugin/tests/TestLoadLibraryUsage.cpp:3 (Diff revision 4) > +#include <windows.h> > + > +void* PR_LoadLibrary(const char *name); Let's just #include "prlink.h" ::: build/clang-plugin/tests/moz.build:52 (Diff revision 4) > 'TestTrivialCtorDtor.cpp', > ] > > +if CONFIG['OS_ARCH'] == 'WINNT': > + SOURCES += [ > + 'TestLoadLibraryUsage.cpp', Should we have 2 copies of this test file, one where we #define UNICODE and one where we don't?

Comment on attachment 8956410 [details] Bug 1440886 - Implement a static analysis checker to detect usage of PR_LoadLibrary and LoadLibraryA/LoadLibraryExA/LoadLibrary/LoadLibraryEx. https://reviewboard.mozilla.org/r/225300/#review233332 ::: build/clang-plugin/LoadLibraryUsageChecker.cpp:10 (Diff revision 5) > +#include "LoadLibraryUsageChecker.h" > +#include "CustomMatchers.h" > + > +// On MacOS the filesystem is UTF-8, on linux the canonical filename is 8-bit > +// string. On Windows data loss conversion will occur. This checker restricts > +// the use of ASCII file functions for loading libraries. please add an "on windows"

How are we going to handle the usage of LoadLibraryExA and LoadLibraryA? This cannot land until all of the issues are fixed.

Filed bug 1445601.

I think now it is possible to land this. Please tell me if not.

I'm gonna test this again on the VM to see if there are still hidden issues, and if not I'm gonna push this to m-i. :emk thank you for the help!

Try is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c86f05cd3744fa38b06d9377071d99bea525301&selectedJob=177267894 Looking at the S targets we can land this now!

Pushed by bpostelnicu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/827deadecf7e Implement a static analysis checker to detect usage of PR_LoadLibrary and LoadLibraryA/LoadLibraryExA/LoadLibrary/LoadLibraryEx. r=Nika