[PATCH] D11877: [Support] On Windows, generate PDF files for graphs and open with associated viewer

[Aaron Ballman via llvm-commits]

On Mon, Aug 17, 2015 at 1:24 PM, Michael Kruse <llvm at meinersbur.de> wrote:
> 2015-08-17 18:59 GMT+02:00 Aaron Ballman <aaron at aaronballman.com>:
>> I would rename the enumerator prefixes to VK_ (for ViewerKind).
>
> OK
>
>
>>>>> +#ifdef LLVM_ON_WIN32
>>>>> +  if (!Viewer && S.TryFindProgram("cmd", ViewerPath)) {
>>>>> +    Viewer = PDFV_CmdStart;
>>>>> +  }
>>>>
>>>> Is there an advantage to using cmd instead of manually opening the
>>>> file with the related PDF viewer via ShellExecuteEx (or one of the
>>>> other Shell APIs)?
>>>
>>> The Support cross-platform layer doesn't expose such API (at least I
>>> am not aware of). Since this function is only useful at debug-time,
>>> calling cmd seems like a sufficient solution to me. I am not sure
>>> whether including Windows.h into this file will find general
>>> acceptance.
>>
>> That's certainly fair. I'm just hesitant to spin up an entire command
>> processor and pass flags to it that aren't heavily vetted (I'm not
>> certain how much of this is under user control, so this is a bit of a
>> red flag to me). For instance, does this open up a way for this
>> feature to be abused to run arbitrary commands on a server-hosted
>> Clang (like coliru or wandbox)?
>
> The only argument passed is the filename which is generated in the
> temp directory, no user string passed.

Looking further at the code, the user doesn't have direct control over
that filename, so I'm considerably less worried.

> If there is was a security issue it would be as severe as before since
> on other systems. E.g. xdg-open could open an URL in a browser with an
> security exploit for the browser which opens.

That's not quite true as before we were not allowing you to launch
cmd, so this would be an additional attack vector. Thankfully, that's
not an issue here.

> But if ShellExecuteEx is the preferred solution I can also make this happen.

I'm less convinced that ShellExecuteEx is needed here specifically. I
suspect we will want something more general exposed via Support
someday, but that's not a problem for this.

Thanks!

~Aaron