Closed Bug 1391711 Opened 7 years ago Closed 4 years ago

Implement a static analysis checker to detect incorrect usages of fopen, open

Categories

(Developer Infrastructure :: Source Code Analysis, enhancement, P3)

Product:
Developer Infrastructure
Component:
Source Code Analysis
Type:
enhancement

Tracking

(firefox75 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla75
Tracking Flags:
Tracking Status
firefox75 --- fixed

People

(Reporter: Sylvestre, Assigned: andi)

References

(Depends on 1 open bug)

Details

Attachments

(2 files)

Bug 1391711 - Implement a static analysis checker to detect usages of fopen/ open like functions on win32 platform.
59 bytes, text/x-review-board-request
nika
: review+
Details
Bug 1391711 - Implement a static analysis checker to detect usages of fopen/ open like functions on win32 platform. r=froydnj
47 bytes, text/x-phabricator-request
Details | Review 
This caused bug 1389160.
We probably don't want to run it on all the code.
Sorry, openm was typo.
Summary: Implement a static analysis checker to detect incorrect usages of fopen, openm → Implement a static analysis checker to detect incorrect usages of fopen, open
Assignee: nobody → bpostelnicu
Component: Telemetry → Rewriting and Analysis
Product: Toolkit → Core
Ehsan could you please let me know what modules shouldn't ne scanned by this analysis? Because as Sylvestre said i think there are some places where we don't want to restrict fopen.
Flags: needinfo?(ehsan)
Priority: -- → P3
I don't understand this bug at all.  Can you please provide enough information to make this bug actionable?  What are the examples of unacceptable usages of fopen?  What are the examples of acceptable ones?  I did read bug 1389160 and I understand the problem there but still I'm not sure I understand what's proposed here, besides completely banning fopen but that's not what the summary says.

Also comment 2 is a question for Sylvestre, I'm not sure what code he had in mind...
Flags: needinfo?(ehsan) → needinfo?(sledru)
Ehsan, I don't know if banning fopen is an option or not but. If not, we should avoid the fopen usage when opening the profile path. Makes sense?
Flags: needinfo?(sledru)
Per-user installed Firefox will be put under %userprofile%. We should never use fopen/open at least on Windows, just like nsIFile "native" methods.
(In reply to Masatoshi Kimura [:emk] from comment #5)
> Per-user installed Firefox will be put under %userprofile%. We should never
> use fopen/open at least on Windows, just like nsIFile "native" methods.

So you suggest to have this checker available only on windows static analysis build?
Windows-only static analysis makes sense to me.
We are sure we don't have similar issues with Linux and Mac?
On macOS, the filesystem encoding is always UTF-8, so conversion loss will not happen. On Linux, no conversions will happen in the first place because 8-bit string is the canonical POSIX file name. Only Windows users will be suffered from lossy conversions. In case macOS or Linux have similar issues, we can add static analysis later. Adding Windows static analysis would be strict improvoment.
(In reply to Masatoshi Kimura [:emk] from comment #9)
> On macOS, the filesystem encoding is always UTF-8, so conversion loss will
> not happen. On Linux, no conversions will happen in the first place because
> 8-bit string is the canonical POSIX file name. Only Windows users will be
> suffered from lossy conversions. In case macOS or Linux have similar issues,
> we can add static analysis later. Adding Windows static analysis would be
> strict improvoment.

I have a question regarding this platform differentiation and on the way how the static analysis should work. Should it world only on windows but scan the entire source code that's associated with this platform or should it only scan the fopen that guarded with ifdef to be available only on windows. I think that it should scan all of the source code even though the same code will be build on different platforms.
I've attached a draft of the checker, from here on is trivial to have the final version, after I'll get my repply to the previous question.
Latest version of this checker, is only available on windows. Also the component for clang-tidy has also been limited only to windows builds. In order to keep consistency across clang-plugin and clang-tidy builds.
(In reply to Andi-Bogdan Postelnicu [:andi] from comment #10)
> I have a question regarding this platform differentiation and on the way how
> the static analysis should work. Should it world only on windows but scan
> the entire source code that's associated with this platform or should it
> only scan the fopen that guarded with ifdef to be available only on windows.
> I think that it should scan all of the source code even though the same code
> will be build on different platforms.

I agree to scan the entire source code that is built on Windows even though the some part of code will be built on different platforms.
(In reply to Masatoshi Kimura [:emk] from comment #15)
> (In reply to Andi-Bogdan Postelnicu [:andi] from comment #10)
> > I have a question regarding this platform differentiation and on the way how
> > the static analysis should work. Should it world only on windows but scan
> > the entire source code that's associated with this platform or should it
> > only scan the fopen that guarded with ifdef to be available only on windows.
> > I think that it should scan all of the source code even though the same code
> > will be build on different platforms.
> 
> I agree to scan the entire source code that is built on Windows even though
> the some part of code will be built on different platforms.

One thing though, the bug mentions the usage of open on Windows, are we referring to this function:

>>int open(const char *path, int oflag, ... );

If so i don't think that this function is available on win32 api, this being a POSIX C function. In this case we are left only with fopen.
Pretty sure that Windows declares a deprected open() function that maps to _open(): <https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/open-wopen>.  _open should probably be blacklisted similarly.

There are also a lot of other functions that need to be blacklisted similarly, like _open_s, fopen_s, CreateFile, OpenFile, etc.
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #17)
> Pretty sure that Windows declares a deprected open() function that maps to
> _open():
> <https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/open-
> wopen>.  _open should probably be blacklisted similarly.
> 
> There are also a lot of other functions that need to be blacklisted
> similarly, like _open_s, fopen_s, CreateFile, OpenFile, etc.
Thanks for this I'll be adding these two to the checker.
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #17)
> There are also a lot of other functions that need to be blacklisted
> similarly, like _open_s, fopen_s, CreateFile, OpenFile, etc.

nit: CreateFileA. CreateFileW should not be blacklisted.
Thanks for the additional info, I'm gonna update the patch to take into account:
open, _open, _open_s, fopen, fopen_s
Checker has been updated with support for: open, _open, _open_s, fopen, fopen_s. The way how the current checker works it relies almost entirely on the default matchers by match by funtion name an continuing with function parameters. This is a little bit strict but we don't want to have overlaps.
Does it find any issue in our code?
(In reply to Sylvestre Ledru [:sylvestre] from comment #23)
> Does it find any issue in our code?
yes, it finds a couple.
Added support for: OpenFile
So for now this checker blacklists the usage of: open, _open, _open_s, fopen, fopen_s, OpenFile.

What other functions should be blacklisted?
Used lambdas in order to better some matchers.
(In reply to Andi-Bogdan Postelnicu [:andi] from comment #27)
> Added support for: OpenFile
> So for now this checker blacklists the usage of: open, _open, _open_s,
> fopen, fopen_s, OpenFile.
> 
> What other functions should be blacklisted?

I apologize that comment #19 was not clear. CreateFileA should be blacklisted, but CreateFileW should not.
(In reply to Masatoshi Kimura [:emk] from comment #30)
> (In reply to Andi-Bogdan Postelnicu [:andi] from comment #27)
> > Added support for: OpenFile
> > So for now this checker blacklists the usage of: open, _open, _open_s,
> > fopen, fopen_s, OpenFile.
> > 
> > What other functions should be blacklisted?
> 
> I apologize that comment #19 was not clear. CreateFileA should be
> blacklisted, but CreateFileW should not.
OK thank you, I'll add that one as well. If you want you can already test it again the tree by activating in mozconfig:
>>ac_add_options --enable-clang-plugin
(In reply to Masatoshi Kimura [:emk] from comment #30)
> (In reply to Andi-Bogdan Postelnicu [:andi] from comment #27)
> > Added support for: OpenFile
> > So for now this checker blacklists the usage of: open, _open, _open_s,
> > fopen, fopen_s, OpenFile.
> > 
> > What other functions should be blacklisted?
> 
> I apologize that comment #19 was not clear. CreateFileA should be
> blacklisted, but CreateFileW should not.

Should this be limited only on UNICODE builds? Because If i recall CreateFileW is used only when UNICODE is defined otherwise CreateFileA is used.
If UNICODE is not defined, "CreateFile" (no suffix) macro will be defined as an alias of CreateFileA. If UNICODE is defined, CreateFile will be an alias of CreateFileW. The UNICODE macro will not affect CreateFileA and CreateFileW.

Summary: Blacklist only CreateFileA if you know UNICODE is defined. Otherwise, blacklist CreateFile and CreateFileA.
(In reply to Masatoshi Kimura [:emk] from comment #33)
> If UNICODE is not defined, "CreateFile" (no suffix) macro will be defined as
> an alias of CreateFileA. If UNICODE is defined, CreateFile will be an alias
> of CreateFileW. The UNICODE macro will not affect CreateFileA and
> CreateFileW.
> 
> Summary: Blacklist only CreateFileA if you know UNICODE is defined.
> Otherwise, blacklist CreateFile and CreateFileA.

Added this modification in the latest patch.
(In reply to Masatoshi Kimura [:emk] from comment #33)
> If UNICODE is not defined, "CreateFile" (no suffix) macro will be defined as
> an alias of CreateFileA. If UNICODE is defined, CreateFile will be an alias
> of CreateFileW. The UNICODE macro will not affect CreateFileA and
> CreateFileW.
> 
> Summary: Blacklist only CreateFileA if you know UNICODE is defined.
> Otherwise, blacklist CreateFile and CreateFileA.

Do we need to cover any other case? I think the last patch covers them all.
Per the above message. Thank you!
Flags: needinfo?(VYV03354)
I could not come up with anything.
Flags: needinfo?(VYV03354)
(In reply to Masatoshi Kimura [:emk] from comment #39)
> I could not come up with anything.

Thank you, I'll be submitting it to review then.
Comment on attachment 8910240 [details]
Bug 1391711 - Implement a static analysis checker to detect usages of fopen/ open like functions on win32 platform.

https://reviewboard.mozilla.org/r/181728/#review190570

I have some comments but they're mostly style nits.

r=me.

::: build/clang-plugin/Checks.inc:14
(Diff revision 9)
>  CHECK(CanRunScriptChecker, "can-run-script")
>  CHECK(DanglingOnTemporaryChecker, "dangling-on-temporary")
>  CHECK(ExplicitImplicitChecker, "implicit-constructor")
>  CHECK(ExplicitOperatorBoolChecker, "explicit-operator-bool")
> +#ifdef WIN32
> +CHECK(FopenUssageChecker, "fopen-ussage")

nit: s/ussage/usage here and elsewhere in the patch.

::: build/clang-plugin/FopenUssageChecker.cpp:10
(Diff revision 9)
> +#include "FopenUssageChecker.h"
> +#include "CustomMatchers.h"
> +
> +void FopenUssageChecker::registerMatchers(MatchFinder *AstMatcher) {
> +
> +  auto isPointerToConstCharType = [](const unsigned int Position) {

This matcher's name is a bit confusing, it's not clear that it's referring to a parameter rather than the function - perhaps hasConstCharPtrParam()?

::: build/clang-plugin/FopenUssageChecker.cpp:11
(Diff revision 9)
> +#include "CustomMatchers.h"
> +
> +void FopenUssageChecker::registerMatchers(MatchFinder *AstMatcher) {
> +
> +  auto isPointerToConstCharType = [](const unsigned int Position) {
> +    return (hasParameter(Position, hasType(pointsTo(isAnyCharacter()))),

Isn't this just discarding the first hasPamater in the , operator? I think you need an allOf()

::: build/clang-plugin/FopenUssageChecker.cpp:15
(Diff revision 9)
> +  auto isPointerToConstCharType = [](const unsigned int Position) {
> +    return (hasParameter(Position, hasType(pointsTo(isAnyCharacter()))),
> +            hasParameter(Position, hasType(pointsTo(isConstQualified()))));
> +  };
> +
> +  auto ifOfType = [](const unsigned int Position, const char *Name) {

s/if/is

::: build/clang-plugin/FopenUssageChecker.cpp:15
(Diff revision 9)
> +  auto isPointerToConstCharType = [](const unsigned int Position) {
> +    return (hasParameter(Position, hasType(pointsTo(isAnyCharacter()))),
> +            hasParameter(Position, hasType(pointsTo(isConstQualified()))));
> +  };
> +
> +  auto ifOfType = [](const unsigned int Position, const char *Name) {

perhaps change name to hasParamOfType?

::: build/clang-plugin/FopenUssageChecker.cpp:19
(Diff revision 9)
> +
> +  auto ifOfType = [](const unsigned int Position, const char *Name) {
> +    return hasParameter(Position, hasType(asString(Name)));
> +  };
> +
> +  auto isIntegerType = [](const unsigned int Position) {

perhaps change name to hasIntegerParam?

::: build/clang-plugin/FopenUssageChecker.cpp:29
(Diff revision 9)
> +      callExpr(
> +          allOf(
> +              isFirstParty(),
> +              callee(functionDecl(allOf(
> +                  isInSystemHeader(),
> +                  anyOf(allOf(anyOf(allOf(hasName("fopen"),

Are there other functions with these names we're worried about false-positiveing-on?
It seems like it would be simpler to just match on the names of the functions rather than checking the specific type signature.

I don't think this is a super big deal though.

::: build/clang-plugin/FopenUssageChecker.cpp:61
(Diff revision 9)
> +void FopenUssageChecker::check(const MatchFinder::MatchResult &Result) {
> +  const CallExpr *FuncCall = Result.Nodes.getNodeAs<CallExpr>("funcCall");
> +
> +  if (FuncCall) {
> +    diag(FuncCall->getLocStart(),
> +         "usage of fopen/ open and related functions is forbidden",

Perhaps reword as "usage of ASCII file functions (such as %0) is forbidden on windows", and pass in the name of the blocked function as %0?
Attachment #8910240 - Flags: review?(nika) → review+
Comment on attachment 8910240 [details]
Bug 1391711 - Implement a static analysis checker to detect usages of fopen/ open like functions on win32 platform.

https://reviewboard.mozilla.org/r/181728/#review190678

::: build/clang-plugin/tests/TestFopenUssage.cpp:20
(Diff revision 9)
> +  errno_t err = _sopen_s(&fd4, "dummy.txt", _O_RDONLY, _SH_DENYRW, 0); // expected-error {{usage of fopen/ open and related functions is forbidden}}
> +
> +  LPOFSTRUCT buffer;
> +  HFILE hFile1 = OpenFile("dummy.txt", buffer, OF_READ); // expected-error {{usage of fopen/ open and related functions is forbidden}}
> +
> +#ifndef UNCODE

Nit: UNICODE :-)
Depends on: 1409692
Product: Core → Firefox Build System
The only thing that prevents the landing of this patch is the huge amount of 'dangerous' functions that are used and marked by this patch in our code. 
Do you think you can help with the fixes?
Flags: needinfo?(VYV03354)

This is a re-work of https://hg.mozilla.org/mozreview/gecko/rev/a28ac360cc17f4aeb1cf5074723f49f2f8416271#index_header,
and because time passed and not all of the issues reported by this checker were fixed and we want to land it,
I've decided to move from errors diagnostic messages to warnings.

A new log with the warnings related build with fopen on build-win64/debug.

Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2d6fc4d1275b
Implement a static analysis checker to detect usages of fopen/ open like functions on win32 platform. r=sg

https://hg.mozilla.org/mozilla-central/rev/2d6fc4d1275b

Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox75: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla75
Regressions: 1614814
Regressions: 1614840
Regressions: 1615245
Flags: needinfo?(VYV03354)
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: