[lttng-dev] [lttng-tools] Fix: channel name with '.' and '/' brings problems
Jérémie Galarneau
jeremie.galarneau at efficios.com
Thu Nov 27 11:54:54 EST 2014
On Wed, Nov 26, 2014 at 11:59 PM, Philippe Proulx
<eeppeliteloop at gmail.com> wrote:
> On Wed, Nov 26, 2014 at 10:46 PM, Jonathan Rajotte
> <jonathan.r.julien at gmail.com> wrote:
>> Hey Phil,
>>
>> Not sure if modifying user input without any warnings about it is a good
>> idea. This clearly solve problems but do we want to solve it this way ?
>>
>> It might be a better idea to warn the user about improper channel name or
>> simply block the command and return an error.
>
> Hi Jo,
>
> I think you are right. I forgot to write about this, but I thought
> about it, and I think too
> that it's a good idea. The only reason I didn't add warnings was that
> the user input
> is already (potentially) modified without warnings:
>
> strncpy(chan.name, channel_name, NAME_MAX);
> chan.name[NAME_MAX - 1] = '\0';
>
> That is, channel names are truncated to (NAME_MAX - 1) characters:
>
> lttng enable-channel -k aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\
> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\
> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\
> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\
> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\
> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\
> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\
> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\
> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
>
> The effective channel name is 254 'a's.
>
> So, first question: should the name be modified at all, or should an
> error be displayed
> when the name is invalid? I'm in favor of the second option.
Agreed.
>
> Second question: if an error is to be displayed, should the validation
> happen in the
> lttng CLI or in the session daemon? I'm in favor of the second option,
> as, for example,
> session names are currently validaded on the session daemon side, which is legit
> since validation should be done by the model ultimately.
>
Agreed, again. I think the protocol will not allow lttng-ctl to send a
session name > 255 chars, so that may be an exception to handle on the
client side. As for the rest, as long as we return a clear error code
which the client can use to display a better error message than
"Session creation failed", I'm all for it!
> I'll let the maintainer decide whatever is the best direction for
> this. Honestly, I was
> expecting a nack for this patch, which is more a discussion starter
> than anything
> else ;-)
Good, NACK it is then! ;)
Please feel free to submit a patch or to include this discussion in a
new bug entry.
Thanks!
Jérémie
>
> Phil
>>
>> On Wed, Nov 26, 2014 at 10:32 PM, Philippe Proulx <eeppeliteloop at gmail.com>
>> wrote:
>>>
>>> This patch ensures:
>>>
>>> 1. A channel name does not contain any '/' character, since
>>> relative paths may be injected in the channel name
>>> otherwise (knowing that the channel name is eventually
>>> part of a file name)
>>> 2. A channel name does not start with a '.' character, since
>>> trace readers (Babeltrace is one of them) could interpret
>>> files starting with a dot as hidden files and ignore
>>> them when opening the CTF trace
>>>
>>> Fixes: #751
>>>
>>> Signed-off-by: Philippe Proulx <eeppeliteloop at gmail.com>
>>> ---
>>> src/bin/lttng/commands/enable_channels.c | 38
>>> +++++++++++++++++++++++++++-----
>>> 1 file changed, 33 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/src/bin/lttng/commands/enable_channels.c
>>> b/src/bin/lttng/commands/enable_channels.c
>>> index f8272e9..e6cce49 100644
>>> --- a/src/bin/lttng/commands/enable_channels.c
>>> +++ b/src/bin/lttng/commands/enable_channels.c
>>> @@ -275,11 +275,39 @@ static int enable_channel(char *session_name)
>>> /* Strip channel list (format: chan1,chan2,...) */
>>> channel_name = strtok(opt_channels, ",");
>>> while (channel_name != NULL) {
>>> - /* Copy channel name and normalize it */
>>> + /* Copy channel name, sanitize and normalize it */
>>> strncpy(chan.name, channel_name, NAME_MAX);
>>> chan.name[NAME_MAX - 1] = '\0';
>>>
>>> - DBG("Enabling channel %s", channel_name);
>>> + char *src, *dst;
>>> + int got_first = 0;
>>> +
>>> + for (src = dst = chan.name; *src != '\0'; ++src) {
>>> + *dst = *src;
>>> +
>>> + /*
>>> + * Channel name could be used in file names, so
>>> remove
>>> + * invalid '/'
>>> + */
>>> + if (*dst != '/') {
>>
>>
>> Maybe send some warning here ?
>>
>>>
>>> + /*
>>> + * Remove starting dots since this could
>>> create
>>> + * file names starting with dots, and
>>> trace
>>> + * readers could interpret them as hidden
>>> files
>>> + * and ignore them.
>>> + */
>>> + if (*dst != '.') {
>>
>>
>> Same
>>
>>>
>>> + got_first = 1;
>>> + dst++;
>>> + } else if (got_first) {
>>> + dst++;
>>> + }
>>> + }
>>> + }
>>> +
>>> + *dst = '\0';
>>> +
>>> + DBG("Enabling channel %s", chan.name);
>>>
>>> ret = lttng_enable_channel(handle, &chan);
>>> if (ret < 0) {
>>> @@ -288,19 +316,19 @@ static int enable_channel(char *session_name)
>>> case LTTNG_ERR_KERN_CHAN_EXIST:
>>> case LTTNG_ERR_UST_CHAN_EXIST:
>>> case LTTNG_ERR_CHAN_EXIST:
>>> - WARN("Channel %s: %s (session %s)",
>>> channel_name,
>>> + WARN("Channel %s: %s (session %s)",
>>> chan.name,
>>> lttng_strerror(ret),
>>> session_name);
>>> warn = 1;
>>> break;
>>> default:
>>> - ERR("Channel %s: %s (session %s)",
>>> channel_name,
>>> + ERR("Channel %s: %s (session %s)",
>>> chan.name,
>>> lttng_strerror(ret),
>>> session_name);
>>> error = 1;
>>> break;
>>> }
>>> } else {
>>> MSG("%s channel %s enabled for session %s",
>>> - get_domain_str(dom.type),
>>> channel_name, session_name);
>>> + get_domain_str(dom.type),
>>> chan.name, session_name);
>>> success = 1;
>>> }
>>>
>>> --
>>> 2.1.3
>>>
>>>
>>> _______________________________________________
>>> lttng-dev mailing list
>>> lttng-dev at lists.lttng.org
>>> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
>>
>>
>>
>>
>> --
>> Jonathan Rajotte Julien
>
> _______________________________________________
> lttng-dev mailing list
> lttng-dev at lists.lttng.org
> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
--
Jérémie Galarneau
EfficiOS Inc.
http://www.efficios.com
More information about the lttng-dev
mailing list