Bug#123272: crash segfaults on wrong command line input

[Matej Vela <vela@debian.org>]

tags 123272 - patch
thanks

Aurélien GÉRÔME <ag@roxor.cx> writes:

>>  * nsub is an optional argument; its absence is not an error.
>
> It leads to SIGSEGV, so it seems to me it is indeed.

The documentation is clear:

  crashme.txt:45: [NSUB]  The [NSUB] is optional, the  number  of  vfork  sub-
  crashme.txt:46:         processes  running all at once.  [...]

Removing features is not the way to fix bugs.

>>  * The if branch you are patching doesn't actually use nsub; only the
>>    first three arguments are passed to old_main().
>
> I do not see that when I read the thing with indent as my helper.

Videlicet:

  crashme.c:392: else if ((argc == 6) && ((strlen(argv[4]) == 0) ||
  crashme.c:393:                          (strcmp(argv[4],".") == 0)))
  crashme.c:394:   {verbose_level = atol(argv[5]);
  crashme.c:395:    old_main(4,argv);}

The first condition on line 392 checks that there are five arguments
(plus the program name); this guarantees that argv[4] is non-null, and
the other two conditions will not produce a segfault.  Line 394 stores
the fifth argument (VERBOSE) in verbose_level.  Line 395 calls
old_main() with an unchanged argv[].

As you can see below, old_main() neither accesses argv[4] nor passes
argv[] to other functions:

  crashme.c:434:void old_main(argc,argv)
  crashme.c:435:     int argc;
  crashme.c:436:     char **argv;
  crashme.c:437:{char *ptr;
  crashme.c:438: copyright_note(3);
  crashme.c:439: nbytes = atol(argv[1]);
  crashme.c:440: if (ptr = strchr(argv[1],'.'))
  crashme.c:441:   incptr = atol(&ptr[1]);
  crashme.c:442: if (argv[1][0] == '+') malloc_flag = 1;
  crashme.c:443: nseed = atol(argv[2]);
  crashme.c:444: ntrys = atol(argv[3]);
  crashme.c:445: sprintf(notes,"crashme %s%ld.%d %ld %ld",
  crashme.c:446:         (malloc_flag == 0) ? "" : "+",nbytes,incptr,nseed,ntrys);
  crashme.c:447: note(3);
  crashme.c:448: record_note();
  crashme.c:449: if (malloc_flag == 0)
  crashme.c:450:   {the_data = bad_malloc((nbytes < 0) ? -nbytes : nbytes);
  crashme.c:451:    badboy = castaway(the_data);
  crashme.c:452:    sprintf(notes,"Badboy at %d. 0x%X",badboy,badboy);
  crashme.c:453:    note(3);}
  crashme.c:454: srand(nseed);
  crashme.c:455:#ifdef WIN32
  crashme.c:456: SetErrorMode(SEM_FAILCRITICALERRORS |
  crashme.c:457:              SEM_NOGPFAULTERRORBOX |
  crashme.c:458:              SEM_NOOPENFILEERRORBOX);
  crashme.c:459:#endif
  crashme.c:460: badboy_loop();}

This leads me to conclude that argv[4] is not the cause of the crash.

>>  * From a cursory gdb session, the reason crashme segfaults is that it
>>    executes illegal instructions, not in the options parsing.
>
> So why does it not segfault anymore with my work?

  $ ./patched-crashme +8192.16 478 100
  [...]
  try 38, offset 608
  Segmentation fault

> While you seem perfectly fine saying what I did is stupid,

I did not say it was stupid, I said it was incorrect, and why.  Do you
see this as a personal insult?

> I would be rather interested that you propose a better solution for
> a bug opened more than 4 years ago and which nobody cared to fix.

I couldn't find it at first glance and gave up; the fact that no one
cared to in 4 years is telling me something.

Cheers,

Matej