On Wed, Feb 01, 2006 at 09:49:26AM +0100, Nils Larsch wrote:
> >This is chnaging the beahavior of static long conn_ctrl(BIO *b, int cmd,
> >long num, void
> >*ptr) function.
> >I think it is a typo (just like some other epople using openssl).
> >Nils, could you please confirm me that, or explain me the reason why
> >changing this?
>
> well "if (!data->state != BIO_CONN_S_OK)" obviously doesn't make
> much sense as BIO_CONN_S_OK (== 6) is always unequal to 0 or 1,
> but looking at the code again I think it really should be
> "if (data->state != BIO_CONN_S_OK)" as conn_state() does nothing
> if the BIO is already in the BIO_CONN_S_OK state.
"fixing" this typo like that still breaks the state machine at connect
time.
let's say we add some debug around the if statement:
----snip----snip----snip----snip----snip----snip----snip----snip----
case BIO_C_DO_STATE_MACHINE:
/* use this one to start the connection */
fprintf(stderr, "data->state = %d\n", data->state);
if (!(data->state != BIO_CONN_S_OK)) {
fprintf(stderr, "called conn_state()\n");
ret=(long)conn_state(b,data);
} else {
fprintf(stderr, "whoops! wrong turn!\n");
ret=1;
}
break;
----snip----snip----snip----snip----snip----snip----snip----snip----
and use this trivial code for testing:
----snip----snip----snip----snip----snip----snip----snip----snip----
#include <openssl/bio.h>
main()
{
BIO *b = BIO_new(BIO_s_connect());
int fd, res;
BIO_set_conn_hostname(b, "127.0.0.1:4242");
res = BIO_do_connect(b);
fprintf(stderr, "do_connect: %d\n", res);
fd = BIO_get_fd(b, &fd);
fprintf(stderr, "fd = %d\n", fd);
BIO_puts(b, "test\n");
fd = BIO_get_fd(b, &fd);
fprintf(stderr, "fd = %d\n", fd);
return 0;
}
----snip----snip----snip----snip----snip----snip----snip----snip----
prior to the incriminated commit, we got this at execution
(assuming some server is listening on 127.0.0.1:4242):
----snip----snip----snip----snip----snip----snip----snip----snip----
$ ./test
data->state = 1
called conn_state()
do_connect: 1
fd = 3
fd = 3
----snip----snip----snip----snip----snip----snip----snip----snip----
everything looks okay.
notice the value of data->state before the BIO_do_connect() call,
which is NOT BIO_CONN_S_OK !
just in case, let's test without anyone listening on 127.0.0.1:4242.
----snip----snip----snip----snip----snip----snip----snip----snip----
$ ./test
data->state = 1
called conn_state()
do_connect: -1
fd = 3
fd = 3
----snip----snip----snip----snip----snip----snip----snip----snip----
BIO_do_connect() returned -1, as expected from a refused connection
to a closed port...
now let's look at what happens with the parentheses from the commit:
----snip----snip----snip----snip----snip----snip----snip----snip----
$ ./test
data->state = 1
whoops! wrong turn!
do_connect: 1
fd = -1
fd = 3
----snip----snip----snip----snip----snip----snip----snip----snip----
uh-oh! conn_state() is obviously not called (noticed the "use this
one to *start* the connection" just above the if in the code ?).
just to be sure, let's try again with a closed port on the other side:
----snip----snip----snip----snip----snip----snip----snip----snip----
$ ./test
data->state = 1
whoops! wrong turn!
do_connect: 1
fd = -1
fd = 3
----snip----snip----snip----snip----snip----snip----snip----snip----
same result, and no error reported. not exactly what I call consistent
behavior...
what's interesting is the fd value. before the BIO_puts() it's -1, which
is normal since *nothing* was done yet due to the "wrong turn".
after the BIO_puts() call, the connection is established, because of
this code in conn_write() (the same is also true if we do a read
instead):
----snip----snip----snip----snip----snip----snip----snip----snip----
if (data->state != BIO_CONN_S_OK)
{
ret=conn_state(b,data);
if (ret <= 0)
return(ret);
}
----snip----snip----snip----snip----snip----snip----snip----snip----
this effectively establishes the connection, but a bit too late.
I don't know if you're like that, but when I do a connect(2) call on
unix sockets, I like to have my socket right now, not just after the
first read() or write() :P
IMHO, the negation in your commit should be removed, and the if statement
(which looks like it's been some "miraculously-working" typo since a
few years from now) should just be like the ones in conn_read() and
conn_write().
Cheers,
--
Gabriel Forté <[EMAIL PROTECTED]>
______________________________________________________________________
OpenSSL Project http://www.openssl.org
Development Mailing List [email protected]
Automated List Manager [EMAIL PROTECTED]
