musl - Re: [PATCH] bugfix: initialize a state variable in lio

Follow @Openwall on Twitter for new release announcements and other news

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1371367371.16425.363.camel@eris.loria.fr>
Date: Sun, 16 Jun 2013 09:22:51 +0200
From: Jens Gustedt <jens.gustedt@...ia.fr>
To: musl@...ts.openwall.com
Subject: Re: [PATCH] bugfix: initialize a state variable in lio_wait

Hello,

Am Sonntag, den 16.06.2013, 01:40 -0400 schrieb Rich Felker:
> On Sun, Jun 16, 2013 at 12:16:27AM +0200, Szabolcs Nagy wrote:
> > * Jens Gustedt <Jens.Gustedt@...ia.fr> [2013-06-16 00:01:20 +0200]:
> > > got_err was only set to 1 in case of error, but never written
> > > otherwise.
> > 
> > i wonder why gcc -Wmaybe-uninitialized does not catch this
> 
> Indeed, this is very strange. Anyway, I'm committing the fix.

Looking a bit deeper into it, I think this is not only strange but
suspicious.

First, this means that we have not enough coverage for the aio
routines to find simple bugs like that one. aio isn't much used,
obviously.

But then I also checked with clang, and it doesn't find this
either. First, I thought that the control flow surrounding this must
be fundamentally flawed if the compilers aren't capable to track such
simple cases of uninitialized variables. But even by simplifying, it
doesn't trigger, so I am not sure what is going on.

It makes not much sense to me to have an infinite loop at the
outside. I think the control flow should merely be something as
below. Still I am not sure that the strategy of continuing after an
error has been detected is paying something. I would be more in favor
in erroring out as early as possible and return err in errno to let
the application know that there was a problem. Waiting until we
checked everybody and then returning EIO is throwing away information.

Jens

static int lio_wait(struct lio_state *st)
{
  int got_err = 0;
  struct aiocb **const cbs = st->cbs;

  for (int i=0, cnt = st->cnt; i<cnt; i++) {
    if (cbs[i]) {
    RETRY:;
      switch(aio_error(cbs[i])) {
      case EINPROGRESS:
        if (aio_suspend((void *)cbs, cnt, 0))
          return -1;
        else
          goto RETRY;
      case 0:
        break;
      default:
        got_err=1;
        cbs[i] = 0;
        break;
      }
    }
  }
  if (got_err) {
    errno = EIO;
    return -1;
  } else
    return 0;
}

-- 
:: INRIA Nancy Grand Est :: http://www.loria.fr/~gustedt/   ::
:: AlGorille ::::::::::::::: office Nancy : +33 383593090   ::
:: ICube :::::::::::::: office Strasbourg : +33 368854536   ::
:: ::::::::::::::::::::::::::: gsm France : +33 651400183   ::
:: :::::::::::::::::::: gsm international : +49 15737185122 ::

Download attachment "signature.asc" of type "application/pgp-signature" (199 bytes)

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.