Last Comment Bug 642243 - run binscope to alert us and turn the tree red when we don't use ASLR and other protection tools
: run binscope to alert us and turn the tree red when we don't use ASLR and ot...
Status: RESOLVED FIXED
[sg:want P2]
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: unspecified
: x86 All
: -- normal with 1 vote (vote)
: mozilla11
Assigned To: Ian Melven :imelven
:
:
Mentors:
Depends on: 677797 674250 700513 728429
Blocks: 1021214 1236356
  Show dependency treegraph
 
Reported: 2011-03-16 12:41 PDT by chris hofmann
Modified: 2016-01-03 09:30 PST (History)
19 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
BinScope report for 4.0rc1 (1.80 MB, text/html)
2011-03-16 13:41 PDT, Daniel Veditz [:dveditz]
no flags Details
python script to run all the binscope modules via integration with the build (1.71 KB, text/plain)
2011-07-24 15:34 PDT, Ian Melven :imelven
no flags Details
v2 python script (1.73 KB, text/plain)
2011-07-24 15:53 PDT, Ian Melven :imelven
no flags Details
script updated with feedback (2.91 KB, text/plain)
2011-07-26 09:29 PDT, Ian Melven :imelven
no flags Details
v4 python script (2.89 KB, text/plain)
2011-07-28 09:59 PDT, Ian Melven :imelven
no flags Details
WIP patch (3.97 KB, patch)
2011-10-18 15:10 PDT, Ian Melven :imelven
no flags Details | Diff | Splinter Review
patch v2 (3.92 KB, patch)
2011-10-24 17:26 PDT, Ian Melven :imelven
no flags Details | Diff | Splinter Review
patch v3 (4.74 KB, patch)
2011-10-25 16:47 PDT, Ian Melven :imelven
ted: review-
Details | Diff | Splinter Review
patch v4 (5.51 KB, patch)
2011-11-07 16:48 PST, Ian Melven :imelven
ted: review+
Details | Diff | Splinter Review
patch v5 (5.42 KB, patch)
2011-11-17 14:26 PST, Ian Melven :imelven
ian.melven: review+
Details | Diff | Splinter Review

Description chris hofmann 2011-03-16 12:41:24 PDT
follow up from [Bug 641630] ANGLE's libEGL.dll and libGLESv2.dll don't have ASLR enabled.

confidential until we get something in place and/or that bug is openned.

wonder if it would make sense to add this as a test and turn the tree red when SDL requirements are not met

http://www.microsoft.com/downloads/en/details.aspx?displaylang=en&FamilyID=90e6181c-5905-4799-826a-772eafd4440a

dveditz looked at this a bit, and it may not be the right tool but we need it or something similar.
Comment 1 Daniel Veditz [:dveditz] 2011-03-16 13:41:55 PDT
Created attachment 519763 [details]
BinScope report for 4.0rc1

I ran all BinScope tests against 4.0rc1, we'll have to figure out which ones we actually care about. There are several failures we should fix.
Comment 2 Ted Mielczarek [:ted.mielczarek] 2011-03-17 13:50:03 PDT
It'd probably be easier to use dumpbin, or just write a custom tool in C++ to do this. Parsing the PE headers isn't terribly hard, Microsoft provides all the structure definitions in ImageHlp.h. (I wrote attachment 449081 [details] which uses them to get at some information.)
Comment 3 Ian Melven :imelven 2011-07-01 11:50:38 PDT
dumpbin /headers can get some of these (NX and ASLR) : 

            8140 DLL characteristics
                   Dynamic base
                   NX compatible
                   Terminal Server Aware

binscope seems to do a bit more than this obviously - /GS is a really big one we need as well, need to look more into how binscope detects that
Comment 4 Ted Mielczarek [:ted.mielczarek] 2011-07-01 11:57:30 PDT
I haven't ever looked into it, but presumably you could look for an import of __security_init_cookie. /GS is on by default in VC8 or newer, so it seems less likely to be a problem.
Comment 5 Ian Melven :imelven 2011-07-06 17:34:41 PDT
i just ran binscope against browser.exe and plugin-container.exe for nightly (7), aurora (6), and FF5 (release) and saw no issues :)

the symbol server string SRV*c:\symcache\*http://symbols.mozilla.org/firefox needs to be used to obtain results

binscope can also be run in command line mode and we can supply an xslt as well to get results in whatever format we need for the build system

i guess the next step is to find someone in releng to talk to about this ? any suggestions from folks cc'd on the bug ?
Comment 6 chris hofmann 2011-07-10 01:48:42 PDT
seems like to might be somewhere in the middle of figuring out how this fits into the build infrastructure or the test automation infrastructure, which sadly would mean no one steps up to figure out to make it start happening.  cc'ing bmoss and joduin to find the best owner for this.
Comment 7 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-07-11 18:46:54 PDT
If the tools are on the machines and can be run from the command line, we can just add it to make check.
Comment 8 Ian Melven :imelven 2011-07-11 19:31:57 PDT
(In reply to comment #7)
> If the tools are on the machines and can be run from the command line, we
> can just add it to make check.

binscope definitely won't be on the machine by default, it also required NET Framework 2.0 and maybe some other fun dependencies. i'll find some time soon to take a look at make check and see how the command line version of binscope could integrate while we see if someone from releng wants to join the fun :)
Comment 9 cmtalbert 2011-07-13 16:58:43 PDT
This looks pretty simple from my brief read through of this bug and the microsoft link in comment 0.  As far as automation integration is concerned, there are three steps that need to be resolved.  

1. Get the dependencies/tools onto the build slaves
2. Run the tool
3. Parse the results

Note that 1 and 2,3 can be (and should be) done in parallel.

= Get the Binaries onto the Build Slaves =
* Figure out what the dependencies are in terms of Microsoft toolchain (as different from what we currently have on the build machines)
* File a bug to change the windows build reference platform w.r.t. the tool additions

= Running the tool =
RelEng prefers to have one simple way to run things - one script to call, for example.  There are options
* We have access to bash on windows (as provided by msys) and so you can provide RelEng with a bash script that executes the proper commands for binscope.  Alternatively, you could also write a python script that wraps the binscope exectuable and call it that way.  
* Once you have a "one-executable-method" to run the tool then we need to write the buildbot step that will run this script. 
* Or if you wire this in with your own C++ code, you could invoke that through a make target.

= Getting the results =
Buildbot sucks at result parsing.  I'd prefer to do the parsing of the results on the buildbot slave side - so ideally, however we run the tool outputs a "PASS" or "FAIL" status and provides us with a log.  It is then pretty easy to wire that reporting into buildbot.  (It uses PASS or FAIL to turn the build different colors, and the log can be appended to the buildlog).

Note that since we're talking about microsoft tool that (as I understand it) requires the microsoft VC toolchain, we will need to do this on *buildslaves* not *testslaves*.  This distinction is important once you start talking with RelEng.  So, keep that in mind.
Comment 10 Ian Melven :imelven 2011-07-19 11:28:16 PDT
from http://www.microsoft.com/download/en/details.aspx?id=11910 :

Supported Operating Systems: Windows 7, Windows Server 2008, Windows Vista

For Standalone version:

• Host system must be running the latest version of the Windows OS supported by your application

• .NET Framework 2.0 or later

• Windows Server 2003 SP2 – with manual modifications detailed in Instructions

• Windows Server 2008 (or higher) 

---

this implies the machines running this need to have Windows 7 on them. i'll look into what the build and test machines are running for Windows. 

binscope doesn't require visual studio to be installed. just the NET Framework 2.0 or higher, and apparently Windows 7

as far as an easy to parse etc script, i can definitely write up something that outputs PASS or FAIL and dumps a log of the rest of the info. i'll also look into if there's python or something like that on the build/test slaves as well as bash.
Comment 11 Ian Melven :imelven 2011-07-19 11:34:16 PDT
https://wiki.mozilla.org/ReferencePlatforms/Test/Win7

looks like we have a win 7 test reference platform
Comment 12 Ian Melven :imelven 2011-07-19 11:44:02 PDT
oh i see that clint said in c#9 that it can be a python script. i'll try to whip that up soon.
Comment 13 cmtalbert 2011-07-20 13:27:07 PDT
I thought that this was a change to the builder only, not test side.  What is the usecase to have all the test slaves that download the build from changeset X do this test?  Why not just do this test once on the builder when the build from changeset X is produced.

I think doing this on the tests is a waste of developer time in that it will make the end to end time longer, which is the opposite of what I want to be doing. :)

So, build reference platform for win32:
https://wiki.mozilla.org/ReferencePlatforms/Win32

Build reference platform for win64:
https://wiki.mozilla.org/ReferencePlatforms/Win64
Comment 14 Ian Melven :imelven 2011-07-20 13:40:28 PDT
(In reply to comment #13)
> I thought that this was a change to the builder only, not test side.  What
> is the usecase to have all the test slaves that download the build from
> changeset X do this test?  Why not just do this test once on the builder
> when the build from changeset X is produced.
> 
> I think doing this on the tests is a waste of developer time in that it will
> make the end to end time longer, which is the opposite of what I want to be
> doing. :)
> 
> So, build reference platform for win32:
> https://wiki.mozilla.org/ReferencePlatforms/Win32
> 
> Build reference platform for win64:
> https://wiki.mozilla.org/ReferencePlatforms/Win64

i am still learning how all our build/test stuff works :) yeah, doing it on the builder makes a lot more sense and catching it upstream from the test boxes. i definitely agree that we don't want to make the end to end time longer (and this change doesn't require doing so)
Comment 15 Ian Melven :imelven 2011-07-20 13:46:02 PDT
the win32 build is based off of win 2003 server sp2 which the binscope page says it supports, the win64 build build is win 2008 server so we should be good to go there. i'll see if binscope works 'out of the box' on w2k3 server sp2 and do the python script when i can.
Comment 16 cmtalbert 2011-07-21 10:32:24 PDT
(In reply to comment #15)
> the win32 build is based off of win 2003 server sp2 which the binscope page
> says it supports, the win64 build build is win 2008 server so we should be
> good to go there. i'll see if binscope works 'out of the box' on w2k3 server
> sp2 and do the python script when i can.

Awesome, sounds great.
Comment 17 Ian Melven :imelven 2011-07-23 18:00:08 PDT
ok, Windows Server 2003 SP2 fully patched still needs NET Framwork 3.5 apparently. i'll file a bug to get this on the W2K3 build servers once the script is ready to go.
Comment 18 Ian Melven :imelven 2011-07-23 18:43:03 PDT
both Windows 2003 and Windows 2008 are going to need "Debugging Tools for Windows" installed, or at least symchk.exe to download symbols, this is now a component of the Windows 7 SDK and can't be downloaded separately any more apparently :(
Comment 19 Ian Melven :imelven 2011-07-23 22:02:35 PDT
a fully patched Windows 2008 server doesn't need anything other than the Debugging Tools installed to run binscope (no .NET framework).
Comment 20 Ian Melven :imelven 2011-07-24 15:34:04 PDT
Created attachment 548050 [details]
python script to run all the binscope modules via integration with the build
Comment 21 Ian Melven :imelven 2011-07-24 15:37:35 PDT
autobinscope.py produces output like :

C:\src>python autobinscope.py
Microsoft SDL BinScope binary analysis tool v1.0.4027.29711
PASS

(would be FAIL if one or more of the tests failed). all the paths and such are constants in the script. 

Debugging Tools/Windows SDK doesn't need to be installed anywhere, binscope is smart enough to understand a Windows symbol path pointing to the Mozilla symbol server. i'll file some followup bugs to start the other bits (NET Framework 3.5 on the Windows 2003 build machines, Binscope itself installed as well) moving.
Comment 22 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-07-24 15:42:07 PDT
You'll want to print 'TEST-UNEXPECTED-FAIL' to get the tinderbox log parser to pick it up.
Comment 23 Ian Melven :imelven 2011-07-24 15:53:58 PDT
Created attachment 548052 [details]
v2 python script

Thanks Kyle ! changed to output "TEST-UNEXPECTED-FAIL" on a fail
Comment 24 Ted Mielczarek [:ted.mielczarek] 2011-07-25 05:49:17 PDT
Just some notes:
a) The build slaves already have "Debugging Tools for Windows" installed, we use some binaries for there for our source server support
b) You shouldn't need to point at symbols.mozilla.org, the build machine has all the PDB files locally since it just built the binaries
c) The TARGET_BINARY should be passed on the commandline, since it's not going to be actually installed (it will be in a known location in the objdir)
d) You might need to change some of those other hardcoded paths, I'm not sure where the best place to put things on the build slave is
e) You should add some more information to the fail line so people looking at a build log can figure out exactly what failed, like "TEST-UNEXPECTED-FAIL | autobinscope | Binaries are not properly DEP/ASLR enabled"
Comment 25 Ian Melven :imelven 2011-07-25 10:23:03 PDT
(In reply to comment #24)
> Just some notes:
> a) The build slaves already have "Debugging Tools for Windows" installed, we
> use some binaries for there for our source server support

we don't need this any more, binscope knows how to do symbols without needing symchk.exe and friends from Debugging Tools :)

> b) You shouldn't need to point at symbols.mozilla.org, the build machine has
> all the PDB files locally since it just built the binaries

unfortunately i couldn't get binscope to work with passing in a local symbol dir, it seems to require a Windows Symbol SRV type path :/

> c) The TARGET_BINARY should be passed on the commandline, since it's not
> going to be actually installed (it will be in a known location in the objdir)

great point, i'll modify the script to do this. the other thing i thought of since yesterday is that we will also want to do the check on plugin-container as well as firefox.exe. i'll make the script expect to be passed paths to both
these binaries as its arguments

> d) You might need to change some of those other hardcoded paths, I'm not
> sure where the best place to put things on the build slave is

right, i was hoping for some tips from our RelEng friends on proper values here. 

> e) You should add some more information to the fail line so people looking
> at a build log can figure out exactly what failed, like
> "TEST-UNEXPECTED-FAIL | autobinscope | Binaries are not properly DEP/ASLR
> enabled"

another good point, i'll do this and also add a pointer to the log binscope itself writes out in the error message as well.

thanks very much for the feedback !
Comment 26 Ted Mielczarek [:ted.mielczarek] 2011-07-25 10:31:21 PDT
(In reply to comment #25)
> > b) You shouldn't need to point at symbols.mozilla.org, the build machine has
> > all the PDB files locally since it just built the binaries
> 
> unfortunately i couldn't get binscope to work with passing in a local symbol
> dir, it seems to require a Windows Symbol SRV type path :/

We'll need to rethink this then, as build machines are not allowed to access machines outside of the build network.
Comment 27 Ian Melven :imelven 2011-07-25 12:01:37 PDT
(In reply to comment #26)
> (In reply to comment #25)
> > > b) You shouldn't need to point at symbols.mozilla.org, the build machine has
> > > all the PDB files locally since it just built the binaries
> > 
> > unfortunately i couldn't get binscope to work with passing in a local symbol
> > dir, it seems to require a Windows Symbol SRV type path :/
> 
> We'll need to rethink this then, as build machines are not allowed to access
> machines outside of the build network.

the docs do claim that it works with a local symbol directory, i'll re-investigate. the restriction on the build machines going outside of the build network makes total sense to me.
Comment 28 Ian Melven :imelven 2011-07-25 16:26:27 PDT
ok binscope works just fine with using a local directory as the symbol directory. it also (thankfully) expects the pdbs to be in that directory in a flat structure (ie it doesn't require the Microsoft symbol hashing etc stuff). i'll upload a new version of the script incoporating this and the other feedback from Ted.
Comment 29 Ian Melven :imelven 2011-07-26 09:29:50 PDT
Created attachment 548493 [details]
script updated with feedback

updated the script with Ted's feedback, thanks !
Comment 30 Ben Hearsum (:bhearsum) 2011-07-28 07:09:22 PDT
Comment on attachment 548493 [details]
script updated with feedback

># autobinscope.py
># imelven@mozilla.com 7/26/11
># run Microsoft's Binscope tool (http://www.microsoft.com/download/en/details.aspx?id=11910)
># against a fresh build. output a 'binscope.log' file with full details
># of the run and appropriate strings to integrate with the buildbots
>
># from the docs : "The error code returned when running under the command line is equal 
># to the number of failures the tool reported plus the number of errors. BinScope will return 
># 0 only if there are no errors or failures."
>
>import sys
>import subprocess
>
>BINSCOPE_BINARY = "C:\\Program Files\\Microsoft\\SDL BinScope\\binscope.exe"

This is a sensible fallback, but it might be worthwhile looking for it in %PATH% first, to allow for alternate installation locations.

># CHANGEME : please point these to where you would like the binscope output logs to go 
>BINSCOPE_OUTPUT_LOGFILE_FIREFOX = "C:\\binscope_xml_output_firefox.log"
>BINSCOPE_OUTPUT_LOGFILE_PLUGIN_CONTAINER = "C:\\binscope_xml_output_plugin_container.log"

Can binscope.exe be output to stdout directly? Unless these logs are going to be uploaded somewhere, there's not much point in writing to them.

If we do need to write to logs, and if this is going to be wrapped by the build system, these should have an option to be overridden by a parameter or environment variable so that they can be outputted somewhere in the objdir.

>
># usage
>if len(sys.argv) < 3:
>  print "usage : autobinscope.by <path to firefox binary> <path to plugin-container binary <path to symbols>"
>  sys.exit(0)

It would be nice to be a bit more generic here and say "app_path" - other applications (Thunderbird, SeaMonkey) can probably make use of this, too. It might even be worthwhile dropping the plugin-container binary argument and having the program called twice. It looks like the binscope invocation is the same for both.
Comment 31 Ian Melven :imelven 2011-07-28 09:58:46 PDT
(In reply to comment #30)

thanks very much for the feedback. i made the log file path an optional parameter since it doesn't appear that i can make binscope write to standard out, look in the %PATH% for binscope first, falling back to the default install location and made the script more generic (implying the build system now needs to run it twice, as you suggested).
Comment 32 Ian Melven :imelven 2011-07-28 09:59:57 PDT
Created attachment 549152 [details]
v4 python script

updated with feedback from bhearsum
Comment 33 Ian Melven :imelven 2011-08-09 09:18:00 PDT
bhearsum has gotten binscope on to the build machines, can someone help with next steps to get calling the script into the build ?
Comment 34 Ted Mielczarek [:ted.mielczarek] 2011-08-09 09:27:20 PDT
What you'll need to do is figure out a place to put this in mozilla-central, then add a Makefile target to call it as part of the build (perhaps in a check:: rule, so that it gets run as part of "make check").
Comment 35 Ian Melven :imelven 2011-10-18 14:42:15 PDT
how does /build/win32 sound as a place for autobinscope.py to live ?

i think i'm still going to need some releng help to find the appropriate Makefile's make check to stick the invocation of autobinscope.py in - it needs to be at the end of the Windows build when the binaries etc are all there - does this imply it needs to go in the top level Makefile ?
Comment 36 Ted Mielczarek [:ted.mielczarek] 2011-10-18 14:52:53 PDT
build/win32 seems fine for the location. I think what you want to do is hook this into a "check" target. "make check" gets run after the build is complete, this is the perfect time to run your test.

You can simply add a rule like:
ifeq (WINNT, $(OS_TARGET))
check::
    $(PYTHON) $(srcdir)/autobinscope.py $(DIST)/bin/whatever.exe ...
endif

to the bottom of build/win32/Makefile.in.
Comment 37 Ian Melven :imelven 2011-10-18 14:54:49 PDT
(In reply to Ted Mielczarek [:ted, :luser] from comment #36)
> build/win32 seems fine for the location. I think what you want to do is hook
> this into a "check" target. "make check" gets run after the build is
> complete, this is the perfect time to run your test.
> 
> You can simply add a rule like:
> ifeq (WINNT, $(OS_TARGET))
> check::
>     $(PYTHON) $(srcdir)/autobinscope.py $(DIST)/bin/whatever.exe ...
> endif
> 
> to the bottom of build/win32/Makefile.in.

awesome, thanks Ted, i'll give it a try and then try on Try :D
Comment 38 Ian Melven :imelven 2011-10-18 15:10:02 PDT
Created attachment 567900 [details] [diff] [review]
WIP patch

WIP patch - i need to figure out where the symbols end up after a win32 build when i get back to somewhere i have a windows box. then i can try this locally and if it works do a win32 only try run to see it work there.
Comment 39 Ian Melven :imelven 2011-10-24 15:57:07 PDT
ted pointed me to 'make buildsymbols' which when run from the objdir copies the symbols (and compresses them) to dist/crashreporter-symbols

right now i'm having trouble with getting autobinscope.py to be able to find the binscope.exe from when i run it within the mozilla build environment, it keeps telling me file not found although the same path works fine for executing the binary outside of the python script. it's a little confusing because i'm not sure what style of path python is expecting when it's running inside mozilla build - neither a cygwin /c/Program Files/ etc. or c:\Program Files\ etc. path seems to be working...
Comment 40 Ian Melven :imelven 2011-10-24 17:26:37 PDT
Created attachment 569245 [details] [diff] [review]
patch v2

cleaned up the script and got the default path stuff working in mozilla-build. ready for a Windows try run to see if it works there.
Comment 41 Ted Mielczarek [:ted.mielczarek] 2011-10-25 05:15:58 PDT
Python is a normal Windows binary, so you need to give it real Windows-style paths. You'll either have to escape the backslashes or use raw strings, like:
"c:\\foo\\bar" or r"c:\foo\bar".

Also, you're going to want to accept the path to binscope.exe as an environment variable that RelEng will set in the buildbot environment, since normal developers won't have it on their machines. Your check test should exit gracefully if it's not set, so that "make check" succeeds on developer machines.
Comment 42 Ian Melven :imelven 2011-10-25 09:02:01 PDT
(In reply to Ted Mielczarek [:ted, :luser] from comment #41)
> Python is a normal Windows binary, so you need to give it real Windows-style
> paths. You'll either have to escape the backslashes or use raw strings, like:
> "c:\\foo\\bar" or r"c:\foo\bar".

yeah i finally got this going yesterday. 

> Also, you're going to want to accept the path to binscope.exe as an
> environment variable that RelEng will set in the buildbot environment, since
> normal developers won't have it on their machines. Your check test should
> exit gracefully if it's not set, so that "make check" succeeds on developer
> machines.

the script looks in $PATH for binscope before using the default location, should i check another env var for a specific binscope path too ? i'm fine with that.

i'll clean it up so it exits cleanly if binscope isn't found. my try run found an issue with exception syntax being different between 2.5 (which is what was on the try machine) and 2.6 (which is what's in mozilla build).
Comment 43 Ian Melven :imelven 2011-10-25 16:47:54 PDT
Created attachment 569547 [details] [diff] [review]
patch v3

Ted, please let me know if it's enough for binscope to look in PATH for binscope and then fall back to the default or if you prefer a specific env variable (i would probably try the env variable location, PATH, and then the default if that's the case)

i realized i needed to call this for plugin-container as well in the make check and cleaned up a few other things. 

successful try run : https://tbpl.mozilla.org/?tree=Try&rev=8c75e96a943d

log looks like :

d:/mozilla-build/python25/python2.5.exe /e/builds/moz2_slave/try-w32/build/build/win32/autobinscope.py ../../dist/bin/firefox.exe ../../dist/crashreporter-symbols/
Microsoft SDL BinScope binary analysis tool v1.0.4027.29711
Could not locate binscope in PATH, trying default location of :
c:\Program Files\Microsoft\SDL BinScope\binscope.exe
PASS |autobinscope.py| ../../dist/bin/firefox.exe succeeded
d:/mozilla-build/python25/python2.5.exe /e/builds/moz2_slave/try-w32/build/build/win32/autobinscope.py ../../dist/bin/plugin-container.exe ../../dist/crashreporter-symbols/
Microsoft SDL BinScope binary analysis tool v1.0.4027.29711
Could not locate binscope in PATH, trying default location of :
c:\Program Files\Microsoft\SDL BinScope\binscope.exe
PASS |autobinscope.py| ../../dist/bin/plugin-container.exe succeeded
Comment 44 Ted Mielczarek [:ted.mielczarek] 2011-11-03 10:48:46 PDT
Comment on attachment 569547 [details] [diff] [review]
patch v3

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

r- for a few things, but a lot of them are nitpicky.

::: build/win32/Makefile.in
@@ +111,5 @@
>  endif # WIN32_REDIST_DIR
> +
> +# run the binscope tool to make sure the binary and all libraries
> +# are using all available Windows OS-level security mechanisms
> +ifeq (WINNT, $(OS_TARGET))

This is redundant, since you're in build/win32, which only gets built on Windows.

@@ +114,5 @@
> +# are using all available Windows OS-level security mechanisms
> +ifeq (WINNT, $(OS_TARGET))
> +check::
> +	$(PYTHON) $(srcdir)/autobinscope.py $(DIST)/bin/firefox.exe $(DIST)/crashreporter-symbols/
> +	$(PYTHON) $(srcdir)/autobinscope.py $(DIST)/bin/plugin-container.exe $(DIST)/crashreporter-symbols/

Might be nice to switch this to take multiple .exe files on the command line. Maybe you could switch the script args to be like <symbol path> <exe> [<exe> ...]? Then you could get away with a single invocation.

::: build/win32/autobinscope.py
@@ +1,1 @@
> +# autobinscope.py

You want a real license header here. You can use the MPL-tri license: http://www.mozilla.org/MPL/boilerplate-1.1/ or something simpler (BSD/MIT/Public domain) since this is just for testing. Also, put a shebang line at the start? The canonical Python one is generally #!/usr/bin/env python

@@ +13,5 @@
> +
> +import sys
> +import subprocess
> +
> +#BINSCOPE_DEFAULT_PATH = "c:\\Program Files (x86)\\Microsoft\\SDL BinScope\\binscope.exe"

Don't leave stuff in but commented out, just remove it.

@@ +14,5 @@
> +import sys
> +import subprocess
> +
> +#BINSCOPE_DEFAULT_PATH = "c:\\Program Files (x86)\\Microsoft\\SDL BinScope\\binscope.exe"
> +BINSCOPE_DEFAULT_PATH = "c:\\Program Files\\Microsoft\\SDL BinScope\\binscope.exe"

FYI you can also do raw strings in Python like r"foo\bar", the slashes are treated as literals. Handy for writing out Windows paths.

@@ +21,5 @@
> +
> +# usage
> +if len(sys.argv) < 3:
> +  print "usage : autobinscope.by path_to_binary path_to_symbols [log_file_path]"
> +  print "log_file_path is optional, log will be written to .\binscope_xml_output.log by default"

You didn't escape the backslash here, FYI. Also, you could have done this as a triple-quoted string:
print """usage: ...
...."""

@@ +59,5 @@
> +        "/c", "ATLVersionCheck", "/c", "ATLVulnCheck", "/c", "FunctionPointersCheck",
> +        "/c", "SharedSectionCheck", "/c", "APTCACheck", "/c", "NXCheck",
> +        "/c", "GSCheck", "/c", "GSFunctionSafeBuffersCheck", "/c", "GSFriendlyInitCheck",
> +        "/c", "CompilerVersionCheck", "/c", "SafeSEHCheck", "/c", "SNCheck",
> +        "/c", "DBCheck"], stdout=subprocess.PIPE)

Can you put these args in a list up above and reuse that list instead of duplicating it? Also, given how this winds up looking, I think I'd prefer if you just required either:
a) BINSCOPE set in the environment to the path to the binary or
b) Installed in the default location. (Is this how it's installed on our build slaves?)

@@ +70,5 @@
> +        print "Could not locate binscope at default location of :\n" + BINSCOPE_DEFAULT_PATH
> +        print "Binscope wasn't installed, skipping this check and exiting..."
> +        sys.exit(0)
> +
> +proc1.wait

You're not actually calling a function here...

@@ +72,5 @@
> +        sys.exit(0)
> +
> +proc1.wait
> +
> +output = proc1.communicate()[0]

...but it works out okay because communicate calls wait internally.

@@ +75,5 @@
> +
> +output = proc1.communicate()[0]
> +
> +#print "\noutput from binscope is : " + output
> +#print "\n"

Again, don't leave commented code in.

@@ +82,5 @@
> +#print "\nretcode is " + str(proc1.returncode)
> +
> +# is this a PASS or a FAIL ? 
> +if proc1.returncode != 0:
> +  print "TEST-UNEXPECTED-FAIL |autobinscope.py| " + binary_path + " is missing a needed Windows protection, such as /GS or ASLR"

I'd prefer if you used string formatting rather than concatenation, like;
print "TEST-UNEXPECTED FAIL | autobinscope.py | %s is missing .... " % binary_path

@@ +84,5 @@
> +# is this a PASS or a FAIL ? 
> +if proc1.returncode != 0:
> +  print "TEST-UNEXPECTED-FAIL |autobinscope.py| " + binary_path + " is missing a needed Windows protection, such as /GS or ASLR"
> +else:
> +  print "PASS |autobinscope.py| " + binary_path + " succeeded"

Should be TEST-PASS.
Comment 45 Ian Melven :imelven 2011-11-07 16:46:47 PST
(In reply to Ted Mielczarek [:ted, :luser] from comment #44)
> Comment on attachment 569547 [details] [diff] [review] [diff] [details] [review]
> patch v3
>
> > +# run the binscope tool to make sure the binary and all libraries
> > +# are using all available Windows OS-level security mechanisms
> > +ifeq (WINNT, $(OS_TARGET))
> 
> This is redundant, since you're in build/win32, which only gets built on
> Windows.

fixed, deleted the ifeq

> Might be nice to switch this to take multiple .exe files on the command
> line. Maybe you could switch the script args to be like <symbol path> <exe>
> [<exe> ...]? Then you could get away with a single invocation.

this is the one change i didn't make - if you feel strongly about this let me know, i kind of like having one run of script -> one binscope invocation (for the binscope log file output being per run if nothing else)
 
> You want a real license header here. You can use the MPL-tri license:
> http://www.mozilla.org/MPL/boilerplate-1.1/ or something simpler
> (BSD/MIT/Public domain) since this is just for testing. Also, put a shebang
> line at the start? The canonical Python one is generally #!/usr/bin/env
> python

i used the MPL-tri license and added the shebang line as suggested
 
> > +#BINSCOPE_DEFAULT_PATH = "c:\\Program Files (x86)\\Microsoft\\SDL BinScope\\binscope.exe"
> 
> Don't leave stuff in but commented out, just remove it.

done !
 
> FYI you can also do raw strings in Python like r"foo\bar", the slashes are
> treated as literals. Handy for writing out Windows paths.

thanks, good to know :)
 
> > +  print "usage : autobinscope.by path_to_binary path_to_symbols [log_file_path]"
> > +  print "log_file_path is optional, log will be written to .\binscope_xml_output.log by default"
> 
> You didn't escape the backslash here, FYI. Also, you could have done this as
> a triple-quoted string:
> print """usage: ...
> ...."""

i switched to using """ here, thanks again for the tip
 
> Can you put these args in a list up above and reuse that list instead of
> duplicating it? Also, given how this winds up looking, I think I'd prefer if
> you just required either:
> a) BINSCOPE set in the environment to the path to the binary or
> b) Installed in the default location. (Is this how it's installed on our
> build slaves?)

the default location varies depending on if we're on 32 or 64 bit Windows so i changed the path to come from the BINSCOPE environment variable - i will file
a blocking bug to get this set up on the build slaves 

> > +proc1.wait
> 
> You're not actually calling a function here...

fixed !
 
> > +#print "\noutput from binscope is : " + output
> > +#print "\n"
> 
> Again, don't leave commented code in.

removed !
 
> I'd prefer if you used string formatting rather than concatenation, like;
> print "TEST-UNEXPECTED FAIL | autobinscope.py | %s is missing .... " %
> binary_path

fixed - another good Python tip :)

> > +  print "PASS |autobinscope.py| " + binary_path + " succeeded"
> 
> Should be TEST-PASS.

fixed !
Comment 46 Ian Melven :imelven 2011-11-07 16:48:01 PST
Created attachment 572676 [details] [diff] [review]
patch v4

Patch updated with review feedback as outlined above
Comment 47 Ian Melven :imelven 2011-11-07 16:55:59 PST
Filed bug 700513 (need BINSCOPE env variable on windows build slaves pointing to dir binscope is installed in) as a dependency - when this is done and propagates, i will do a try run and if all looks good (pending review of course), this should be ready to go :)
Comment 48 Ted Mielczarek [:ted.mielczarek] 2011-11-08 11:07:52 PST
Comment on attachment 572676 [details] [diff] [review]
patch v4

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

r=me with just a few nits/suggestions.

::: build/win32/autobinscope.py
@@ +72,5 @@
> +  
> +# execute binscope against the binary, using the BINSCOPE environment
> +# variable as the path where binscope.exe is located 
> +try:
> +  binscope_path = os.path.join(os.environ['BINSCOPE'], BINSCOPE_EXE)

Seems like you might as well just require %BINSCOPE% to be set to the full path to the .exe here.

@@ +74,5 @@
> +# variable as the path where binscope.exe is located 
> +try:
> +  binscope_path = os.path.join(os.environ['BINSCOPE'], BINSCOPE_EXE)
> +except KeyError:
> +  print "Failed when trying to get the value of the BINSCOPE environment variable, skipping this check"

I'd probably phrase this as "BINSCOPE environment variable is not set, can't check DEP/ASLR status."

@@ +78,5 @@
> +  print "Failed when trying to get the value of the BINSCOPE environment variable, skipping this check"
> +  sys.exit(0)
> +  
> +try:    
> +  proc1 = subprocess.Popen([binscope_path, "/target", binary_path,

You can probably just name this "proc" since you only do one invocation now.

@@ +101,5 @@
> +output = proc1.communicate()[0]
> +
> +# is this a PASS or a FAIL ? 
> +if proc1.returncode != 0:
> +  print "TEST-UNEXPECTED-FAIL |autobinscope.py| %s is missing a needed Windows protection, such as /GS or ASLR" % binary_path

Nit: spaces around the | characters (here and below).

@@ +105,5 @@
> +  print "TEST-UNEXPECTED-FAIL |autobinscope.py| %s is missing a needed Windows protection, such as /GS or ASLR" % binary_path
> +else:
> +  print "TEST-PASS |autobinscope.py| %s succeeded" % binary_path
> +
> +# we're done here.

This comment seems superfluous.
Comment 49 Ian Melven :imelven 2011-11-14 15:12:47 PST
note to self for when i do the last fix up prior to landing : in bug 700513 releng has BINSCOPE pointing to the actual .exe - so take this into consideration when making final review edits as above
Comment 50 Ian Melven :imelven 2011-11-17 14:26:18 PST
Created attachment 575298 [details] [diff] [review]
patch v5

(hopefully) final version, addressing ted's r+ comment - carrying ted's r+ over. will do a try run and mark checkin-needed if everything looks good there.
Comment 51 Ian Melven :imelven 2011-11-18 12:58:10 PST
Successful try run : https://tbpl.mozilla.org/?tree=Try&rev=340fff0b9d57

marking checkin-needed
Comment 53 Ed Morley [:emorley] 2011-11-20 14:22:02 PST
https://hg.mozilla.org/mozilla-central/rev/c3a085773624
Comment 54 Justin Wood (:Callek) 2011-11-21 08:58:04 PST
In skimming my SeaMonkey make check log I noticed a reference to firefox.exe, so double checked and I should have caught this earlier.

Got r=khuey over IRC for the fix:

https://hg.mozilla.org/integration/mozilla-inbound/rev/069e4da44a5f
Comment 55 Ted Mielczarek [:ted.mielczarek] 2011-11-21 09:23:34 PST
Oops, I missed that. Thanks for the fix!
Comment 56 Ed Morley [:emorley] 2011-11-21 19:11:15 PST
https://hg.mozilla.org/mozilla-central/rev/069e4da44a5f
Comment 57 Tim Abraldes [:TimAbraldes] [:tabraldes] 2014-06-05 13:25:54 PDT
The patch in this bug only made us run BinScope on firefox.exe and plugin-container.exe

Notably, we're not running BinScope against libEGL.dll and libGLESv2.dll (mentioned in comment 0) and we're not running BinScope against xul.dll

I've filed bug 1021214 for running BinScope against the rest of our DLLs and EXEs

Note You need to log in before you can comment on or make changes to this bug.