conntrack 1:0.9.7-1.1ubuntu1 (Accepted)
 

Hi,

On Mon, Nov 24, 2008 at 12:15:13AM -0000, Manny Vindiola wrote:
> conntrack (1:0.9.7-1.1ubuntu1) jaunty; urgency=low
>
> * Merge from debian unstable (LP: 256380), remaining changes:
> + #include <limits.h> in {main,ignore_pool}.c to get PATH_MAX and INT_MAX
> + local.c: Fix insecure printf usage
> + debian/rules:
> -undef _FORTIFY_SOURCE so that it doesn't fail about ignored chdir()
> return value.

Undefining FORTIFY should only be done in extreme cases when it is not
possible to correct the situations it helps detects. As documented[1], it
is much better to check the ignored return values, and handle them in some
graceful way, instead of disabling FORTIFY for the entire build, as this
will leave the program without the run-time protections FORTIFY provides.
In situations where it is not obvious how an error can be handled
correctly, tricking the compiler into throwing away the return value is
still preferred to disabling FORTIFY globally for the build. (And in all
situations, the changes should be forwarded to the Debian bug tracker.)

-Kees

[1] https://wiki.ubuntu.com/CompilerFlags

--
Kees Cook
Ubuntu Security Team

--
ubuntu-devel mailing list
ubuntu-devel@lists.ubuntu.com
Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/ubuntu-devel

conntrack 1:0.9.7-1.1ubuntu1 (Accepted)
 

On Sun, 2008-11-23 at 16:31 -0800, Kees Cook wrote:
> Hi,
>
> On Mon, Nov 24, 2008 at 12:15:13AM -0000, Manny Vindiola wrote:
> > conntrack (1:0.9.7-1.1ubuntu1) jaunty; urgency=low
> >
> > * Merge from debian unstable (LP: 256380), remaining changes:
> > + #include <limits.h> in {main,ignore_pool}.c to get PATH_MAX and INT_MAX
> > + local.c: Fix insecure printf usage
> > + debian/rules:
> > -undef _FORTIFY_SOURCE so that it doesn't fail about ignored chdir()
> > return value.
>
> Undefining FORTIFY should only be done in extreme cases when it is not
> possible to correct the situations it helps detects. As documented[1], it
> is much better to check the ignored return values, and handle them in some
> graceful way, instead of disabling FORTIFY for the entire build, as this
> will leave the program without the run-time protections FORTIFY provides.
> In situations where it is not obvious how an error can be handled
> correctly, tricking the compiler into throwing away the return value is
> still preferred to disabling FORTIFY globally for the build. (And in all
> situations, the changes should be forwarded to the Debian bug tracker.)

Hi Kees,

Thanks for bringing this up.

For the record Manny didn't introduce this change, just merged it. I
appreciate you raising the issue, and I know you weren't attacking
Manny.

I have attacked a couple of these failures recently, and the thing I
always find hard is knowing what to do with an error condition. Without
knowing the code it is hard to know how an error should be handled.
For instance today I was looking at an ircd that wasn't checking the
return code on a write call, writing to its log file. I don't think
an error should abort, but in many cases that will be the only sensible
thing to do.

Are there any guidelines for how to handle these failures so that we can
get better at fixing them?

Thanks,

James



--
ubuntu-devel mailing list
ubuntu-devel@lists.ubuntu.com
Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/ubuntu-devel

conntrack 1:0.9.7-1.1ubuntu1 (Accepted)
 

On Mon, Nov 24, 2008 at 01:02:49AM +0000, James Westby wrote:
> For the record Manny didn't introduce this change, just merged it. I
> appreciate you raising the issue, and I know you weren't attacking
> Manny.

Yup, I saw a bunch of people had worked on this package, so I thought it'd
be good to bring it up. :) And thank you Manny for helping out! I didn't
mean to single you out -- this is really a reminder for everyone. :)

> I have attacked a couple of these failures recently, and the thing I
> always find hard is knowing what to do with an error condition. Without
> knowing the code it is hard to know how an error should be handled.
> For instance today I was looking at an ircd that wasn't checking the
> return code on a write call, writing to its log file. I don't think
> an error should abort, but in many cases that will be the only sensible
> thing to do.
>
> Are there any guidelines for how to handle these failures so that we can
> get better at fixing them?

Well, as you say, it's always different. The way I've tended to triage
them is:

- If there is are obvious error handlers in near-by code, emulate them.
That's what I did in my suggested patch to conntrack[1]. This is,
obviously, the preferred method of dealing with it.

- If there isn't an obvious way to handle the error, then stubbing out an
empty handler is the next way to go -- fundamentally, this doesn't
improve (or weaken) the quality of the code -- ignoring the return code
is what's already happening, so this doesn't change anything. However,
what it _does_ do, is allows the FORTIFY option to still be enabled,
which means the program will gain the run-time FORTIFY protections.

- If there isn't a way to work around the error, and the problem is
isolated to a small area of the source code, I've disabled FORTIFY for
_only_ those source files[2], which can be a pain, depending on the
package's build methods.

- If nothing works, and other people have looked at it, and no one has any
ideas about how to work around a problem with FORTIFY, then it's time to
disable it for the entire build using the -U_FORTIFY_SOURCE CFLAG.

Now, in all of these situations, upstream needs to be notified. Especially
for the #2, where they need to make a larger design decision about how to
deal with the unhandled error condition.

Also, in situations where you've had to disable FORTIFY (#3, #4), please
document the issue in the CompilerFlags[3] wiki page (preferably also with
a bug).

And thank you to everyone who has had to deal with the fall-out of these
options! I know they can really be an annoyance, but taken as a whole,
Ubuntu is far better off with these flags enabled, and everyone's code
quality goes up. :)

-Kees

[1] http://launchpadlibrarian.net/19891829/fortify-warnings.patch.dpatch
[2] http://launchpadlibrarian.net/17169346/sysklogd_1.5-2ubuntu5_1.5-2ubuntu6.diff.gz
[3] https://wiki.ubuntu.com/CompilerFlags

--
Kees Cook
Ubuntu Security Team

--
ubuntu-devel mailing list
ubuntu-devel@lists.ubuntu.com
Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/ubuntu-devel

conntrack 1:0.9.7-1.1ubuntu1 (Accepted)
 

On Mon, Nov 24, 2008 at 01:02:49AM +0000, James Westby wrote:
> I have attacked a couple of these failures recently, and the thing I
> always find hard is knowing what to do with an error condition. Without
> knowing the code it is hard to know how an error should be handled.
> For instance today I was looking at an ircd that wasn't checking the
> return code on a write call, writing to its log file. I don't think
> an error should abort, but in many cases that will be the only sensible
> thing to do.

This is the obvious conclusion, and the one I mistakenly reached at first,
too. But the main reason it's a programming error to not check the return
code on a write call is *not* because you need to always handle errors, but
because you should handle *successes* in the form of partial writes:

If a write() is interrupted by a signal handler before any bytes are
written, then the call fails with the error EINTR; if it is interrupted
after at least one byte has been written, the call succeeds, and
returns the number of bytes written.

write(2)

So using write() means you should always have retry handling in place.

--
Steve Langasek Give me a lever long enough and a Free OS
Debian Developer to set it on, and I can move the world.
Ubuntu Developer http://www.debian.org/
slangasek@ubuntu.com vorlon@debian.org

--
ubuntu-devel mailing list
ubuntu-devel@lists.ubuntu.com
Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/ubuntu-devel

conntrack 1:0.9.7-1.1ubuntu1 (Accepted)
 

On Mon, Nov 24, 2008 at 03:56:39AM -0800, Steve Langasek wrote:
> On Mon, Nov 24, 2008 at 01:02:49AM +0000, James Westby wrote:
> > I have attacked a couple of these failures recently, and the thing I
> > always find hard is knowing what to do with an error condition. Without
> > knowing the code it is hard to know how an error should be handled.
> > For instance today I was looking at an ircd that wasn't checking the
> > return code on a write call, writing to its log file. I don't think
> > an error should abort, but in many cases that will be the only sensible
> > thing to do.
>
> This is the obvious conclusion, and the one I mistakenly reached at first,
> too. But the main reason it's a programming error to not check the return
> code on a write call is *not* because you need to always handle errors, but
> because you should handle *successes* in the form of partial writes:
>
> If a write() is interrupted by a signal handler before any bytes are
> written, then the call fails with the error EINTR; if it is interrupted
> after at least one byte has been written, the call succeeds, and
> returns the number of bytes written.
>
> write(2)
>
> So using write() means you should always have retry handling in place.

Indeed. gnulib provides a full_write function that you can crib (in
GPLv3-compatible code, or otherwise study and reimplement, I suppose) if
you want to ensure that everything is written; or safe_write if you just
want to retry on EINTR but don't mind if not everything gets written,
which for example may be appropriate in code operating on non-blocking
file descriptors.

--
Colin Watson [cjwatson@ubuntu.com]

--
ubuntu-devel mailing list
ubuntu-devel@lists.ubuntu.com
Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/ubuntu-devel