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, 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