[lttng-dev] [PATCH] A minimal rewrite of lttctl
Matthew Khouzam
matthew.khouzam at ericsson.com
Thu Dec 8 11:53:59 EST 2011
wow! sorry about that last post
if (opt_head = (struct lttctl_option *)malloc(sizeof(struct lttctl_option))
{
return enomem;
}
I would be lost without a compiler.
On 11-12-08 10:29 AM, Matthew Khouzam wrote:
> Please correct me if I'm wrong, but shouldn't the failure to malloc
> return an enomem like :
> if opt_head = (struct lttctl_option *)malloc(sizeof(struct lttctl_option));
> {
> return enomem;
> }
> because it is hiding a larger more system destabilizing problem.
>
> please others comment.
> On 11-12-07 03:27 PM, Thibault, Daniel wrote:
>> ------------------------------
>>> Date: Tue, 6 Dec 2011 12:04:26 -0500
>>> From: Mathieu Desnoyers <compudj at krystal.dyndns.org>
>>>
>>> If you can split out the obvious bugfixes from the refactoring/rewrite,
>>> I would gladly accept the bugfixes.
>> ------------------------------
>>
>> All right, here is a minimal patch for lttctl.c:
>>
>> ------------------------------
>> --- ../ltt-control-0.89-792f03c/lttctl/lttctl.c 2011-05-12 08:44:27.000000000 -0400
>> +++ ../ltt-control-0.89-792f03c.minimalchanges/lttctl/lttctl.c 2011-12-07 15:18:28.000000000 -0500
>> @@ -64,7 +64,7 @@
>> struct lttctl_option *next;
>> };
>>
>> -struct lttctl_option *opt_head, *last_opt;
>> +static struct lttctl_option *opt_head, *last_opt;
>>
>> static int opt_create;
>> static int opt_destroy;
>> @@ -236,9 +236,11 @@
>>
>> if (!opt_head) {
>> opt_head = (struct lttctl_option *)malloc(sizeof(struct lttctl_option));
>> + if (opthead) {
>> init_channel_opt(&opt_head->opt_mode.chan_opt, opt_name);
>> opt_head->type = CHANNEL;
>> opt_head->next = NULL;
>> + }
>> last_opt = opt_head;
>> return opt_head;
>> }
>> @@ -251,15 +253,17 @@
>> }
>>
>> new_opt = (struct lttctl_option *)malloc(sizeof(struct lttctl_option));
>> + if (new_opt) {
>> init_channel_opt(&new_opt->opt_mode.chan_opt, opt_name);
>> new_opt->type = CHANNEL;
>> new_opt->next = NULL;
>> last_opt->next = new_opt;
>> last_opt = new_opt;
>> + }
>> return new_opt;
>> }
>>
>> -int set_channel_opt(struct channel_option *opt, char *opt_name, char *opt_valstr)
>> +static int set_channel_opt(struct channel_option *opt, char *opt_name, char *opt_valstr)
>> {
>> int opt_val, ret;
>> ------------------------------
>>
>> It makes opt_head and last_opt private.
>> In find_insert_channel_opt, it handles malloc() failure; previously, if malloc() failed while creating a new lttctl_option list node, one immediately got a segmentation fault.
>> It makes set_channel_opt() private (there is no reason why it should be exportable, and all the other functions in this unit are private).
>>
>> Here is a minimal patch for liblttctl.c:
>>
>> ------------------------------
>> --- ../ltt-control-0.89-792f03c/liblttctl/liblttctl.c 2011-05-12 08:44:27.000000000 -0400
>> +++ ../ltt-control-0.89-792f03c.minimalchanges/liblttctl/liblttctl.c 2011-12-07 15:03:31.000000000 -0500
>> @@ -35,7 +35,7 @@
>> #include <fcntl.h>
>> #include <stdlib.h>
>>
>> -#define MAX_CHANNEL (256)
>> +//#define MAX_CHANNEL (256)
>>
>> static char debugfsmntdir[PATH_MAX];
>>
>> @@ -107,11 +107,6 @@
>> {
>> int fd;
>>
>> - if (!fname) {
>> - fprintf(stderr, "%s: args invalid\n", __func__);
>> - return 1;
>> - }
>> -
>> fd = open(fname, O_WRONLY);
>> if (fd == -1) {
>> fprintf(stderr, "%s: open %s failed: %s\n", __func__, fname,
>> @@ -227,6 +222,10 @@
>> continue;
>> old_list = list;
>> list = malloc(sizeof(char *) * ++nr_chan);
>> + if (!list) {
>> + nr_chan = -ENOMEM;
>> + break;
>> + }
>> memcpy(list, old_list, sizeof(*list) * (nr_chan - 1));
>> free(old_list);
>> list[nr_chan - 1] = strdup(dirent->d_name);
>> @@ -405,7 +404,7 @@
>> int ret;
>> char ctlfname[PATH_MAX];
>>
>> - if (!name) {
>> + if (!name || !trans) {
>> fprintf(stderr, "%s: args invalid\n", __func__);
>> ret = -EINVAL;
>> goto arg_error;
>> @@ -477,9 +476,10 @@
>> goto op_err;
>> }
>>
>> - for (; n_channel > 0; n_channel--) {
>> + int ch = 0;
>> + for ( ; ch < n_channel; ch++) {
>> ret = __lttctl_set_channel_enable(name,
>> - channellist[n_channel - 1], enable);
>> + channellist[ch], enable);
>> if (ret)
>> goto op_err_clean;
>> }
>> @@ -541,9 +541,10 @@
>> goto op_err;
>> }
>>
>> - for (; n_channel > 0; n_channel--) {
>> + int ch = 0;
>> + for ( ; ch < n_channel; ch++) {
>> ret = __lttctl_set_channel_overwrite(name,
>> - channellist[n_channel - 1], overwrite);
>> + channellist[ch], overwrite);
>> if (ret)
>> goto op_err_clean;
>> }
>> @@ -609,9 +610,10 @@
>> goto op_err;
>> }
>>
>> - for (; n_channel > 0; n_channel--) {
>> + int ch = 0;
>> + for ( ; ch < n_channel; ch++) {
>> ret = __lttctl_set_channel_subbuf_num(name,
>> - channellist[n_channel - 1], subbuf_num);
>> + channellist[ch], subbuf_num);
>> if (ret)
>> goto op_err_clean;
>> }
>> @@ -677,9 +679,10 @@
>> goto op_err;
>> }
>>
>> - for (; n_channel > 0; n_channel--) {
>> + int ch = 0;
>> + for ( ; ch < n_channel; ch++) {
>> ret = __lttctl_set_channel_subbuf_size(name,
>> - channellist[n_channel - 1], subbuf_size);
>> + channellist[ch], subbuf_size);
>> if (ret)
>> goto op_err_clean;
>> }
>> @@ -745,9 +748,10 @@
>> goto op_err;
>> }
>>
>> - for (; n_channel > 0; n_channel--) {
>> + int ch = 0;
>> + for ( ; ch < n_channel; ch++) {
>> ret = __lttctl_set_channel_switch_timer(name,
>> - channellist[n_channel - 1], switch_timer);
>> + channellist[ch], switch_timer);
>> if (ret)
>> goto op_err_clean;
>> }
>> @@ -769,6 +773,9 @@
>> char mnt_type[PATH_MAX];
>> int trymount_done = 0;
>>
>> + if (!mntdir)
>> + return -EINVAL;
>> +
>> FILE *fp = fopen("/proc/mounts", "r");
>> if (!fp)
>> return -EINVAL;
>> @@ -779,14 +786,19 @@
>> break;
>>
>> if (!strcmp(mnt_type, "debugfs")) {
>> + // 4 for the LTT_PATH "/ltt", 9 for the LTT_CONTROL_PATH "/control/"
>> + // 9 for the LTT_CHANNEL_PATH "/channel/", 13 for the LTT_BUFFERS_TIMER_PATH "/switch_timer"
>> + // NAME_MAX for the trace name and the channel name
>> + if (strlen(mnt_dir) >= (PATH_MAX - (4 + 9 + 9 + 13 + 2*NAME_MAX)))
>> + return -ENOENT;
>> strcpy(mntdir, mnt_dir);
>> return 0;
>> }
>> }
>>
>> if (!trymount_done) {
>> - mount("debugfs", "/sys/kernel/debug/", "debugfs", 0, NULL);
>> trymount_done = 1;
>> + if (!mount("debugfs", "/sys/kernel/debug/", "debugfs", 0, NULL))
>> goto find_again;
>> }
>> ------------------------------
>>
>> It comments out the MAX_CHANNEL define, which is unused.
>> It removes the lttctl_sendop() parameter check, which is unnecessary (this private function will always get valid arguments).
>> It handles malloc() failure in lttctl_get_channellist(); previously, if malloc() failed while growing the channellist, one immediately got a segmentation fault.
>> It fixes the missing parameter check in lttctl_set_trans().
>> In each of the lttctl_set_channel_*() functions, it fixes a memory leak: the original code would count n_channel down to zero before passing it to lttctl_free_channellist(), so that none of the strings stored in the channellist array were freed.
>> It fixes the missing parameter check in getdebugfsmntdir().
>> It limits the length of the debugfs mounting point path, based on the longest sub-path possible (generously using PATH_MAX for the channel name, which in practice is limited to 13 characters). Otherwise, whenever sprintf is used to build a debugfs path string, the string buffer could be overrun.
>> Finally, it prevents getdebugfsmntdir() from scanning /proc/mounts uselessly when the mount command fails.
>>
>> I've also written (but omitted from this patch) the following:
>>
>> * A version of lttctl_sendop that handles the case where the write() is incomplete. This can potentially occur, but is so rare (I think it happens only if a signal interrupts the write) that it may not be worthwhile to fix it.
>>
>> * A version of lttctl_get_channellist that uses readdir_r for thread safety. Is this worthwhile?
>>
>> Daniel U. Thibault
>> R & D pour la défense Canada - Valcartier (RDDC Valcartier) / Defence R&D Canada - Valcartier (DRDC Valcartier)
>> Système de systèmes (SdS) / System of Systems (SoS)
>> Solutions informatiques et expérimentations (SIE) / Computing Solutions and Experimentations (CSE)
>> 2459 Boul. Pie XI Nord
>> Québec, QC G3J 1X5
>> CANADA
>> Vox : (418) 844-4000 x4245
>> Fax : (418) 844-4538
>> NAC: 918V QSDJ
>> Gouvernement du Canada / Government of Canada
>> <http://www.valcartier.drdc-rddc.gc.ca/>
>>
>> _______________________________________________
>> lttng-dev mailing list
>> lttng-dev at lists.lttng.org
>> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
> _______________________________________________
> lttng-dev mailing list
> lttng-dev at lists.lttng.org
> http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
More information about the lttng-dev
mailing list