On Wed, Nov 26, 2014 at 11:59 PM, Philippe Proulx <[email protected]> wrote: > On Wed, Nov 26, 2014 at 10:46 PM, Jonathan Rajotte > <[email protected]> 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 <[email protected]> >> 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 <[email protected]> >>> --- >>> 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 >>> [email protected] >>> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev >> >> >> >> >> -- >> Jonathan Rajotte Julien > > _______________________________________________ > lttng-dev mailing list > [email protected] > http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev -- Jérémie Galarneau EfficiOS Inc. http://www.efficios.com _______________________________________________ lttng-dev mailing list [email protected] http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
