[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: Bug#352535: ITP: gitmail -- Very simple graphical mail user agent for sending mail (GTK)

ma, 2006-02-13 kello 23:00 +0100, Adeodato Simó kirjoitti:
> * Adeodato Simó [Mon, 13 Feb 2006 22:58:24 +0100]:
> >   And it may fall under the "too buggy that we refuse to support it"
>   Ah, forgot to say that the code is, at least, full of malloc(FIXED_NUM), 
>   that afterwards get used without any check for errors.

As an example:

GList *readfile (char *file, GtkWidget *comboboxentry) {
        char *homedir = (char*)malloc(1024);
        strcpy (homedir, g_get_home_dir());

There are at least two big problems with this code:

1) It does not check that the malloc succeeds, resulting in a
segmentation fault on the next line if malloc fails. It would be better
to bail out with a clear error message to the user; it is arguably OK to
not try to do anything fancier to recover from the problem: if you can't
allocate even one kilobyte, it is questionable what you can do at all.
(GTK+ has ready-made functions for this.)

2) There is no guarantee that 1024 bytes is enough to make a copy of the
path to the home directory. In this case, it is pretty unlikely that the
problem ever occurs, but if it does, then you're screwed.

That example was the first I found. The code is indeed full of them.
>From the same function:

        char *buffer = (char*) malloc (10000);
        int len;
        len = read (fd, buffer, SSIZE_MAX);

Apart from the lack of checking whether malloc succeeds, SSIZE_MAX is
(much) greater than ten thousand, and any file bigger than about ten
kilobytes will cause this to happen. Files bigger than ten kilobytes are
pretty common. This particular function is now only being used to read
something that looks like config files, but for the sake of sanity,
security, and safety, it is not OK to assume they will always be less
than ten kilobytes.

I'd say that the code is not ready to be included in Debian. It needs a
very careful code review first.

While we do have some pretty crappy programs in Debian already, let's
try to avoid adding programs with known big problems, and now we know
about this one.

If the program does get reviewed, and all the problems that are found
get fixed, then I have no objection to the package.

The road is wide and the sky is tall, before I die I will see it

Reply to: