Bug#597914: perdition: type mismatch in call to vanessa_socket_pipe_func

[Sergio Gelato]

* Simon Horman [2010-09-26 22:37:57 +0900]:
 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?

Yes.

 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?

Quite. Thanks.



-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#597914: perdition: type mismatch in call to vanessa_socket_pipe_func

[Simon Horman]

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.

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.

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.

Index: perdition/perdition/perdition.c
===
--- perdition.orig/perdition/perdition.c2010-09-26 15:42:17.0 
+0900
+++ perdition/perdition/perdition.c 2010-09-26 15:42:30.0 +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.0 +0900
+++ perdition/perdition/io.c2010-09-26 15:55:41.0 +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.0 +0900
+++ perdition/perdition/db/odbc/perditiondb_odbc.c  2010-09-26 
15:42:30.0 +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

Bug#597914: perdition: type mismatch in call to vanessa_socket_pipe_func

[Sergio Gelato]

* 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.

 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.

 Index: perdition/perdition/perdition.c
 ===
 --- perdition.orig/perdition/perdition.c  2010-09-26 15:42:17.0 
 +0900
 +++ perdition/perdition/perdition.c   2010-09-26 15:42:30.0 +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.0 +0900
 +++ perdition/perdition/io.c  2010-09-26 15:55:41.0 +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: 

Bug#597914: perdition: type mismatch in call to vanessa_socket_pipe_func

[Simon Horman]

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.c2010-09-26 15:42:17.0 
  +0900
  +++ perdition/perdition/perdition.c 2010-09-26 15:42:30.0 +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.0 +0900
  +++ perdition/perdition/io.c2010-09-26 15:55:41.0 +0900
  @@ -655,7 +655,7 @@ err:
* 0 otherwise (one of io_a or io_b 

Bug#597914: perdition: type mismatch in call to vanessa_socket_pipe_func

[Simon Horman]

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.0 
   +0900
   +++ perdition/perdition/perdition.c   2010-09-26 15:42:30.0 
   +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: 

Bug#597914: perdition: type mismatch in call to vanessa_socket_pipe_func

[Simon Horman]

On Fri, Sep 24, 2010 at 10:36:09AM +0200, Sergio Gelato wrote:
 Package: perdition
 Version: 1.19~rc3-1
 
 (A look at the Mercurial repository shows that the problem is still
 present in the latest upstream version.)
 
 I noticed the following in my logs today (irrelevant information censored):
 Sep 23 22:34:27 hostname perdition[31439]: Close: IP1-IP2 
 user=username received=150480 sent=-1801249610
 
 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?




-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#597914: perdition: type mismatch in call to vanessa_socket_pipe_func

[Sergio Gelato]

* 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.



-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#597914: perdition: type mismatch in call to vanessa_socket_pipe_func

[Sergio Gelato]

Package: perdition
Version: 1.19~rc3-1

(A look at the Mercurial repository shows that the problem is still
present in the latest upstream version.)

I noticed the following in my logs today (irrelevant information censored):
Sep 23 22:34:27 hostname perdition[31439]: Close: IP1-IP2 
user=username received=150480 sent=-1801249610

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.



-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org