Bug 339405 - [PATCH] Adds ability to invoke a script in order to determine a log-file name for a child being forked
Summary: [PATCH] Adds ability to invoke a script in order to determine a log-file name...
Status: REPORTED
Alias: None
Product: valgrind
Classification: Developer tools
Component: general (other bugs)
Version First Reported In: 3.10 SVN
Platform: Compiled Sources All
: NOR normal
Target Milestone: ---
Assignee: Julian Seward
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-09-26 02:59 UTC by Eliot Moss
Modified: 2024-05-10 06:49 UTC (History)
5 users (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments
Patch that adds this feature (modifies one source code file and one documentation file) (6.53 KB, patch)
2014-09-26 02:59 UTC, Eliot Moss
Details
Updated patch that addresses comments (7.97 KB, patch)
2014-11-20 16:09 UTC, Eliot Moss
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Eliot Moss 2014-09-26 02:59:31 UTC
Created attachment 88843 [details]
Patch that adds this feature (modifies one source code file and one documentation file)

--log-file= current supports expanding a pid or an environment variable to parameterize the name used for logging output.  This patch adds the ability to invoke a script that creates and outputs the file name to use.  It also patches the documentation, explaining this new feature.
Comment 1 Julian Seward 2014-09-26 16:43:43 UTC
Interesting.  Are there any security implications of this?  I am
not asserting that there are, but was just wondering if there were.
Comment 2 Eliot Moss 2014-09-26 18:11:41 UTC
On 9/26/2014 12:43 PM, Julian Seward wrote:
> https://bugs.kde.org/show_bug.cgi?id=339405
>
> --- Comment #1 from Julian Seward <jseward@acm.org> ---
> Interesting.  Are there any security implications of this?  I am
> not asserting that there are, but was just wondering if there were.

I can't think of any, but I'm not a security expert.  The script
is run via "/bin/bash -c" so does whatever you can do that way.
I don't see that anything acquires privileges, etc., that it
would not otherwise have.

Regards -- Eliot
Comment 3 Philippe Waroquiers 2014-09-27 13:11:52 UTC
(In reply to Eliot Moss from comment #2)
>
> I can't think of any, but I'm not a security expert.  The script
> is run via "/bin/bash -c" so does whatever you can do that way.
Note that the patch uses rather /bin/sh -c
> I don't see that anything acquires privileges, etc., that it
> would not otherwise have.
I also do not see how that would give additional privileges to the user
launching Valgrind. The only possible problem would be via setuid program,
but I guess there is no way to have valgrind running setuid programs (otherwise,
that would already be a security problem).

Some minor comments on the patch:
* /bin/sh -c can run any kind of command, not only scripts.
* Maybe the part that launches the program and recuperate the output could
   be a separate function ? There is already something very similar in
     m_gdbserver/target.c which launches a program and recuperates the output.
   Either that could be done with an interface like popen(3)
   or (assuming this is all for programs producing small output), something like
   HChar*   VG_(launch_a_program_and_allocates_memory_containing_output)( HChar*prog, ...)
   (with a better name :)
   or whatever that factorises the 2.
* Use HChar* instead of Char* ?
Comment 4 Florian Krohm 2014-09-27 13:45:25 UTC
(In reply to Eliot Moss from comment #2)
> On 9/26/2014 12:43 PM, Julian Seward wrote:
> > https://bugs.kde.org/show_bug.cgi?id=339405
> >
> > --- Comment #1 from Julian Seward <jseward@acm.org> ---
> > Interesting.  Are there any security implications of this?  I am
> > not asserting that there are, but was just wondering if there were.
> 
> I can't think of any, but I'm not a security expert.  The script
> is run via "/bin/bash -c" so does whatever you can do that way.
> I don't see that anything acquires privileges, etc., that it
> would not otherwise have.
> 
> Regards -- Eliot

+ ...... This
+      specifier is rarely needed, but very useful in certain circumstances,
+      such as when a program invokes a number of child processes whose logs
+      need to go to different places based factors difficult to express with
+      the <option>%p</option> or <option>%q</option> options. 

parse error :)

Minor comment: no TABs in the source code please.
Comment 5 Eliot Moss 2014-09-27 14:31:06 UTC
On 9/27/2014 9:45 AM, Florian Krohm wrote:

> + ...... This
> +      specifier is rarely needed, but very useful in certain circumstances,
> +      such as when a program invokes a number of child processes whose logs
> +      need to go to different places based factors difficult to express with
> +      the <option>%p</option> or <option>%q</option> options.
>
> parse error :)

Yes, I see the omitted word now.  Concerning TABs, I try not to have them,
but I guess some got by me.

What's the protocol?  Should I make the suggested changes or will a valgrind
developer do that?

Regards -- Eliot
Comment 6 Eliot Moss 2014-11-20 16:09:53 UTC
Created attachment 89653 [details]
Updated patch that addresses comments

This updates the previous patch by: removing tabs from source code, changing Char to HChar, and fixing English grammar in the doc file.  It builds under 14740 (the current head).
Comment 7 Eliot Moss 2014-11-20 16:10:29 UTC
In hope that this patch might be included in the upcoming release ...
Comment 8 Costas Skarakis 2024-05-09 14:35:17 UTC
Hello from the future! I'd really appreciate to see this added to a valgrind release. Is it still possible?