[Pdbg] [PATCH v3 09/11] main: Overhaul target selection
Alistair Popple
alistair at popple.id.au
Thu May 24 22:25:44 AEST 2018
Thanks Amitay, fixing this has been on the list for a while. A couple of
comments below though.
On Thursday, 24 May 2018 3:50:15 PM AEST Amitay Isaacs wrote:
> +/* Parse argument of the form 0-5,7,9-11,15,17 */
> +static bool parse_list(const char *arg, int max, int *list, int *count)
> +{
> + char str[strlen(arg)+1];
> + char *tok, *tmp, *saveptr = NULL;
> + int n = 0, i, j;
> +
> + strcpy(str, arg);
> +
> + tmp = str;
> + while ((tok = strtok_r(tmp, ",", &saveptr)) != NULL) {
> + char *a, *b, *saveptr2 = NULL;
> + int from, to;
> +
> + a = strtok_r(tok, "-", &saveptr2);
> + if (a == NULL) {
> + return false;
> + } else {
> + from = atoi(a);
> + if (from >= max) {
> + return false;
> + }
> + }
> +
> + b = strtok_r(NULL, "-", &saveptr2);
> + if (b == NULL) {
> + to = from;
> + } else {
> + to = atoi(b);
> + if (to >= max) {
> + return false;
> + }
> + }
> +
> + if (from > to) {
> + return false;
> + }
> +
> + for (i=from; i<=to; i++) {
> + list[n] = i;
> + n++;
What is preventing a buffer overrun here? I suspect there is one here because
with this patch applied I hit a segfault running the following command:
pdbg -p0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 getscom 0xf000f
Obviously this is a little contrived but it's important we deal gracefully with
frustrated users who really really want this to run only on p0 :-)
> + }
> +
> + tmp = NULL;
> + };
> +
> + if (n > 0) {
> + qsort(list, n, sizeof(int), int_cmp);
> + j = 0;
> + for (i=1; i<n; i++) {
> + if (list[i] != list[j]) {
> + list[++j] = list[i];
In general I feel this function could do with a few more brief comments to aid
understanding. For example at first glance it may not be immediately obvious to
everyone what the above is doing (removing duplicates from a list).
There also seems to be a few corner cases when I tested with the following fake
device-tree which adds a couple of extra pib nodes:
/dts-v1/;
/ {
#address-cells = <0x1>;
#size-cells = <0x0>;
fsi at 0 {
#address-cells = <0x2>;
#size-cells = <0x1>;
compatible = "ibm,fake-fsi";
reg = <0x0 0x0 0x0>;
index = <0x0>;
status = "mustexist";
pib at 0 {
compatible = "ibm,fake-pib";
reg = <0x0 0x0 0x0>;
index = <0x0>;
};
pib at 1 {
compatible = "ibm,fake-pib";
reg = <0x0 0x1 0x0>;
index = <0x1>;
};
pib at 2 {
compatible = "ibm,fake-pib";
reg = <0x0 0x1 0x0>;
index = <0x2>;
};
};
};
For example this works as expected:
$ ./pdbg -p0 getscom 0xf000f
fake_pib_read(0x000f000f, 0xdeadbeef)
p0:0xf000f = 0x00000000deadbeef
But this doesn't (it appears to return the result for p0 instead of p1):
$ ./pdbg -p1 getscom 0xf000f
fake_pib_read(0x000f000f, 0xdeadbeef)
p0:0xf000f = 0x00000000deadbeef
Even though this works:
$ ./pdbg -p0-1 getscom 0xf000f
fake_pib_read(0x000f000f, 0xdeadbeef)
p0:0xf000f = 0x00000000deadbeef
fake_pib_read(0x000f000f, 0xdeadbeef)
p1:0xf000f = 0x00000000deadbeef
But this doesn't:
$ ./pdbg -p1-2 getscom 0xf000f
fake_pib_read(0x000f000f, 0xdeadbeef)
p0:0xf000f = 0x00000000deadbeef
fake_pib_read(0x000f000f, 0xdeadbeef)
p1:0xf000f = 0x00000000deadbeef
So it would be nice if we could get the above cases fixed (and even better write
some tests! - although that is something that has been on my TODO list for
ages). However the concept is great and really improves a big usability issue so
keen to see it fixed up so we can merge it. When you do repost it please post it
as a seperate patch as I have already merged the rest of the patches in this
series. Thanks!
- Alistair
> + }
> + }
> + n = j+1;
> + }
> +
> + *count = n;
> + return true;
> +}
> +
> static bool parse_options(int argc, char *argv[])
> {
> int c;
> bool opt_error = true;
> - static int current_processor = INT_MAX, current_chip = INT_MAX, current_thread = INT_MAX;
> + int p_list[MAX_PROCESSORS];
> + int c_list[MAX_CHIPS];
> + int t_list[MAX_THREADS];
> + int p_count = 0, c_count = 0, t_count = 0;
> + int cur_p, cur_c, cur_t;
> + int i, j, k;
> struct option long_opts[] = {
> {"all", no_argument, NULL, 'a'},
> {"backend", required_argument, NULL, 'b'},
> @@ -195,50 +274,45 @@ static bool parse_options(int argc, char *argv[])
> switch(c) {
> case 'a':
> opt_error = false;
> - for (current_processor = 0; current_processor < MAX_PROCESSORS; current_processor++) {
> - processorsel[current_processor] = &chipsel[current_processor][0];
> - for (current_chip = 0; current_chip < MAX_CHIPS; current_chip++) {
> - chipsel[current_processor][current_chip] = &threadsel[current_processor][current_chip][0];
> - for (current_thread = 0; current_thread < MAX_THREADS; current_thread++)
> - threadsel[current_processor][current_chip][current_thread] = 1;
> - }
> +
> + if (p_count == 0) {
> + p_count = MAX_PROCESSORS;
> + for (i=0; i<MAX_PROCESSORS; i++)
> + p_list[i] = i;
> + }
> +
> + if (c_count == 0) {
> + c_count = MAX_CHIPS;
> + for (i=0; i<MAX_CHIPS; i++)
> + c_list[i] = i;
> + }
> +
> + if (t_count == 0) {
> + t_count = MAX_THREADS;
> + for (i=0; i<MAX_THREADS; i++)
> + t_list[i] = i;
> }
> break;
>
> case 'p':
> - errno = 0;
> - current_processor = strtoul(optarg, &endptr, 0);
> - opt_error = (errno || *endptr != '\0');
> - if (!opt_error) {
> - if (current_processor >= MAX_PROCESSORS)
> - opt_error = true;
> - else
> - processorsel[current_processor] = &chipsel[current_processor][0];
> - }
> + if (!parse_list(optarg, MAX_PROCESSORS, p_list, &p_count))
> + fprintf(stderr, "Failed to parse '-p %s'\n", optarg);
> + else
> + opt_error = false;
> break;
>
> case 'c':
> - errno = 0;
> - current_chip = strtoul(optarg, &endptr, 0);
> - opt_error = (errno || *endptr != '\0');
> - if (!opt_error) {
> - if (current_chip >= MAX_CHIPS)
> - opt_error = true;
> - else
> - chipsel[current_processor][current_chip] = &threadsel[current_processor][current_chip][0];
> - }
> + if (!parse_list(optarg, MAX_CHIPS, c_list, &c_count))
> + fprintf(stderr, "Failed to parse '-c %s'\n", optarg);
> + else
> + opt_error = false;
> break;
>
> case 't':
> - errno = 0;
> - current_thread = strtoul(optarg, &endptr, 0);
> - opt_error = (errno || *endptr != '\0');
> - if (!opt_error) {
> - if (current_thread >= MAX_THREADS)
> - opt_error = true;
> - else
> - threadsel[current_processor][current_chip][current_thread] = 1;
> - }
> + if (!parse_list(optarg, MAX_THREADS, t_list, &t_count))
> + fprintf(stderr, "Failed to parse '-t %s'\n", optarg);
> + else
> + opt_error = false;
> break;
>
> case 'b':
> @@ -292,10 +366,37 @@ static bool parse_options(int argc, char *argv[])
> }
> } while (c != EOF && !opt_error);
>
> - if (opt_error)
> + if (opt_error) {
> print_usage(argv[0]);
> + return false;
> + }
> +
> + if ((c_count > 0 || t_count > 0) && p_count == 0) {
> + fprintf(stderr, "No processor(s) selected\n");
> + fprintf(stderr, "Use -p or -a to select processor(s)\n");
> + return false;
> + }
> +
> + if (t_count > 0 && c_count == 0) {
> + fprintf(stderr, "No chip(s) selected\n");
> + fprintf(stderr, "Use -c or -a to select chip(s)\n");
> + return false;
> + }
> +
> + for (i=0; i<p_count; i++) {
> + cur_p = p_list[i];
> + processorsel[i] = &chipsel[cur_p][0];
> + for (j=0; j<c_count; j++) {
> + cur_c = c_list[j];
> + chipsel[cur_p][cur_c] = &threadsel[cur_p][cur_c][0];
> + for (k=0; k<t_count; k++) {
> + cur_t = t_list[k];
> + threadsel[cur_p][cur_c][cur_t] = 1;
> + }
> + }
> + }
>
> - return !opt_error;
> + return true;
> }
>
> void target_select(struct pdbg_target *target)
>
More information about the Pdbg
mailing list