Messages in this thread Patch in this message | | | Date | Wed, 11 Jan 2023 14:18:12 +0100 | Subject | Re: [PATCH v1] perf script flamegraph: Avoid d3-flame-graph package dependency | From | Andreas Gerstmayr <> |
| |
On 10.01.23 20:51, Ian Rogers wrote: > On Tue, Jan 10, 2023 at 10:02 AM Andreas Gerstmayr > <agerstmayr@redhat.com> wrote: >> >> On 05.01.23 10:24, Ian Rogers wrote: >>> On Wed, Jan 4, 2023 at 7:04 PM Ian Rogers <irogers@google.com> wrote: >>>> >>>> Currently flame graph generation requires a d3-flame-graph template to >>>> be installed. Unfortunately this is hard to come by for things like >>>> Debian [1]. If the template isn't installed warn and download it from >>>> jsdelivr CDN. If downloading fails generate a minimal flame graph >>>> again with the javascript coming from jsdelivr CDN. >>>> >>>> [1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=996839 >>>> >>>> Signed-off-by: Ian Rogers <irogers@google.com> >> >> I'm not entirely convinced that this perf script should make a network >> request. In my opinion >> >> * best would be if this template gets packaged in Debian >> * alternatively print instructions how to install the template: >> >> sudo mkdir /usr/share/d3-flame-graph >> sudo wget -O /usr/share/d3-flame-graph/d3-flamegraph-base.html >> https://cdn.jsdelivr.net/npm/d3-flame-graph@4/dist/templates/d3-flamegraph-base.html >> >> * or eventually just embed the entire template (66 KB) in the Python >> file (not sure if this is permissible though, it's basically a minified >> HTML/JS blob)? >> * if the above options don't work, I'd recommend asking the user before >> making the network request, and eventually persist the template >> somewhere so it doesn't need to be downloaded every time >> >> What do you think? > > So the script following this patch is going to try 3 things: > 1) look for a local template HTML, > 2) if a local template isn't found warn and switch to downloading the > CDN version, > 3) if the CDN version fails to download then use a minimal (embedded > in the script) HTML file with JS dependencies coming from the CDN. > > For (1) there are references in the generated HTML to www.w3.org, > meaning the HTML isn't fully hermetic - although these references may > not matter.
The references to w3.org are XML namespace names. As far as I'm aware they do not matter, as they are merely identifiers and are never accessed [1,2]. Therefore the generated HTML file in its current form is hermetic.
This was a design decision from the start - the flame graph generation and the resulting HTML file should not use any external resources loaded over the network (the external host could be inaccessible or compromised at any time). I understand that this requirement may not apply to every user or company.
> For (2) there will be a download from the CDN of the template during > the execution of flamegraph. The template is better than (3) as it > features additional buttons letting you zoom out, etc. The HTML will > contain CDN references. > For (3) the generated HTML isn't hermetic and will use the CDN. > > For (2) we could give a prompt before trying to download the template, > or we could change it so that we give the wget command. I wouldn't > have the wget command require root privileges, I'd say that the > template could be downloaded and then the path of the download given > as an option to the flamegraph program. Downloading the file and then > adding an option to flamegraph seems inconvenient to the user, > something that the use of urllib in the patch avoids - it is > essentially just doing this for the user without storing the file on > disk. I think I prefer to be less inconvenient, and so the state of > the code at the moment looks best to me. Given that no choice results > in a fully hermetic HTML file, they seem similar in this regard. Is it > okay to try a download with urllib? Should there be a prompt? I think > the warning is enough and allows scripts, etc. using flamegraph to > more easily function across differing distributions.
I fully agree, your changes make the flame graph generation more convenient in case the template is not installed. Also, downloading the template to a local directory and having to specify the path to the template every time is an inconvenience.
I think it is a tradeoff between convenience and security/privacy. jsdelivr can get compromised, serving malicious JS (well, in that case, printing instructions to jsdelivr is also flawed :|). In the end it's up to the maintainers to decide.
> An aside, Namhyung pointed out to me that there is a "perf report -g > folded" option, that was added in part to simplify creating > flamegraphs: > http://lkml.kernel.org/r/1447047946-1691-2-git-send-email-namhyung@kernel.org > Building off of perf report may improve/simplify the flamegraph code, > or avoid the requirement that libpython be linked against perf.
Yep, in this case another tool is required to generate the flame graph visualization, for example [3].
[1] https://www.w3.org/TR/xml-names11/ [2] https://developer.mozilla.org/en-US/docs/Web/SVG/Namespaces_Crash_Course [3] https://github.com/brendangregg/FlameGraph
Cheers, Andreas
> > Thanks, > Ian > > > > >> >> Cheers, >> Andreas >> >>>> --- >>>> tools/perf/scripts/python/flamegraph.py | 63 ++++++++++++++++++------- >>>> 1 file changed, 45 insertions(+), 18 deletions(-) >>>> >>>> diff --git a/tools/perf/scripts/python/flamegraph.py b/tools/perf/scripts/python/flamegraph.py >>>> index b6af1dd5f816..808b0e1c9be5 100755 >>>> --- a/tools/perf/scripts/python/flamegraph.py >>>> +++ b/tools/perf/scripts/python/flamegraph.py >>>> @@ -25,6 +25,27 @@ import io >>>> import argparse >>>> import json >>>> import subprocess >>>> +import urllib.request >>>> + >>>> +minimal_html = """<head> >>>> + <link rel="stylesheet" type="text/css" href="https://cdn.jsdelivr.net/npm/d3-flame-graph@4.1.3/dist/d3-flamegraph.css"> >>> >>> (hopefully fixed Martin Spier's e-mail address) >>> >>> The @4.1.3 comes from the README.md: >>> https://github.com/spiermar/d3-flame-graph/blob/master/README.md >>> Does it make sense just to drop it or use @latest ? It'd be nice not >>> to patch this file for every d3-flame-graph update. >>> >>> Thanks, >>> Ian >>> >>>> +</head> >>>> +<body> >>>> + <div id="chart"></div> >>>> + <script type="text/javascript" src="https://d3js.org/d3.v7.js"></script> >>>> + <script type="text/javascript" src="https://cdn.jsdelivr.net/npm/d3-flame-graph@4.1.3/dist/d3-flamegraph.min.js"></script> >>>> + <script type="text/javascript"> >>>> + const stacks = [/** @flamegraph_json **/]; >>>> + // Note, options is unused. >>>> + const options = [/** @options_json **/]; >>>> + >>>> + var chart = flamegraph(); >>>> + d3.select("#chart") >>>> + .datum(stacks[0]) >>>> + .call(chart); >>>> + </script> >>>> +</body> >>>> +""" >>>> >>>> # pylint: disable=too-few-public-methods >>>> class Node: >>>> @@ -50,15 +71,18 @@ class FlameGraphCLI: >>>> self.args = args >>>> self.stack = Node("all", "root") >>>> >>>> - if self.args.format == "html" and \ >>>> - not os.path.isfile(self.args.template): >>>> - print("Flame Graph template {} does not exist. Please install " >>>> - "the js-d3-flame-graph (RPM) or libjs-d3-flame-graph (deb) " >>>> - "package, specify an existing flame graph template " >>>> - "(--template PATH) or another output format " >>>> - "(--format FORMAT).".format(self.args.template), >>>> - file=sys.stderr) >>>> - sys.exit(1) >>>> + if self.args.format == "html": >>>> + if os.path.isfile(self.args.template): >>>> + self.template = f"file://{self.args.template}" >>>> + else: >>>> + print(f""" >>>> +Warning: Flame Graph template '{self.args.template}' >>>> +does not exist, a template will be downloaded via http. To avoid this >>>> +please install a package such as the js-d3-flame-graph or >>>> +libjs-d3-flame-graph, specify an existing flame graph template >>>> +(--template PATH) or another output format (--format FORMAT). >>>> +""", file=sys.stderr) >>>> + self.template = "https://cdn.jsdelivr.net/npm/d3-flame-graph@4.1.3/dist/templates/d3-flamegraph-base.html" >>>> >>>> @staticmethod >>>> def get_libtype_from_dso(dso): >>>> @@ -129,15 +153,18 @@ class FlameGraphCLI: >>>> options_json = json.dumps(options) >>>> >>>> try: >>>> - with io.open(self.args.template, encoding="utf-8") as template: >>>> - output_str = ( >>>> - template.read() >>>> - .replace("/** @options_json **/", options_json) >>>> - .replace("/** @flamegraph_json **/", stacks_json) >>>> - ) >>>> - except IOError as err: >>>> - print("Error reading template file: {}".format(err), file=sys.stderr) >>>> - sys.exit(1) >>>> + with urllib.request.urlopen(self.template) as template: >>>> + output_str = '\n'.join([ >>>> + l.decode('utf-8') for l in template.readlines() >>>> + ]) >>>> + except Exception as err: >>>> + print(f"Error reading template {self.template}: {err}\n" >>>> + "a minimal flame graph will be generated", file=sys.stderr) >>>> + output_str = minimal_html >>>> + >>>> + output_str = output_str.replace("/** @options_json **/", options_json) >>>> + output_str = output_str.replace("/** @flamegraph_json **/", stacks_json) >>>> + >>>> output_fn = self.args.output or "flamegraph.html" >>>> else: >>>> output_str = stacks_json >>>> -- >>>> 2.39.0.314.g84b9a713c41-goog >>>> >>> >> >> -- >> Red Hat Austria GmbH, Registered seat: A-1200 Vienna, Millennium Tower, >> 26.floor, Handelskai 94-96, Austria >> Commercial register: Commercial Court Vienna, FN 479668w >> >
-- Red Hat Austria GmbH, Registered seat: A-1200 Vienna, Millennium Tower, 26.floor, Handelskai 94-96, Austria Commercial register: Commercial Court Vienna, FN 479668w
| |