Re: Use external editor in 'commands' command

[Alfredo Ortega <ortegaalfredo@gmail.com>]

2009/1/16 Eli Zaretskii <eliz@gnu.org>:
>> Date: Fri, 16 Jan 2009 05:38:41 -0200
>> From: Alfredo Ortega <ortegaalfredo@gmail.com>
>>
>> Thanks for the corrections. Here are are both changelogs and the
>> updated diff, I hope there are less errors now...
>> I promise that my next patches will be better!
>
> No one is born with this, so there's nothing wrong in making such
> minor mistakes.
>
> I have several comments to your code:
>
>> +  char vitmp[50];
>
> vitmp[] is a file name, so 50 is not nearly enough characters to hold
> the longest possible name.  At the very least, please use
> FILENAME_MAX, or (better) some dynamic code that grows it as needed,
> because some hosts (e.g., Hurd) don't have any limitations on file
> name length.
>
>> +  char cmdline[100];
>
> Same here: 100 is not enough, because $EDITOR holds a file name.
>
>> +  if (!strcmp(COMMANDS_EDCOMMAND,p)) {
>
> This is not GNU style of laying out brace-delimited blocks; please use
> the same style and indentation as elsewhere in GDB sources.
>
>> +     strcpy(vitmp,"/tmp/.gdbXXXXXX");
>
> Please leave a blank between the function name and the left
> parenthesis, and also between the comma and the following argument in
> argument lists.  Like this:
>
>         strcpy (vitmp, "/tmp/.gdbXXXXXX");
>
> Btw, using "/tmp/.gdbXXXXXX" is non-portable to Windows, where there's
> no guarantee there will be a "/tmp" on the current drive, and it will
> not work at all in the DJGPP (a.k.a. DOS) port of GDB, because DOS
> filesystems do not allow file names with a leading dot.
>
>> +     /* Generates the temporal file name*/
>                         ^^^^^^^^
> "temporary"
>
>> +     /* vitmp = tempnam(NULL,".gdb"); this is more secure according to man mkstemp, but gcc complains... */
>
> What complaint do you see?
>
>> +     if (mkstemp(vitmp)<0) return;
>
> Are you using mkstemp from libiberty?  Because otherwise it may not be
> available on the host.
>
>> +                                     if (fsize<strlen(l->line)+1) {
>> +                                             fclose(tmpstream);
>> +                                             unlink(vitmp);
>> +                                             return;
>> +                                             };
>
> Don't we want some error message in this case?
>
>> +                     if ((editor = (char *) getenv ("EDITOR")) == NULL)
>> +                             editor = "/bin/ex";
>
> This is likewise non-portable: "/bin/ex" is only guaranteed to exist
> on Posix platforms.
>

In the next patch I will adress all those issues (The fixed lenght
buffers are ugly but you can never overflow them, the mkstemp template
and the snprintf don't allow for it, but then some bad truncation and
unpredictable execution may happen).

gcc will complain about tmpnamp() like this:

test.c:(.text+0x13): warning: the use of `tempnam' is dangerous,
better use `mkstemp'

The man page of FreeBSD explains it much better than the Debian one,
now I know it's because a possible race condition.
About the "/bin/ex" issue, to maintain consistency I did it the same
way that the EDIT command, here:

gdb/cli/cli-cmds.c:695

  if ((editor = (char *) getenv ("EDITOR")) == NULL)
      editor = "/bin/ex";

BTW "/bin/ex" exists on solaris 9, but not Debian or *BSD (is
/usr/bin/ex there), as a result text editing won't work by default on
most systems.
It would be difficult to choose a portable editor...maybe with some #defines