Message-ID: <928c76bf-39bc-cbc0-373e-70c5561cd5b0@redhat.com>
Date:   Wed, 11 Jan 2023 14:18:12 +0100
From:   Andreas Gerstmayr <agerstmayr@...hat.com>
To:     Ian Rogers <irogers@...gle.com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
        996839@...s.debian.org, Brendan Gregg <brendan@...el.com>,
        spiermar@...il.com
Subject: Re: [PATCH v1] perf script flamegraph: Avoid d3-flame-graph package
 dependency

On 10.01.23 20:51, Ian Rogers wrote:
> On Tue, Jan 10, 2023 at 10:02 AM Andreas Gerstmayr
> <agerstmayr@...hat.com> wrote:
>>
>> On 05.01.23 10:24, Ian Rogers wrote:
>>> On Wed, Jan 4, 2023 at 7:04 PM Ian Rogers <irogers@...gle.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@...gle.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

Powered by blists - more mailing lists