bug-gnulib
[Top][All Lists]
Advanced

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

Re: [Bug-gnulib] getline & getline_safe


From: Bruno Haible
Subject: Re: [Bug-gnulib] getline & getline_safe
Date: Thu, 24 Jul 2003 22:46:13 +0200
User-agent: KMail/1.5

Derek Robert Price wrote:
> And I still disagree for the reasons explained in:
> <http://mail.gnu.org/archive/html/bug-gnulib/2003-07/msg00064.html>.

Sorry, one more mail that went to the archives but never into my mailbox :-(

> I still don't agree. In the single case where the number of characters
> read (the return value of the function) is the same as the character
> limit passed to it, the caller can test the last character (ignoring
> the NUL byte) of it's buffer to see if it contains a delimiter or not.
> Thus, with a character limit specified, the edge case is detectable and
> can be dealt with in something like a typical while(read) loop, where
> each time the buffer is filled, it is dealt with and the next block of
> bytes are read until the caller is satisfied.

And what can the program do inside this while(read) loop, assuming that
it absolutely wants to avoid unlimited memory growth? It cannot accumulate
the result, since that would take too much space. It cannot easily apply a
regexp, since the regex function which works on a concatenation of two
strings is not portable (a GNU extension). No sane programmer will
really want to handle all of the line's pieces *and* limit the memory.

Indeed, in CVS itself you don't need the remainder of the line, you
just bail out like this:
http://ccvs.cvshome.org/source/browse/ccvs/src/server.c?rev=1.307&content-type=text/x-cvsweb-markup&sortby=log

    if( getnline( &tmp, &tmp_allocated, PATH_MAX, stdin ) < 0 )
        {
            error (1, 0, "bad auth protocol start: EOF");
        }

And then later in the same file:

    getnline( &repository, &repository_allocated, PATH_MAX, stdin );
    getnline( &username, &username_allocated, PATH_MAX, stdin );
    getnline( &password, &password_allocated, PATH_MAX, stdin );

So you see, the average programmer does not want to get into complications,
reading the line piece by piece, doing a while(read) loop etc. He wants
to emit just 1 getnline() call per line. And for this case it is better
to truncate the username after PATH_MAX bytes than to pretend that the
password were the remainder of the username's line.

> As for "line-oriented protocols", I would assume the developer knew what
> they were doing when they made a call to "getnline" as opposed to getline.

When I see the code cited above, from src/server.c, I'm not so sure about
this.

And when I read Karl Fogel's original mail
http://www.mail-archive.com/address@hidden/msg00327.html
where he talks about "impose appropriate restrictions on line length"
I'm sure he did not mean to fake newline characters where there are none.

> Also please consider simplicity!

Correctness goes over simplicity.

> You've also neglected to comment on the reordering of the arguments to
> getndelim2.  The new order is much more intuitive

I can hardly argue about matters of taste. For questions of taste, polls
are better than just my or just your opinion.

> Finally, the original bug reported by my user was a typing conflict
> which occurred on 64-bit systems where ssize_t was not the same thing as
> an int.  Now, I'm not sure how they managed to get past your $if
> __GLIBC__ < 2 on that system to hit the prototypes in getline.h, but
> they did.

It's very simple: The original report
http://ccvs.cvshome.org/issues/show_bug.cgi?id=130
dates from 2003-07-13, and from
http://ccvs.cvshome.org/source/browse/ccvs/lib/getline.h
you can see that your lib/getline.h did not have a #if protection at
all at that time.

> I switched the header to switch on HAVE_WORKING_GETLINE &
> HAVE_GETDELIM as detected by configure

This is not necessary. since from the gettext releases I know that
the "#if __GLIBC__ < 2" condition is the right one for all existing
systems, including glibc-2.0.x and Linux libc5. And btw, setting
HAVE_GETDELIM without testing for it explicitly is against the
spirit of autoconf. It may break other modules.

Bruno





reply via email to

[Prev in Thread] Current Thread [Next in Thread]