On Sun, Sep 26, 2010 at 10:37:57PM +0900, Simon Horman wrote:
> On Sun, Sep 26, 2010 at 01:20:14PM +0200, Sergio Gelato wrote:
> > * Simon Horman [2010-09-26 16:58:05 +0900]:
> > > On Sat, Sep 25, 2010 at 11:10:53PM +0200, Sergio Gelato wrote:
> > > > * Simon Horman [2010-09-25 21:34:02 +0900]:
> > > > > On Fri, Sep 24, 2010 at 10:36:09AM +0200, Sergio Gelato wrote:
> > > > > > The main problem is that perdition/io.c:io_pipe() and its caller
> > > > > > perdition/perdition.c:perdition_log_close() use int counters
> > > > > > while the corresponding arguments in vanessa_socket_pipe_func()
> > > > > > are declared size_t. I'd worry about stack corruption on platforms
> > > > > > where sizeof(size_t) > sizeof(int).
> > > > > >
> > > > > > Suggested fix: declare those counters size_t, and (for cosmetic
> > > > > > purposes)
> > > > > > cast them to unsigned long before formatting them with %lu instead
> > > > > > of %d.
> > > > >
> > > > > Thanks, I'll fix that.
> > > > >
> > > > > Do you think it warrants an update to the testing (= already frozen
> > > > > squeeze)
> > > > > package?
> > > >
> > > > I do: amd64 has sizeof(size_t)==8 and sizeof(int)==4.
> > >
> > > Hi,
> > >
> > > could you take a look at the following patch and see what you think?
> > > I have isolated three integer/size_t with problems.
> > >
> > > 1) perdition_log_close() takes integer arguments but the counters are
> > > actually size_t.
> > >
> > > I think that the resulting bug is cosmetic.
> >
> > I stand corrected. On looking more carefully at the various versions of
> > perdition I'm interested in, I now see that changeset 695 actually did
> > fix the problem I was most concerned with. (You didn't update the comments,
> > but that's also cosmetic.) Then I see no need to push this fix into
> > squeeze. (lenny, on the other hand, might benefit from that changeset 695.)
> >
> > > 2) __io_pipe_read() and __io_pipe_write() return an int but
> > > the counter in question is a ssize_t.
> > >
> > > I actually don't think this can manifest in a problem as the maximum
> > > return value is limited by the count argument, which is 1024
> > > (=BUFFER_SIZE). If count was greater than 2^31 then a problem could
> > > occur whereby a large read was mis-read as an error and the connection
> > > would be prematurely closed.
> >
> > I wouldn't be so sure. On platforms where int is shorter than ssize_t,
> > vanessa_socket_pipe_read_write_func() will use data that may not have been
> > initialised properly. Even if it has been zeroed, depending on endianness
> > the effect could be equivalent to a shift, or the result may not be
> > sign-extended correctly. amd64 seems to fall into this last category:
> > storing (-1) into %eax zeroes the upper half of %rax, which should
> > defeat the error handling in your code.
> >
> > *This* fix would therefore be welcome for squeeze according to my criteria.
>
> I'm not entirely convinced,
To clarify, what I mean is. My testing didn't show up a problem.
But I am prepared to accept that there is a problem. So, actually,
I am convinced.
> but I do agree that the current situation
> is precarious and fixing it in squeeze seems worthwhile.
>
> > > 3) perditiondb_odbc.c:dbserver_get() passes the address of
> > > an SQLINTEGER instead of the address of a SQLLEN to the
> > > odbc library call SQLBindCol() twice.
> > >
> > > This seems like it could cause a stack corruption on
> > > systems such as amd64 where SQLINTEGER (= typdef int) is 4 bytes wide
> > > but SQLLEN (= typdef long) is 8 bytes wide.
> >
> > I agree with you here (except that I, and my compiler, count three instances
> > of the problem, not two; but that doesn't affect the patch). Also a
> > candidate
> > for squeeze, I would think.
>
> Ok, perhaps I counted wrong.
>
> In any case, can I confirm that we agree that the io.c and perditiondb_odbc.c
> portions of the change below should go into squeeze?
>
> And for Lenny, I'll look into adding 695 + the io.c and perditiondb_odbc.c
> portions of the change below. Does that sounds good to you?
>
> > > Index: perdition/perdition/perdition.c
> > > ===================================================================
> > > --- perdition.orig/perdition/perdition.c 2010-09-26 15:42:17.000000000
> > > +0900
> > > +++ perdition/perdition/perdition.c 2010-09-26 15:42:30.000000000
> > > +0900
> > > @@ -309,14 +309,14 @@ static void perdition_log_close_early(co
> > > }
> > >
> > > static void perdition_log_close(const char *from_to_host_str,
> > > - struct auth *auth, int received, int sent)
> > > + struct auth *auth, size_t received, size_t sent)
> > > {
> > > struct quoted_str authorisation_id = quote_str(auth->authorisation_id);
> > >
> > > VANESSA_LOGGER_ERR_UNSAFE("Closing session:%s "
> > > "authorisation_id=%s%s%s "
> > > "authentication_id=\"%s\" "
> > > - "received=%d sent=%d",
> > > + "received=%zu sent=%zu",
> > > from_to_host_str,
> > > authorisation_id.quote,
> > > authorisation_id.str,
> > > Index: perdition/perdition/io.c
> > > ===================================================================
> > > --- perdition.orig/perdition/io.c 2010-09-26 15:42:17.000000000 +0900
> > > +++ perdition/perdition/io.c 2010-09-26 15:55:41.000000000 +0900
> > > @@ -655,7 +655,7 @@ err:
> > > * 0 otherwise (one of io_a or io_b closes gracefully)
> > > **********************************************************************/
> > >
> > > -static int __io_pipe_read(int fd, void *buf, size_t count, void *data){
> > > +static ssize_t __io_pipe_read(int fd, void *buf, size_t count, void
> > > *data){
> > > io_t *io;
> > > io_select_t *s;
> > > ssize_t bytes;
> > > @@ -693,7 +693,9 @@ err:
> > > }
> > >
> > >
> > > -static int __io_pipe_write(int fd, const void *buf, size_t count, void
> > > *data){
> > > +static ssize_t
> > > +__io_pipe_write(int fd, const void *buf, size_t count, void *data)
> > > +{
> > > io_t *io;
> > > io_select_t *s;
> > >
> > > Index: perdition/perdition/db/odbc/perditiondb_odbc.c
> > > ===================================================================
> > > --- perdition.orig/perdition/db/odbc/perditiondb_odbc.c 2010-09-26
> > > 15:42:17.000000000 +0900
> > > +++ perdition/perdition/db/odbc/perditiondb_odbc.c 2010-09-26
> > > 15:42:30.000000000 +0900
> > > @@ -214,7 +214,7 @@ int dbserver_get(const char *key_str, co
> > > char **str_return, size_t * len_return)
> > > {
> > > SQLINTEGER rc;
> > > - SQLINTEGER rc2;
> > > + SQLLEN rc2;
> > > int status = -1;
> > > SQLHENV env;
> > > SQLHDBC hdbc;
> >
--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
