[Top][All Lists]

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

Re: oops? read/write vs type of length parameter

From: Jim Meyering
Subject: Re: oops? read/write vs type of length parameter
Date: Wed, 13 Apr 2011 08:31:09 +0200

Paul Eggert wrote:
> On 04/12/2011 08:53 AM, Paul Eggert wrote:
>> I can do it, in the next day or two.
> OK, I did it, by auditing all uses of emacs_read and emacs_write to
> find and fix all negative buffer sizes.  However, instead of negative
> sizes I found overflows and some other issues, described below; and I
> therefore now agree with Eli that emacs_write shouldn't use the same
> signature as POSIX 'write'.  However, I also agree with Jim that
> emacs_write's API need not artificially limit buffer sizes to SSIZE_MAX.
> Here's the key idea of the attached patch.  Since
> "N = emacs_write (FD, BUF, SIZE)" returns N if successful, and some
> value less than N if it fails, the caller can easily determine whether
> it succeeded by testing N == SIZE.  So on failure there's no need for
> emacs_write to return a special value like -1.
> With this in mind, emacs_write can simply return the number of bytes
> written, which will always be a size_t value.  Jim, this idea is
> already used by gnulib's full_write function, so there's good
> precedent for departing from the POSIX signature here.  And Eli, this
> avoids the problem where the size is so large that a signed return
> value would overflow and become negative.
> Here are some other bugs in Emacs that I found as part of this audit:
>  * Several places in sound.c attempt to stuff a size into an 'int',
>    which doesn't work.  This problem is independent of the above, and
>    its fix is independent of the above too, so it's split into a
>    separate patch in the attached bundle.
>  * send_process attempts to stuff a size into an 'int' as well.  This
>    can be fixed by changing one of its local variables from int to size_t.
>  * emacs_gnutls_write has the same issues as emacs_write, and should
>    be fixed the same way.
> Attached is a bzr bundle of two proposed patches to fix the problem as
> described above.  Comments are welcome.

That looks like a fine change.
Thanks for doing the work.
The improved ease of use makes this change worthwhile to me.
No more hassling with a mix of signed and unsigned lengths,
and thus less risk of misuse and fewer compiler warnings.

reply via email to

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