Discussion:
[freetds] Way to get connection params on failure?
Marc Abramowitz
2014-01-03 02:31:41 UTC
Permalink
Recently a well-known Python database module developer was using pymssql
and it wasn't working for him because he was connecting to a SQL Server on
a non-standard port and he had put this port in an entry in his
freetds.conf and was able to connect with tsql, but he was getting errors
when trying to connect with pymssql, which turned out to be because pymssql
calls DB-LIB functions that don't end up consulting freetds.conf to get the
port and so the connect was failing for him, whereas it passed with tsql,
so he thought that he had discovered a bug in pymssql.

The solution was simple. By adding an explicit `port` argument to his
Python code, stuff worked. But we only discovered this after I advised him
about TDSDUMPCONFIG and such and ultimately figured it out.

The takeaway for me and he said this as well, is that it would be nice if
there were better error messages on connection failure. Either from pymssql
or FreeTDS. This would help people diagnose their connection issues much
quicker.

Currently a failed connection results in an error like this:

Error 20009 (severity 9):
Unable to connect: Adaptive Server is unavailable or does not exist
OS error 61, "Connection refused"

Note that it doesn't say what IP address, port, etc. it was trying to
connect to.

Is there a way to either augment the default FreeTDS error messages --
e.g.: "Connection refused to 127.0.0.1:1433" or DB-API calls that, given
the dbproc or such, will give the IP address, port, etc. so that I can
print them out in my error handler?

Marc
Marc Abramowitz
2014-01-04 22:59:00 UTC
Permalink
I think code may better explain what I'm looking for. Something like this:

https://gitorious.org/freetds/freetds/merge_requests/24

The particular implementation here is probably not ideal, as I stored the
server name/port in a global variable, which seems pretty icky. I
considered adding a field to the dbproc struct -- this seems like a better
place for it but then I shied away from doing that because I wondered if
you guys try to limit adding stuff to structs in case it affects binary
compatibility? (though perhaps it's OK if the client always deals with
pointers to the structs and treats them opaquely?). Or maybe it could even
be passed on the stack, though that looked tricky because there are so many
levels.of calls.

In any case, this shows the augmented error message that I think would be
ideal. If the idea is good and the implementation can be improved, I'm
willing to fix it up, if you give me some guidelines.

Marc
Post by Marc Abramowitz
Recently a well-known Python database module developer was using pymssql
and it wasn't working for him because he was connecting to a SQL Server on
a non-standard port and he had put this port in an entry in his
freetds.conf and was able to connect with tsql, but he was getting errors
when trying to connect with pymssql, which turned out to be because pymssql
calls DB-LIB functions that don't end up consulting freetds.conf to get the
port and so the connect was failing for him, whereas it passed with tsql,
so he thought that he had discovered a bug in pymssql.
The solution was simple. By adding an explicit `port` argument to his
Python code, stuff worked. But we only discovered this after I advised him
about TDSDUMPCONFIG and such and ultimately figured it out.
The takeaway for me and he said this as well, is that it would be nice if
there were better error messages on connection failure. Either from pymssql
or FreeTDS. This would help people diagnose their connection issues much
quicker.
Unable to connect: Adaptive Server is unavailable or does not exist
OS error 61, "Connection refused"
Note that it doesn't say what IP address, port, etc. it was trying to
connect to.
Is there a way to either augment the default FreeTDS error messages --
e.g.: "Connection refused to 127.0.0.1:1433" or DB-API calls that, given
the dbproc or such, will give the IP address, port, etc. so that I can
print them out in my error handler?
Marc
Frediano Ziglio
2014-01-05 19:21:27 UTC
Permalink
Hi Marc,
should be much easier than this. TDSSOCKET has a login parameter
that during login is not NULL and contains the login informations. If
is not valid... I think it should!

Frediano
Post by Marc Abramowitz
https://gitorious.org/freetds/freetds/merge_requests/24
The particular implementation here is probably not ideal, as I stored the
server name/port in a global variable, which seems pretty icky. I
considered adding a field to the dbproc struct -- this seems like a better
place for it but then I shied away from doing that because I wondered if
you guys try to limit adding stuff to structs in case it affects binary
compatibility? (though perhaps it's OK if the client always deals with
pointers to the structs and treats them opaquely?). Or maybe it could even
be passed on the stack, though that looked tricky because there are so many
levels.of calls.
In any case, this shows the augmented error message that I think would be
ideal. If the idea is good and the implementation can be improved, I'm
willing to fix it up, if you give me some guidelines.
Marc
Post by Marc Abramowitz
Recently a well-known Python database module developer was using pymssql
and it wasn't working for him because he was connecting to a SQL Server on
a non-standard port and he had put this port in an entry in his
freetds.conf and was able to connect with tsql, but he was getting errors
when trying to connect with pymssql, which turned out to be because pymssql
calls DB-LIB functions that don't end up consulting freetds.conf to get the
port and so the connect was failing for him, whereas it passed with tsql,
so he thought that he had discovered a bug in pymssql.
The solution was simple. By adding an explicit `port` argument to his
Python code, stuff worked. But we only discovered this after I advised him
about TDSDUMPCONFIG and such and ultimately figured it out.
The takeaway for me and he said this as well, is that it would be nice if
there were better error messages on connection failure. Either from pymssql
or FreeTDS. This would help people diagnose their connection issues much
quicker.
Unable to connect: Adaptive Server is unavailable or does not exist
OS error 61, "Connection refused"
Note that it doesn't say what IP address, port, etc. it was trying to
connect to.
Is there a way to either augment the default FreeTDS error messages --
e.g.: "Connection refused to 127.0.0.1:1433" or DB-API calls that, given
the dbproc or such, will give the IP address, port, etc. so that I can
print them out in my error handler?
Marc
_______________________________________________
FreeTDS mailing list
FreeTDS at lists.ibiblio.org
http://lists.ibiblio.org/mailman/listinfo/freetds
Marc Abramowitz
2014-01-06 19:02:56 UTC
Permalink
Ah, thanks. That *is* a lot easier!
Post by Frediano Ziglio
Hi Marc,
should be much easier than this. TDSSOCKET has a login parameter
that during login is not NULL and contains the login informations. If
is not valid... I think it should!
Frediano
Post by Marc Abramowitz
I think code may better explain what I'm looking for. Something like
https://gitorious.org/freetds/freetds/merge_requests/24
The particular implementation here is probably not ideal, as I stored the
server name/port in a global variable, which seems pretty icky. I
considered adding a field to the dbproc struct -- this seems like a
better
Post by Marc Abramowitz
place for it but then I shied away from doing that because I wondered if
you guys try to limit adding stuff to structs in case it affects binary
compatibility? (though perhaps it's OK if the client always deals with
pointers to the structs and treats them opaquely?). Or maybe it could
even
Post by Marc Abramowitz
be passed on the stack, though that looked tricky because there are so
many
Post by Marc Abramowitz
levels.of calls.
In any case, this shows the augmented error message that I think would be
ideal. If the idea is good and the implementation can be improved, I'm
willing to fix it up, if you give me some guidelines.
Marc
On Thu, Jan 2, 2014 at 6:31 PM, Marc Abramowitz <msabramo at gmail.com>
Post by Marc Abramowitz
Recently a well-known Python database module developer was using pymssql
and it wasn't working for him because he was connecting to a SQL Server
on
Post by Marc Abramowitz
Post by Marc Abramowitz
a non-standard port and he had put this port in an entry in his
freetds.conf and was able to connect with tsql, but he was getting
errors
Post by Marc Abramowitz
Post by Marc Abramowitz
when trying to connect with pymssql, which turned out to be because
pymssql
Post by Marc Abramowitz
Post by Marc Abramowitz
calls DB-LIB functions that don't end up consulting freetds.conf to get
the
Post by Marc Abramowitz
Post by Marc Abramowitz
port and so the connect was failing for him, whereas it passed with
tsql,
Post by Marc Abramowitz
Post by Marc Abramowitz
so he thought that he had discovered a bug in pymssql.
The solution was simple. By adding an explicit `port` argument to his
Python code, stuff worked. But we only discovered this after I advised
him
Post by Marc Abramowitz
Post by Marc Abramowitz
about TDSDUMPCONFIG and such and ultimately figured it out.
The takeaway for me and he said this as well, is that it would be nice
if
Post by Marc Abramowitz
Post by Marc Abramowitz
there were better error messages on connection failure. Either from
pymssql
Post by Marc Abramowitz
Post by Marc Abramowitz
or FreeTDS. This would help people diagnose their connection issues much
quicker.
Unable to connect: Adaptive Server is unavailable or does not
exist
Post by Marc Abramowitz
Post by Marc Abramowitz
OS error 61, "Connection refused"
Note that it doesn't say what IP address, port, etc. it was trying to
connect to.
Is there a way to either augment the default FreeTDS error messages --
e.g.: "Connection refused to 127.0.0.1:1433" or DB-API calls that,
given
Post by Marc Abramowitz
Post by Marc Abramowitz
the dbproc or such, will give the IP address, port, etc. so that I can
print them out in my error handler?
Marc
_______________________________________________
FreeTDS mailing list
FreeTDS at lists.ibiblio.org
http://lists.ibiblio.org/mailman/listinfo/freetds
_______________________________________________
FreeTDS mailing list
FreeTDS at lists.ibiblio.org
http://lists.ibiblio.org/mailman/listinfo/freetds
Frediano Ziglio
2014-01-05 19:32:42 UTC
Permalink
The problem of your implementation is that TDSECONN is passed by
libTDS and not all libraries add the required parameter so you can
have a not formatted string in other libraries.

Mixing normal string and string with format looks a bit security suspect to me.

Frediano
Post by Marc Abramowitz
https://gitorious.org/freetds/freetds/merge_requests/24
The particular implementation here is probably not ideal, as I stored the
server name/port in a global variable, which seems pretty icky. I
considered adding a field to the dbproc struct -- this seems like a better
place for it but then I shied away from doing that because I wondered if
you guys try to limit adding stuff to structs in case it affects binary
compatibility? (though perhaps it's OK if the client always deals with
pointers to the structs and treats them opaquely?). Or maybe it could even
be passed on the stack, though that looked tricky because there are so many
levels.of calls.
In any case, this shows the augmented error message that I think would be
ideal. If the idea is good and the implementation can be improved, I'm
willing to fix it up, if you give me some guidelines.
Marc
Post by Marc Abramowitz
Recently a well-known Python database module developer was using pymssql
and it wasn't working for him because he was connecting to a SQL Server on
a non-standard port and he had put this port in an entry in his
freetds.conf and was able to connect with tsql, but he was getting errors
when trying to connect with pymssql, which turned out to be because pymssql
calls DB-LIB functions that don't end up consulting freetds.conf to get the
port and so the connect was failing for him, whereas it passed with tsql,
so he thought that he had discovered a bug in pymssql.
The solution was simple. By adding an explicit `port` argument to his
Python code, stuff worked. But we only discovered this after I advised him
about TDSDUMPCONFIG and such and ultimately figured it out.
The takeaway for me and he said this as well, is that it would be nice if
there were better error messages on connection failure. Either from pymssql
or FreeTDS. This would help people diagnose their connection issues much
quicker.
Unable to connect: Adaptive Server is unavailable or does not exist
OS error 61, "Connection refused"
Note that it doesn't say what IP address, port, etc. it was trying to
connect to.
Is there a way to either augment the default FreeTDS error messages --
e.g.: "Connection refused to 127.0.0.1:1433" or DB-API calls that, given
the dbproc or such, will give the IP address, port, etc. so that I can
print them out in my error handler?
Marc
_______________________________________________
FreeTDS mailing list
FreeTDS at lists.ibiblio.org
http://lists.ibiblio.org/mailman/listinfo/freetds
Marc Abramowitz
2014-01-06 19:08:10 UTC
Permalink
Post by Frediano Ziglio
The problem of your implementation is that TDSECONN is passed by
libTDS and not all libraries add the required parameter so you can
have a not formatted string in other libraries.
Mixing normal string and string with format looks a bit security suspect to me.
Yeah, that smelled a little funny to me too.

I updated my PR so that dbperror itself appends the server_name, if
available, to the error message. So no need to pass it into dbperror.

https://gitorious.org/freetds/freetds/merge_requests/24/diffs

Hopefully, that's better.

Marc
Frediano Ziglio
2014-01-09 11:19:41 UTC
Permalink
Hi,
Dstr fields should not be acccessed directly. Are you sure buffer is
freed?

Beside this patch looks good. Perhaps should be moved in libtds so all
layers will use this code.

About dynamic parameters values should be controlled as are only errors
from our library, not from server

Frediano
Post by Marc Abramowitz
Post by Frediano Ziglio
The problem of your implementation is that TDSECONN is passed by
libTDS and not all libraries add the required parameter so you can
have a not formatted string in other libraries.
Mixing normal string and string with format looks a bit security suspect to me.
Yeah, that smelled a little funny to me too.
I updated my PR so that dbperror itself appends the server_name, if
available, to the error message. So no need to pass it into dbperror.
https://gitorious.org/freetds/freetds/merge_requests/24/diffs
Hopefully, that's better.
Marc
_______________________________________________
FreeTDS mailing list
FreeTDS at lists.ibiblio.org
http://lists.ibiblio.org/mailman/listinfo/freetds
Marc Abramowitz
2014-01-10 18:01:52 UTC
Permalink
Hi Freddy,

I just updated the merge request so that I don't access the DSTR fields
directly.

As for the buffer being freed, I also had concerns about that, but I think
it's okay. You will notice that this code is very similar to the code right
above it for doing the dynamic parameter substitution -- they both do:

```
constructed_message.msgtext = buffer;
```

Then later on this happens:

```
/* we're done with the dynamic string now. */
free((char*) constructed_message.msgtext);
```

so that should free the buffer. So I think it's good.

I have not looked at libtds. pymssql only uses dblib, so I don't have a use
case and it would be harder for me to test. Perhaps someone else would be
better suited for moving it to libtds?

Let me know if other changes are needed or if you want the commits
squashed, etc.

Cheers,
Marc
Post by Frediano Ziglio
Hi,
Dstr fields should not be acccessed directly. Are you sure buffer is
freed?
Beside this patch looks good. Perhaps should be moved in libtds so all
layers will use this code.
About dynamic parameters values should be controlled as are only errors
from our library, not from server
Frediano
Post by Marc Abramowitz
Post by Frediano Ziglio
The problem of your implementation is that TDSECONN is passed by
libTDS and not all libraries add the required parameter so you can
have a not formatted string in other libraries.
Mixing normal string and string with format looks a bit security
suspect
Post by Marc Abramowitz
Post by Frediano Ziglio
to me.
Yeah, that smelled a little funny to me too.
I updated my PR so that dbperror itself appends the server_name, if
available, to the error message. So no need to pass it into dbperror.
https://gitorious.org/freetds/freetds/merge_requests/24/diffs
Hopefully, that's better.
Marc
_______________________________________________
FreeTDS mailing list
FreeTDS at lists.ibiblio.org
http://lists.ibiblio.org/mailman/listinfo/freetds
_______________________________________________
FreeTDS mailing list
FreeTDS at lists.ibiblio.org
http://lists.ibiblio.org/mailman/listinfo/freetds
Frediano Ziglio
2014-01-13 13:22:24 UTC
Permalink
Hi,
patch is nicer. However is construct_message is already used you get
a leak as you not free the buffer allocated before (your buffer is
then freed after calling the error handler). Also are you sure you
want to add the server name details every time you have a login? Note
that login field is NULL if you are connected so probably you will
have a core for errors after the connection! I would also use asprintf
to make buffer computation easier.

Something like

if (dbproc->tds_socket->login &&
!tds_dstr_isempty(&dbproc->tds_socket->login->server_name)) {
char * buffer = NULL;
if (asprintf(&buffer, "%s (%s)", msg->msgtext,
tds_dstr_cstr(&dbproc->tds_socket->login->server_name)) >= 0) {
free((char*) constructed_message.msgtext);
constructed_message.msgtext = buffer;
constructed_message.severity = msg->severity;
msg = &constructed_message;
}
}

Frediano
Post by Marc Abramowitz
Hi Freddy,
I just updated the merge request so that I don't access the DSTR fields
directly.
As for the buffer being freed, I also had concerns about that, but I think
it's okay. You will notice that this code is very similar to the code right
```
constructed_message.msgtext = buffer;
```
```
/* we're done with the dynamic string now. */
free((char*) constructed_message.msgtext);
```
so that should free the buffer. So I think it's good.
I have not looked at libtds. pymssql only uses dblib, so I don't have a use
case and it would be harder for me to test. Perhaps someone else would be
better suited for moving it to libtds?
Let me know if other changes are needed or if you want the commits
squashed, etc.
Cheers,
Marc
Post by Frediano Ziglio
Hi,
Dstr fields should not be acccessed directly. Are you sure buffer is
freed?
Beside this patch looks good. Perhaps should be moved in libtds so all
layers will use this code.
About dynamic parameters values should be controlled as are only errors
from our library, not from server
Frediano
Post by Marc Abramowitz
Post by Frediano Ziglio
The problem of your implementation is that TDSECONN is passed by
libTDS and not all libraries add the required parameter so you can
have a not formatted string in other libraries.
Mixing normal string and string with format looks a bit security
suspect
Post by Marc Abramowitz
Post by Frediano Ziglio
to me.
Yeah, that smelled a little funny to me too.
I updated my PR so that dbperror itself appends the server_name, if
available, to the error message. So no need to pass it into dbperror.
https://gitorious.org/freetds/freetds/merge_requests/24/diffs
Hopefully, that's better.
Marc
_______________________________________________
FreeTDS mailing list
FreeTDS at lists.ibiblio.org
http://lists.ibiblio.org/mailman/listinfo/freetds
_______________________________________________
FreeTDS mailing list
FreeTDS at lists.ibiblio.org
http://lists.ibiblio.org/mailman/listinfo/freetds
_______________________________________________
FreeTDS mailing list
FreeTDS at lists.ibiblio.org
http://lists.ibiblio.org/mailman/listinfo/freetds
Marc Abramowitz
2014-01-18 12:12:22 UTC
Permalink
Great comments, thanks!

I updated the merge request and also went ahead and created a squashed
version with a single commit:

https://gitorious.org/freetds/freetds/merge_requests/26

Let me know if you want other changes.
Post by Frediano Ziglio
Hi,
patch is nicer. However is construct_message is already used you get
a leak as you not free the buffer allocated before (your buffer is
then freed after calling the error handler). Also are you sure you
want to add the server name details every time you have a login? Note
that login field is NULL if you are connected so probably you will
have a core for errors after the connection! I would also use asprintf
to make buffer computation easier.
Something like
if (dbproc->tds_socket->login &&
!tds_dstr_isempty(&dbproc->tds_socket->login->server_name)) {
char * buffer = NULL;
if (asprintf(&buffer, "%s (%s)", msg->msgtext,
tds_dstr_cstr(&dbproc->tds_socket->login->server_name)) >= 0) {
free((char*) constructed_message.msgtext);
constructed_message.msgtext = buffer;
constructed_message.severity = msg->severity;
msg = &constructed_message;
}
}
Frediano
Post by Marc Abramowitz
Hi Freddy,
I just updated the merge request so that I don't access the DSTR fields
directly.
As for the buffer being freed, I also had concerns about that, but I
think
Post by Marc Abramowitz
it's okay. You will notice that this code is very similar to the code
right
Post by Marc Abramowitz
```
constructed_message.msgtext = buffer;
```
```
/* we're done with the dynamic string now. */
free((char*) constructed_message.msgtext);
```
so that should free the buffer. So I think it's good.
I have not looked at libtds. pymssql only uses dblib, so I don't have a
use
Post by Marc Abramowitz
case and it would be harder for me to test. Perhaps someone else would be
better suited for moving it to libtds?
Let me know if other changes are needed or if you want the commits
squashed, etc.
Cheers,
Marc
On Thu, Jan 9, 2014 at 3:19 AM, Frediano Ziglio <freddy77 at gmail.com>
Post by Frediano Ziglio
Hi,
Dstr fields should not be acccessed directly. Are you sure buffer is
freed?
Beside this patch looks good. Perhaps should be moved in libtds so all
layers will use this code.
About dynamic parameters values should be controlled as are only errors
from our library, not from server
Frediano
Post by Marc Abramowitz
Post by Frediano Ziglio
The problem of your implementation is that TDSECONN is passed by
libTDS and not all libraries add the required parameter so you can
have a not formatted string in other libraries.
Mixing normal string and string with format looks a bit security
suspect
Post by Marc Abramowitz
Post by Frediano Ziglio
to me.
Yeah, that smelled a little funny to me too.
I updated my PR so that dbperror itself appends the server_name, if
available, to the error message. So no need to pass it into dbperror.
https://gitorious.org/freetds/freetds/merge_requests/24/diffs
Hopefully, that's better.
Marc
_______________________________________________
FreeTDS mailing list
FreeTDS at lists.ibiblio.org
http://lists.ibiblio.org/mailman/listinfo/freetds
_______________________________________________
FreeTDS mailing list
FreeTDS at lists.ibiblio.org
http://lists.ibiblio.org/mailman/listinfo/freetds
_______________________________________________
FreeTDS mailing list
FreeTDS at lists.ibiblio.org
http://lists.ibiblio.org/mailman/listinfo/freetds
_______________________________________________
FreeTDS mailing list
FreeTDS at lists.ibiblio.org
http://lists.ibiblio.org/mailman/listinfo/freetds
Frediano Ziglio
2014-01-18 13:53:17 UTC
Permalink
Committed!

Frediano
Post by Marc Abramowitz
Great comments, thanks!
I updated the merge request and also went ahead and created a squashed
https://gitorious.org/freetds/freetds/merge_requests/26
Let me know if you want other changes.
Post by Frediano Ziglio
Hi,
patch is nicer. However is construct_message is already used you get
a leak as you not free the buffer allocated before (your buffer is
then freed after calling the error handler). Also are you sure you
want to add the server name details every time you have a login? Note
that login field is NULL if you are connected so probably you will
have a core for errors after the connection! I would also use asprintf
to make buffer computation easier.
Something like
if (dbproc->tds_socket->login &&
!tds_dstr_isempty(&dbproc->tds_socket->login->server_name)) {
char * buffer = NULL;
if (asprintf(&buffer, "%s (%s)", msg->msgtext,
tds_dstr_cstr(&dbproc->tds_socket->login->server_name)) >= 0) {
free((char*) constructed_message.msgtext);
constructed_message.msgtext = buffer;
constructed_message.severity = msg->severity;
msg = &constructed_message;
}
}
Frediano
Post by Marc Abramowitz
Hi Freddy,
I just updated the merge request so that I don't access the DSTR fields
directly.
As for the buffer being freed, I also had concerns about that, but I
think
Post by Marc Abramowitz
it's okay. You will notice that this code is very similar to the code
right
Post by Marc Abramowitz
```
constructed_message.msgtext = buffer;
```
```
/* we're done with the dynamic string now. */
free((char*) constructed_message.msgtext);
```
so that should free the buffer. So I think it's good.
I have not looked at libtds. pymssql only uses dblib, so I don't have a
use
Post by Marc Abramowitz
case and it would be harder for me to test. Perhaps someone else would be
better suited for moving it to libtds?
Let me know if other changes are needed or if you want the commits
squashed, etc.
Cheers,
Marc
On Thu, Jan 9, 2014 at 3:19 AM, Frediano Ziglio <freddy77 at gmail.com>
Post by Frediano Ziglio
Hi,
Dstr fields should not be acccessed directly. Are you sure buffer is
freed?
Beside this patch looks good. Perhaps should be moved in libtds so all
layers will use this code.
About dynamic parameters values should be controlled as are only errors
from our library, not from server
Frediano
Post by Marc Abramowitz
Post by Frediano Ziglio
The problem of your implementation is that TDSECONN is passed by
libTDS and not all libraries add the required parameter so you can
have a not formatted string in other libraries.
Mixing normal string and string with format looks a bit security
suspect
Post by Marc Abramowitz
Post by Frediano Ziglio
to me.
Yeah, that smelled a little funny to me too.
I updated my PR so that dbperror itself appends the server_name, if
available, to the error message. So no need to pass it into dbperror.
https://gitorious.org/freetds/freetds/merge_requests/24/diffs
Hopefully, that's better.
Marc
_______________________________________________
FreeTDS mailing list
FreeTDS at lists.ibiblio.org
http://lists.ibiblio.org/mailman/listinfo/freetds
_______________________________________________
FreeTDS mailing list
FreeTDS at lists.ibiblio.org
http://lists.ibiblio.org/mailman/listinfo/freetds
_______________________________________________
FreeTDS mailing list
FreeTDS at lists.ibiblio.org
http://lists.ibiblio.org/mailman/listinfo/freetds
_______________________________________________
FreeTDS mailing list
FreeTDS at lists.ibiblio.org
http://lists.ibiblio.org/mailman/listinfo/freetds
_______________________________________________
FreeTDS mailing list
FreeTDS at lists.ibiblio.org
http://lists.ibiblio.org/mailman/listinfo/freetds
Marc Abramowitz
2014-01-18 18:00:36 UTC
Permalink
Cool! Thanks!

-Marc
http://marc-abramowitz.com
Sent from my iPhone 4S
Post by Frediano Ziglio
Committed!
Frediano
Post by Marc Abramowitz
Great comments, thanks!
I updated the merge request and also went ahead and created a squashed
https://gitorious.org/freetds/freetds/merge_requests/26
Let me know if you want other changes.
Post by Frediano Ziglio
Hi,
patch is nicer. However is construct_message is already used you get
a leak as you not free the buffer allocated before (your buffer is
then freed after calling the error handler). Also are you sure you
want to add the server name details every time you have a login? Note
that login field is NULL if you are connected so probably you will
have a core for errors after the connection! I would also use asprintf
to make buffer computation easier.
Something like
if (dbproc->tds_socket->login &&
!tds_dstr_isempty(&dbproc->tds_socket->login->server_name)) {
char * buffer = NULL;
if (asprintf(&buffer, "%s (%s)", msg->msgtext,
tds_dstr_cstr(&dbproc->tds_socket->login->server_name)) >= 0) {
free((char*) constructed_message.msgtext);
constructed_message.msgtext = buffer;
constructed_message.severity = msg->severity;
msg = &constructed_message;
}
}
Frediano
Post by Marc Abramowitz
Hi Freddy,
I just updated the merge request so that I don't access the DSTR fields
directly.
As for the buffer being freed, I also had concerns about that, but I
think
Post by Marc Abramowitz
it's okay. You will notice that this code is very similar to the code
right
Post by Marc Abramowitz
```
constructed_message.msgtext = buffer;
```
```
/* we're done with the dynamic string now. */
free((char*) constructed_message.msgtext);
```
so that should free the buffer. So I think it's good.
I have not looked at libtds. pymssql only uses dblib, so I don't have a
use
Post by Marc Abramowitz
case and it would be harder for me to test. Perhaps someone else would be
better suited for moving it to libtds?
Let me know if other changes are needed or if you want the commits
squashed, etc.
Cheers,
Marc
On Thu, Jan 9, 2014 at 3:19 AM, Frediano Ziglio <freddy77 at gmail.com>
Post by Frediano Ziglio
Hi,
Dstr fields should not be acccessed directly. Are you sure buffer is
freed?
Beside this patch looks good. Perhaps should be moved in libtds so all
layers will use this code.
About dynamic parameters values should be controlled as are only errors
from our library, not from server
Frediano
Post by Marc Abramowitz
Post by Frediano Ziglio
The problem of your implementation is that TDSECONN is passed by
libTDS and not all libraries add the required parameter so you can
have a not formatted string in other libraries.
Mixing normal string and string with format looks a bit security
suspect
Post by Marc Abramowitz
Post by Frediano Ziglio
to me.
Yeah, that smelled a little funny to me too.
I updated my PR so that dbperror itself appends the server_name, if
available, to the error message. So no need to pass it into dbperror.
https://gitorious.org/freetds/freetds/merge_requests/24/diffs
Hopefully, that's better.
Marc
_______________________________________________
FreeTDS mailing list
FreeTDS at lists.ibiblio.org
http://lists.ibiblio.org/mailman/listinfo/freetds
_______________________________________________
FreeTDS mailing list
FreeTDS at lists.ibiblio.org
http://lists.ibiblio.org/mailman/listinfo/freetds
_______________________________________________
FreeTDS mailing list
FreeTDS at lists.ibiblio.org
http://lists.ibiblio.org/mailman/listinfo/freetds
_______________________________________________
FreeTDS mailing list
FreeTDS at lists.ibiblio.org
http://lists.ibiblio.org/mailman/listinfo/freetds
_______________________________________________
FreeTDS mailing list
FreeTDS at lists.ibiblio.org
http://lists.ibiblio.org/mailman/listinfo/freetds
_______________________________________________
FreeTDS mailing list
FreeTDS at lists.ibiblio.org
http://lists.ibiblio.org/mailman/listinfo/freetds
Frediano Ziglio
2014-01-19 14:24:52 UTC
Permalink
Hi,
? I had to do a commit as your patch caused a crash as dbproc or tds_socket could be null

Frediano

-------- Messaggio originale --------
Da: Marc Abramowitz <msabramo at gmail.com>
Data: 18/01/2014 18:00 (GMT+00:00)
A: FreeTDS Development Group <freetds at lists.ibiblio.org>
Cc: FreeTDS Development Group <freetds at lists.ibiblio.org>
Oggetto: Re: [freetds] Way to get connection params on failure?

Cool! Thanks!

-Marc
http://marc-abramowitz.com
Sent from my iPhone 4S
Post by Frediano Ziglio
Committed!
Frediano
Post by Marc Abramowitz
Great comments, thanks!
I updated the merge request and also went ahead and created a squashed
https://gitorious.org/freetds/freetds/merge_requests/26
Let me know if you want other changes.
Hi,
? patch is nicer. However is construct_message is already used you get
a leak as you not free the buffer allocated before (your buffer is
then freed after calling the error handler). Also are you sure you
want to add the server name details every time you have a login? Note
that login field is NULL if you are connected so probably you will
have a core for errors after the connection! I would also use asprintf
to make buffer computation easier.
Something like
if (dbproc->tds_socket->login &&
!tds_dstr_isempty(&dbproc->tds_socket->login->server_name)) {
???? char * buffer = NULL;
???? if (asprintf(&buffer, "%s (%s)", msg->msgtext,
tds_dstr_cstr(&dbproc->tds_socket->login->server_name)) >= 0) {
???????? free((char*) constructed_message.msgtext);
???????? constructed_message.msgtext = buffer;
???????? constructed_message.severity = msg->severity;
???????? msg = &constructed_message;
??? }
}
Frediano
Post by Marc Abramowitz
Hi Freddy,
I just updated the merge request so that I don't access the DSTR fields
directly.
As for the buffer being freed, I also had concerns about that, but I
think
Post by Marc Abramowitz
it's okay. You will notice that this code is very similar to the code
right
Post by Marc Abramowitz
```
??????? constructed_message.msgtext = buffer;
```
```
? /* we're done with the dynamic string now. */
? free((char*) constructed_message.msgtext);
```
so that should free the buffer. So I think it's good.
I have not looked at libtds. pymssql only uses dblib, so I don't have a
use
Post by Marc Abramowitz
case and it would be harder for me to test. Perhaps someone else would be
better suited for moving it to libtds?
Let me know if other changes are needed or if you want the commits
squashed, etc.
Cheers,
Marc
On Thu, Jan 9, 2014 at 3:19 AM, Frediano Ziglio <freddy77 at gmail.com>
Hi,
? Dstr fields should not be acccessed directly. Are you sure buffer is
freed?
Beside this patch looks good. Perhaps should be moved in libtds so all
layers will use this code.
About dynamic parameters values should be controlled as are only errors
from our library, not from server
Frediano
On Sun, Jan 5, 2014 at 11:32 AM, Frediano Ziglio <freddy77 at gmail.com>
Post by Frediano Ziglio
The problem of your implementation is that TDSECONN is passed by
libTDS and not all libraries add the required parameter so you can
have a not formatted string in other libraries.
Mixing normal string and string with format looks a bit security
suspect
Post by Frediano Ziglio
to me.
Yeah, that smelled a little funny to me too.
I updated my PR so that dbperror itself appends the server_name, if
available, to the error message. So no need to pass it into dbperror.
https://gitorious.org/freetds/freetds/merge_requests/24/diffs
Hopefully, that's better.
Marc
Marc Abramowitz
2014-01-19 19:04:58 UTC
Permalink
Ah ok. Oops. sorry 'bout that!
Post by Frediano Ziglio
Hi,
I had to do a commit as your patch caused a crash as dbproc or
tds_socket could be null
Frediano
-------- Messaggio originale --------
Da: Marc Abramowitz <msabramo at gmail.com>
Data: 18/01/2014 18:00 (GMT+00:00)
A: FreeTDS Development Group <freetds at lists.ibiblio.org>
Cc: FreeTDS Development Group <freetds at lists.ibiblio.org>
Oggetto: Re: [freetds] Way to get connection params on failure?
Cool! Thanks!
-Marc
http://marc-abramowitz.com
Sent from my iPhone 4S
Post by Frediano Ziglio
Committed!
Frediano
Post by Marc Abramowitz
Great comments, thanks!
I updated the merge request and also went ahead and created a squashed
https://gitorious.org/freetds/freetds/merge_requests/26
Let me know if you want other changes.
On Mon, Jan 13, 2014 at 5:22 AM, Frediano Ziglio <freddy77 at gmail.com>
Post by Frediano Ziglio
Hi,
patch is nicer. However is construct_message is already used you get
a leak as you not free the buffer allocated before (your buffer is
then freed after calling the error handler). Also are you sure you
want to add the server name details every time you have a login? Note
that login field is NULL if you are connected so probably you will
have a core for errors after the connection! I would also use asprintf
to make buffer computation easier.
Something like
if (dbproc->tds_socket->login &&
!tds_dstr_isempty(&dbproc->tds_socket->login->server_name)) {
char * buffer = NULL;
if (asprintf(&buffer, "%s (%s)", msg->msgtext,
tds_dstr_cstr(&dbproc->tds_socket->login->server_name)) >= 0) {
free((char*) constructed_message.msgtext);
constructed_message.msgtext = buffer;
constructed_message.severity = msg->severity;
msg = &constructed_message;
}
}
Frediano
Post by Marc Abramowitz
Hi Freddy,
I just updated the merge request so that I don't access the DSTR
fields
Post by Frediano Ziglio
Post by Marc Abramowitz
Post by Frediano Ziglio
Post by Marc Abramowitz
directly.
As for the buffer being freed, I also had concerns about that, but I
think
Post by Marc Abramowitz
it's okay. You will notice that this code is very similar to the code
right
Post by Marc Abramowitz
```
constructed_message.msgtext = buffer;
```
```
/* we're done with the dynamic string now. */
free((char*) constructed_message.msgtext);
```
so that should free the buffer. So I think it's good.
I have not looked at libtds. pymssql only uses dblib, so I don't have
a
Post by Frediano Ziglio
Post by Marc Abramowitz
Post by Frediano Ziglio
use
Post by Marc Abramowitz
case and it would be harder for me to test. Perhaps someone else
would be
Post by Frediano Ziglio
Post by Marc Abramowitz
Post by Frediano Ziglio
Post by Marc Abramowitz
better suited for moving it to libtds?
Let me know if other changes are needed or if you want the commits
squashed, etc.
Cheers,
Marc
On Thu, Jan 9, 2014 at 3:19 AM, Frediano Ziglio <freddy77 at gmail.com>
Post by Frediano Ziglio
Hi,
Dstr fields should not be acccessed directly. Are you sure buffer is
freed?
Beside this patch looks good. Perhaps should be moved in libtds so
all
Post by Frediano Ziglio
Post by Marc Abramowitz
Post by Frediano Ziglio
Post by Marc Abramowitz
Post by Frediano Ziglio
layers will use this code.
About dynamic parameters values should be controlled as are only
errors
Post by Frediano Ziglio
Post by Marc Abramowitz
Post by Frediano Ziglio
Post by Marc Abramowitz
Post by Frediano Ziglio
from our library, not from server
Frediano
Il 06/gen/2014 19:08 "Marc Abramowitz" <msabramo at gmail.com> ha
On Sun, Jan 5, 2014 at 11:32 AM, Frediano Ziglio <
freddy77 at gmail.com>
Post by Frediano Ziglio
Post by Marc Abramowitz
Post by Frediano Ziglio
Post by Marc Abramowitz
Post by Frediano Ziglio
Post by Frediano Ziglio
The problem of your implementation is that TDSECONN is passed by
libTDS and not all libraries add the required parameter so you can
have a not formatted string in other libraries.
Mixing normal string and string with format looks a bit security
suspect
Post by Frediano Ziglio
to me.
Yeah, that smelled a little funny to me too.
I updated my PR so that dbperror itself appends the server_name, if
available, to the error message. So no need to pass it into
dbperror.
Post by Frediano Ziglio
Post by Marc Abramowitz
Post by Frediano Ziglio
Post by Marc Abramowitz
Post by Frediano Ziglio
https://gitorious.org/freetds/freetds/merge_requests/24/diffs
Hopefully, that's better.
Marc
_______________________________________________
FreeTDS mailing list
FreeTDS at lists.ibiblio.org
http://lists.ibiblio.org/mailman/listinfo/freetds
Frediano Ziglio
2014-01-19 19:40:57 UTC
Permalink
Don't mind. This is what tests are about. The fact they can discover
defect in few time means they works as expected!

It would be even great if your code would have some test coverage. At
http://freetds.sourceforge.net/up/out92/coverage/src/dblib/dblib.c.gcov.html
you cannot see any on the lines you added.

Frediano
Post by Marc Abramowitz
Ah ok. Oops. sorry 'bout that!
Post by Frediano Ziglio
Hi,
I had to do a commit as your patch caused a crash as dbproc or
tds_socket could be null
Frediano
-------- Messaggio originale --------
Da: Marc Abramowitz <msabramo at gmail.com>
Data: 18/01/2014 18:00 (GMT+00:00)
A: FreeTDS Development Group <freetds at lists.ibiblio.org>
Cc: FreeTDS Development Group <freetds at lists.ibiblio.org>
Oggetto: Re: [freetds] Way to get connection params on failure?
Cool! Thanks!
-Marc
http://marc-abramowitz.com
Sent from my iPhone 4S
Post by Frediano Ziglio
Committed!
Frediano
Post by Marc Abramowitz
Great comments, thanks!
I updated the merge request and also went ahead and created a squashed
https://gitorious.org/freetds/freetds/merge_requests/26
Let me know if you want other changes.
Post by Frediano Ziglio
Hi,
patch is nicer. However is construct_message is already used you get
a leak as you not free the buffer allocated before (your buffer is
then freed after calling the error handler). Also are you sure you
want to add the server name details every time you have a login? Note
that login field is NULL if you are connected so probably you will
have a core for errors after the connection! I would also use asprintf
to make buffer computation easier.
Something like
if (dbproc->tds_socket->login &&
!tds_dstr_isempty(&dbproc->tds_socket->login->server_name)) {
char * buffer = NULL;
if (asprintf(&buffer, "%s (%s)", msg->msgtext,
tds_dstr_cstr(&dbproc->tds_socket->login->server_name)) >= 0) {
free((char*) constructed_message.msgtext);
constructed_message.msgtext = buffer;
constructed_message.severity = msg->severity;
msg = &constructed_message;
}
}
Frediano
Post by Marc Abramowitz
Hi Freddy,
I just updated the merge request so that I don't access the DSTR fields
directly.
As for the buffer being freed, I also had concerns about that, but I
think
Post by Marc Abramowitz
it's okay. You will notice that this code is very similar to the code
right
Post by Marc Abramowitz
```
constructed_message.msgtext = buffer;
```
```
/* we're done with the dynamic string now. */
free((char*) constructed_message.msgtext);
```
so that should free the buffer. So I think it's good.
I have not looked at libtds. pymssql only uses dblib, so I don't have a
use
Post by Marc Abramowitz
case and it would be harder for me to test. Perhaps someone else would be
better suited for moving it to libtds?
Let me know if other changes are needed or if you want the commits
squashed, etc.
Cheers,
Marc
On Thu, Jan 9, 2014 at 3:19 AM, Frediano Ziglio <freddy77 at gmail.com>
Post by Frediano Ziglio
Hi,
Dstr fields should not be acccessed directly. Are you sure buffer is
freed?
Beside this patch looks good. Perhaps should be moved in libtds so all
layers will use this code.
About dynamic parameters values should be controlled as are only errors
from our library, not from server
Frediano
On Sun, Jan 5, 2014 at 11:32 AM, Frediano Ziglio
<freddy77 at gmail.com>
Post by Frediano Ziglio
The problem of your implementation is that TDSECONN is passed by
libTDS and not all libraries add the required parameter so you can
have a not formatted string in other libraries.
Mixing normal string and string with format looks a bit security
suspect
Post by Frediano Ziglio
to me.
Yeah, that smelled a little funny to me too.
I updated my PR so that dbperror itself appends the server_name, if
available, to the error message. So no need to pass it into dbperror.
https://gitorious.org/freetds/freetds/merge_requests/24/diffs
Hopefully, that's better.
Marc
_______________________________________________
FreeTDS mailing list
FreeTDS at lists.ibiblio.org
http://lists.ibiblio.org/mailman/listinfo/freetds
Loading...