Discussion:
[freetds] Encrypted connection
Ryan Lavelle
2010-10-06 22:23:24 UTC
Permalink
I've setup freeTDS 0.82 with encryption=required under the DNS info in the
freetds.conf to a SQL Server 2005 server, with
TDS version 8 and have been using tcpdump to view the packets going from and
to my host machine to make sure the connection
is encrypted, it connects but when I view the TCPDump, I can clearly see the
SQL statement I'm passing and records returned as
plain chars and unencrypted, is this normal? I thought it would encrypt
the entire connection not just the login, what's weird is if I recompile
freeTDS without the --with-openssl or --with-gnutls, it's still able to
connect even though encryption=required is set.

Regards,

Ryan Lavelle
Plot Lost
2010-10-07 03:54:51 UTC
Permalink
Post by Ryan Lavelle
I've setup freeTDS 0.82 with encryption=required under the DNS info in the
freetds.conf to a SQL Server 2005 server, with
TDS version 8 and have been using tcpdump to view the packets going from and
to my host machine to make sure the connection
is encrypted, it connects but when I view the TCPDump, I can clearly see the
SQL statement I'm passing and records returned as
plain chars and unencrypted, is this normal? I thought it would encrypt
the entire connection not just the login, what's weird is if I recompile
freeTDS without the --with-openssl or --with-gnutls, it's still able to
connect even though encryption=required is set.
Regards,
Ryan Lavelle
If no SSL library is included (openssl or gnutls) then FreeTDS 0.82 does
not check the encryption setting, instead it relies on the server rejecting
the connection. The server will only reject the connection if it has
encryption required set wihin it's own config.

This has been changed in the nightly snapshot (0.83) - you could try that to
see if it helps, with the usual caveats about using nightly releases...
Post by Ryan Lavelle
From my recent experience of this, OpenSSL does not work as expected - only
GnuTLS is able to connect correctly.
Ryan Lavelle
2010-10-07 05:17:42 UTC
Permalink
Post by Plot Lost
Post by Ryan Lavelle
I've setup freeTDS 0.82 with encryption=required under the DNS info in
the
Post by Ryan Lavelle
freetds.conf to a SQL Server 2005 server, with
TDS version 8 and have been using tcpdump to view the packets going from and
to my host machine to make sure the connection
is encrypted, it connects but when I view the TCPDump, I can clearly see the
SQL statement I'm passing and records returned as
plain chars and unencrypted, is this normal? I thought it would encrypt
the entire connection not just the login, what's weird is if I recompile
freeTDS without the --with-openssl or --with-gnutls, it's still able to
connect even though encryption=required is set.
Regards,
Ryan Lavelle
If no SSL library is included (openssl or gnutls) then FreeTDS 0.82 does
not check the encryption setting, instead it relies on the server rejecting
the connection. The server will only reject the connection if it has
encryption required set wihin it's own config.
This has been changed in the nightly snapshot (0.83) - you could try that to
see if it helps, with the usual caveats about using nightly releases...
From my recent experience of this, OpenSSL does not work as expected - only
GnuTLS is able to connect correctly.
_______________________________________________
Ok I recompiled with Gnutls, and now I see something about SSL-Self Signed
fallback in the tcp dump, is that because the server does not have an SSL
certificate? Because I can still see some plain-text unencrypted chars in
the tcp dump.
Plot Lost
2010-10-07 05:53:25 UTC
Permalink
Post by Ryan Lavelle
Ok I recompiled with Gnutls, and now I see something about SSL-Self Signed
fallback in the tcp dump, is that because the server does not have an SSL
certificate? Because I can still see some plain-text unencrypted chars in
the tcp dump.
_______________________________________________
FreeTDS mailing list
FreeTDS at lists.ibiblio.org
http://lists.ibiblio.org/mailman/listinfo/freetds
If the server does not have it's own certificate, then it will
auto-generate a self signed one. This is fine for use with things like
FreeTDS, only clients that do strict certificate checking will complain.

Sounds like FreeTDS is not actually seing your encryption=required setting.
It notices that the server supports encryption so will use that during the
login phase, but once that is complete it reverts back to clear text.

As a quick hack to prove this one way or the other, you could add the
following to the login.c file (in src/tds) in the tds8_do_login function

connection->encryption_level=TDS_ENCRYPTION_REQUIRE;

This should go before it actually builds the pre-login packet, so maybe just
after the line that has

tds->out_flag = TDS8_PRELOGIN;

What this will do is force FreeTDS to think that encryption=require has been
set (as for some reason it is not seeing it at the moment). A side effect of
this will be though that you can never then use FreeTDS to connect to a
server that does not support encryption, which is why I mention this just as
a 'quick-hack' type test. (dependant on TDS version being used to connect
actually supporting encryption)
Marc Abramowitz
2013-10-17 20:11:53 UTC
Permalink
Post by Plot Lost
Sounds like FreeTDS is not actually seing your encryption=required setting.
I just went through the same thing as in this thread and it's 2013. Posting
this here for anyone who stumbles upon this thread like I did.

I found the problem. The FreeTDS userguide says that the encryption setting is
"required", but if you look in the code, it's actually "require". Compounding
this, FreeTDS will ignore the setting and you won't know unless you've set
TDSDUMPCONFIG.

Ouch.

Merge request to correct the userguide:
https://gitorious.org/freetds/freetds/merge_requests/9

Merge request to make an invalid encryption setting a fatal error:
https://gitorious.org/freetds/freetds/merge_requests/10

More details at this post:

http://permalink.gmane.org/gmane.comp.db.tds.freetds/15259
James K. Lowden
2013-10-17 23:28:31 UTC
Permalink
On Thu, 17 Oct 2013 20:11:53 +0000 (UTC)
Post by Marc Abramowitz
I found the problem. The FreeTDS userguide says that the encryption
setting is "required", but if you look in the code, it's actually
"require". Compounding this, FreeTDS will ignore the setting and you
won't know unless you've set TDSDUMPCONFIG.
Ouch.
https://gitorious.org/freetds/freetds/merge_requests/9
https://gitorious.org/freetds/freetds/merge_requests/10
Ouch indeed. I think the next release of FreeTDS should probably
reject all incorrect configurations, both invalid keywords and
out-of-range values. Diagnosing configuration errors is easier if the
error is obvious, and the remedy is simple.

Thanks for chasing this down.

--jkl
Marc Abramowitz
2013-10-19 16:48:12 UTC
Permalink
Post by James K. Lowden
Post by Marc Abramowitz
https://gitorious.org/freetds/freetds/merge_requests/10
Ouch indeed. I think the next release of FreeTDS should probably
reject all incorrect configurations, both invalid keywords and
out-of-range values. Diagnosing configuration errors is easier if the
error is obvious, and the remedy is simple.
Freddy pointed out that my merge request wasn't acceptable because printing to stderr and exit aren't appropriate for a library.

So I could you use tdsdump_log with TDS_DBG_SEVERE -- but that is unnoticeable unless TDSDUMPCONFIG is set (and even then, it can be overlooked in the sea of output).

Is there some middle ground - some way of indicating a config error (esp. a serious config error like an invalid encryption setting) that is very noticeable but doesn't overstep what a library should do?

Marc
James K. Lowden
2013-10-19 18:49:23 UTC
Permalink
On Sat, 19 Oct 2013 09:48:12 -0700
Post by Marc Abramowitz
Post by James K. Lowden
Post by Marc Abramowitz
https://gitorious.org/freetds/freetds/merge_requests/10
Ouch indeed. I think the next release of FreeTDS should probably
reject all incorrect configurations, both invalid keywords and
out-of-range values. Diagnosing configuration errors is easier if
the error is obvious, and the remedy is simple.
Freddy pointed out that my merge request wasn't acceptable because
printing to stderr and exit aren't appropriate for a library.
...
Post by Marc Abramowitz
Is there some middle ground - some way of indicating a config error
(esp. a serious config error like an invalid encryption setting) that
is very noticeable but doesn't overstep what a library should do?
The Right Way is to hook into the error handler. Add something like
this to the table in sybdb.h:

#define SYBECONF 2501 /* local configuration error */

and to the dblib.c::dblib_error_messages table

and invoke that error with tdserror or dbperror.

It's not obvious to me that printing to standard error and exiting is
all that bad, though.

In general, there are two kinds of errors:

1. logic errors, caused by the programmer getting something wrong.
Examples include off-by-one errors and failure to check for a NULL
pointer. These kinds of problems can and do cause the library to
exit via e.g. assert(3). There is no point in continuing, because the
program has reached an "impossible" state that the program did not
anticipate and thus cannot handle.

2. runtime errors, caused by invalid user input or dynamic failures
such as network disconnection or the server not responding. These
should always return an error to the user, who is the only person in a
position to deal with it.

Is an invalid configuration a logic or runtime error? is the
administrator a programmer or a user?

I would say the configuration file is more like program text. It is
static data isolated from the program in such a way that it can be
modified without requiring recompilation. The very first time it is
read -- compiled, if you will -- it will be validated. If it is
invalid, the library exits. Once the configuration is valid, no amount
of other invalid input will cause it to become invalid.

Printing to stderr is unfortunately not robust, because some
applications will not have that descriptor open. Invoking the error
handler is is more general, but will go unseen if the application
doesn't install and error handler. On the other hand, the problem can be
instantantly diagnosed by using any command-line tool such as tsql.

So, IMO printing to stderr and exiting is fine, and invoking the error
handler is in some ways friendlier (perhaps in addition to writing to
stderr). Exiting is OK, too, even if it does force the admin to clean
up errors that might otherwise be innocuous.

HTH.

--jkl
Marc Abramowitz
2013-10-21 20:48:47 UTC
Permalink
That makes sense.

OK, I took another stab at creating a louder notification when the
encryption setting is incorrect:

*https://gitorious.org/freetds/freetds/merge_requests/16* ("Improve
handling of invalid encryption setting")

The goal here is to make it much more noticeable when there is an invalid
encryption setting. I do this by:

- calling tdserror with a new error code (TDSECONF = 20214 = "Local
configuration error")
- print a message to stderr in case in case someone is looking there
- exiting the process if the ENABLE_EXIT_ON_INVALID_CONFIG is defined
(defined by passing --enable-exit-on-invalid-config to./configure).
Post by James K. Lowden
On Sat, 19 Oct 2013 09:48:12 -0700
Post by Marc Abramowitz
Post by James K. Lowden
Post by Marc Abramowitz
https://gitorious.org/freetds/freetds/merge_requests/10
Ouch indeed. I think the next release of FreeTDS should probably
reject all incorrect configurations, both invalid keywords and
out-of-range values. Diagnosing configuration errors is easier if
the error is obvious, and the remedy is simple.
Freddy pointed out that my merge request wasn't acceptable because
printing to stderr and exit aren't appropriate for a library.
...
Post by Marc Abramowitz
Is there some middle ground - some way of indicating a config error
(esp. a serious config error like an invalid encryption setting) that
is very noticeable but doesn't overstep what a library should do?
The Right Way is to hook into the error handler. Add something like
#define SYBECONF 2501 /* local configuration error */
and to the dblib.c::dblib_error_messages table
and invoke that error with tdserror or dbperror.
It's not obvious to me that printing to standard error and exiting is
all that bad, though.
1. logic errors, caused by the programmer getting something wrong.
Examples include off-by-one errors and failure to check for a NULL
pointer. These kinds of problems can and do cause the library to
exit via e.g. assert(3). There is no point in continuing, because the
program has reached an "impossible" state that the program did not
anticipate and thus cannot handle.
2. runtime errors, caused by invalid user input or dynamic failures
such as network disconnection or the server not responding. These
should always return an error to the user, who is the only person in a
position to deal with it.
Is an invalid configuration a logic or runtime error? is the
administrator a programmer or a user?
I would say the configuration file is more like program text. It is
static data isolated from the program in such a way that it can be
modified without requiring recompilation. The very first time it is
read -- compiled, if you will -- it will be validated. If it is
invalid, the library exits. Once the configuration is valid, no amount
of other invalid input will cause it to become invalid.
Printing to stderr is unfortunately not robust, because some
applications will not have that descriptor open. Invoking the error
handler is is more general, but will go unseen if the application
doesn't install and error handler. On the other hand, the problem can be
instantantly diagnosed by using any command-line tool such as tsql.
So, IMO printing to stderr and exiting is fine, and invoking the error
handler is in some ways friendlier (perhaps in addition to writing to
stderr). Exiting is OK, too, even if it does force the admin to clean
up errors that might otherwise be innocuous.
HTH.
--jkl
_______________________________________________
FreeTDS mailing list
FreeTDS at lists.ibiblio.org
http://lists.ibiblio.org/mailman/listinfo/freetds
Frediano Ziglio
2013-10-21 23:51:32 UTC
Permalink
I like tdserror. I don't like printf or an additional option. Printf cause it can go nowhere or in dangerous places, an additional option cause it force different users to recompile the same library with different options based on the usage. Using options to deal with debugging or different system configuration is fine but this option would be ok for an application but not for a library used in different places.

What I would do, also suggested by James is something strong. If you pass a wrong option just try to notify upper layer (as tdserror does) and do not connect! Surely user will fix the configuration! Of course should be documented in the version changelog and UG. I'll try to write a partial, not that tested patch. Does it sound reasonable?

Ps: we have still to sort out the Travis patch. Does the configuration file need to be in the main directory? Cannot you change the name? What happen if another developers want to use Travis too?

Frediano Ziglio
Post by Marc Abramowitz
That makes sense.
OK, I took another stab at creating a louder notification when the
*https://gitorious.org/freetds/freetds/merge_requests/16* ("Improve
handling of invalid encryption setting")
The goal here is to make it much more noticeable when there is an invalid
- calling tdserror with a new error code (TDSECONF = 20214 = "Local
configuration error")
- print a message to stderr in case in case someone is looking there
- exiting the process if the ENABLE_EXIT_ON_INVALID_CONFIG is defined
(defined by passing --enable-exit-on-invalid-config to./configure).
Post by James K. Lowden
On Sat, 19 Oct 2013 09:48:12 -0700
Post by Marc Abramowitz
Post by James K. Lowden
Post by Marc Abramowitz
https://gitorious.org/freetds/freetds/merge_requests/10
Ouch indeed. I think the next release of FreeTDS should probably
reject all incorrect configurations, both invalid keywords and
out-of-range values. Diagnosing configuration errors is easier if
the error is obvious, and the remedy is simple.
Freddy pointed out that my merge request wasn't acceptable because
printing to stderr and exit aren't appropriate for a library.
...
Post by Marc Abramowitz
Is there some middle ground - some way of indicating a config error
(esp. a serious config error like an invalid encryption setting) that
is very noticeable but doesn't overstep what a library should do?
The Right Way is to hook into the error handler. Add something like
#define SYBECONF 2501 /* local configuration error */
and to the dblib.c::dblib_error_messages table
and invoke that error with tdserror or dbperror.
It's not obvious to me that printing to standard error and exiting is
all that bad, though.
1. logic errors, caused by the programmer getting something wrong.
Examples include off-by-one errors and failure to check for a NULL
pointer. These kinds of problems can and do cause the library to
exit via e.g. assert(3). There is no point in continuing, because the
program has reached an "impossible" state that the program did not
anticipate and thus cannot handle.
2. runtime errors, caused by invalid user input or dynamic failures
such as network disconnection or the server not responding. These
should always return an error to the user, who is the only person in a
position to deal with it.
Is an invalid configuration a logic or runtime error? is the
administrator a programmer or a user?
I would say the configuration file is more like program text. It is
static data isolated from the program in such a way that it can be
modified without requiring recompilation. The very first time it is
read -- compiled, if you will -- it will be validated. If it is
invalid, the library exits. Once the configuration is valid, no amount
of other invalid input will cause it to become invalid.
Printing to stderr is unfortunately not robust, because some
applications will not have that descriptor open. Invoking the error
handler is is more general, but will go unseen if the application
doesn't install and error handler. On the other hand, the problem can be
instantantly diagnosed by using any command-line tool such as tsql.
So, IMO printing to stderr and exiting is fine, and invoking the error
handler is in some ways friendlier (perhaps in addition to writing to
stderr). Exiting is OK, too, even if it does force the admin to clean
up errors that might otherwise be innocuous.
HTH.
--jkl
_______________________________________________
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
2013-10-22 01:41:57 UTC
Permalink
Post by Frediano Ziglio
Ps: we have still to sort out the Travis patch. Does the configuration file need to be in the main directory? Cannot you change the name? What happen if another developers want to use Travis too?
Yes, I talked to josh-k, one of the Travis authors on IRC #travis. It has to be .travis.yml in the root of the repo.

With most projects that use Travis, developers simply fork the project on GitHub and then flip the switch to enable Travis CI for their fork, without ever changing the .travis.yml (unless they are improving the Travis config). In this case, it's harder because I put encrypted SQL Server credentials in the .travis.yml - so if someone tries to run Travis CI builds on a fork with an unchanged .travis.yml, Travis will fail to decrypt the data for a different repo and their build will fail because it won't be able to connect to a SQL Server. Such folks would have to find their own Internet-accessible SQL Server and then modify the .travis.yml in their fork with new encrypted credentials. I'd be pleasantly surprised if that became an issue (people actually wanting to use Travis CI on their forks) :-)

In short, .travis.yml can't be renamed or moved, so if that's the only concern, I'd vote to merge it.
Craig A. Berry
2013-10-22 02:25:28 UTC
Permalink
Post by Marc Abramowitz
I put encrypted SQL Server credentials in the .travis.yml - so if someone tries to run Travis CI builds on a fork with an unchanged .travis.yml, Travis will fail to decrypt the data for a different repo and their build will fail because it won't be able to connect to a SQL Server.
That doesn't really sound like a good design to me. Why not have the credentials be a build product of some sort? You can't actually connect to anything until you've built FreeTDS, and it doesn't really seem appropriate to have hard-coded credentials in the master repository.
________________________________________
Craig A. Berry
mailto:craigberry at mac.com

"... getting out of a sonnet is much more
difficult than getting in."
Brad Leithauser
Marc Abramowitz
2013-10-22 02:46:46 UTC
Permalink
Post by Craig A. Berry
That doesn't really sound like a good design to me. Why not have the credentials be a build product of some sort?
Built from what though? They have to be persisted somewhere and for better or worse, Travis CI chose to have everything needed for builds stored in the repo itself.
Marc Abramowitz
2013-10-22 15:37:23 UTC
Permalink
Post by Frediano Ziglio
I like tdserror. I don't like printf or an additional option. Printf
cause it
Post by Frediano Ziglio
can go nowhere or in dangerous places, an additional option cause it force
different users to recompile the same library with different options
based on
Post by Frediano Ziglio
the usage. Using options to deal with debugging or different system
configuration is fine but this option would be ok for an application but
not
Post by Frediano Ziglio
for a library used in different places.
What I would do, also suggested by James is something strong. If you pass
a
Post by Frediano Ziglio
wrong option just try to notify upper layer (as tdserror does) and do not
connect! Surely user will fix the configuration! Of course should be
documented in the version changelog and UG. I'll try to write a partial,
not
Post by Frediano Ziglio
that tested patch. Does it sound reasonable?
I had another branch where I did something like that actually. I just
cleaned it up and made another merge request.

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

If that resembles or is useful towards implementing what you suggested,
feel free to use it, or if it's different, you can go ahead and do your own
thing. The idea certainly sounds reasonable.
Post by Frediano Ziglio
I like tdserror. I don't like printf or an additional option. Printf cause
it can go nowhere or in dangerous places, an additional option cause it
force different users to recompile the same library with different options
based on the usage. Using options to deal with debugging or different
system configuration is fine but this option would be ok for an application
but not for a library used in different places.
What I would do, also suggested by James is something strong. If you pass
a wrong option just try to notify upper layer (as tdserror does) and do not
connect! Surely user will fix the configuration! Of course should be
documented in the version changelog and UG. I'll try to write a partial,
not that tested patch. Does it sound reasonable?
Ps: we have still to sort out the Travis patch. Does the configuration
file need to be in the main directory? Cannot you change the name? What
happen if another developers want to use Travis too?
Frediano Ziglio
Post by Marc Abramowitz
That makes sense.
OK, I took another stab at creating a louder notification when the
*https://gitorious.org/freetds/freetds/merge_requests/16* ("Improve
handling of invalid encryption setting")
The goal here is to make it much more noticeable when there is an invalid
- calling tdserror with a new error code (TDSECONF = 20214 = "Local
configuration error")
- print a message to stderr in case in case someone is looking there
- exiting the process if the ENABLE_EXIT_ON_INVALID_CONFIG is defined
(defined by passing --enable-exit-on-invalid-config to./configure).
On Sat, Oct 19, 2013 at 11:49 AM, James K. Lowden <jklowden at freetds.org
Post by James K. Lowden
On Sat, 19 Oct 2013 09:48:12 -0700
Post by Marc Abramowitz
Post by James K. Lowden
Post by Marc Abramowitz
https://gitorious.org/freetds/freetds/merge_requests/10
Ouch indeed. I think the next release of FreeTDS should probably
reject all incorrect configurations, both invalid keywords and
out-of-range values. Diagnosing configuration errors is easier if
the error is obvious, and the remedy is simple.
Freddy pointed out that my merge request wasn't acceptable because
printing to stderr and exit aren't appropriate for a library.
...
Post by Marc Abramowitz
Is there some middle ground - some way of indicating a config error
(esp. a serious config error like an invalid encryption setting) that
is very noticeable but doesn't overstep what a library should do?
The Right Way is to hook into the error handler. Add something like
#define SYBECONF 2501 /* local configuration error */
and to the dblib.c::dblib_error_messages table
and invoke that error with tdserror or dbperror.
It's not obvious to me that printing to standard error and exiting is
all that bad, though.
1. logic errors, caused by the programmer getting something wrong.
Examples include off-by-one errors and failure to check for a NULL
pointer. These kinds of problems can and do cause the library to
exit via e.g. assert(3). There is no point in continuing, because the
program has reached an "impossible" state that the program did not
anticipate and thus cannot handle.
2. runtime errors, caused by invalid user input or dynamic failures
such as network disconnection or the server not responding. These
should always return an error to the user, who is the only person in a
position to deal with it.
Is an invalid configuration a logic or runtime error? is the
administrator a programmer or a user?
I would say the configuration file is more like program text. It is
static data isolated from the program in such a way that it can be
modified without requiring recompilation. The very first time it is
read -- compiled, if you will -- it will be validated. If it is
invalid, the library exits. Once the configuration is valid, no amount
of other invalid input will cause it to become invalid.
Printing to stderr is unfortunately not robust, because some
applications will not have that descriptor open. Invoking the error
handler is is more general, but will go unseen if the application
doesn't install and error handler. On the other hand, the problem can be
instantantly diagnosed by using any command-line tool such as tsql.
So, IMO printing to stderr and exiting is fine, and invoking the error
handler is in some ways friendlier (perhaps in addition to writing to
stderr). Exiting is OK, too, even if it does force the admin to clean
up errors that might otherwise be innocuous.
HTH.
--jkl
_______________________________________________
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
2013-10-22 20:13:30 UTC
Permalink
I wrote something like this

Date: Tue, 22 Oct 2013 21:12:13 +0100
Subject: [PATCH] Simple and probably incomplete patch to check configuration
values.

Signed-off-by: Frediano Ziglio <freddy77 at gmail.com>
---
include/freetds/tds.h | 3 ++-
src/odbc/connectparams.c | 8 ++++----
src/tds/config.c | 22 +++++++++++++---------
src/tds/login.c | 3 +++
4 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/include/freetds/tds.h b/include/freetds/tds.h
index b27fcf6..f4a2621 100644
--- a/include/freetds/tds.h
+++ b/include/freetds/tds.h
@@ -578,6 +578,7 @@ typedef struct tds_login
unsigned int use_ntlmv2:1;
unsigned int use_lanman:1;
unsigned int mars:1;
+ unsigned int invalid_configuration:1;
} TDSLOGIN;

typedef struct tds_locale
@@ -1187,7 +1188,7 @@ const char *tds_addrinfo2str(struct tds_addrinfo
*addr, char *name, int namemax)

TDSRET tds_set_interfaces_file_loc(const char *interfloc);
extern const char STD_DATETIME_FMT[];
-int tds_config_boolean(const char *value);
+int tds_config_boolean(const char *value, TDSLOGIN * login);

TDSLOCALE *tds_get_locale(void);
TDSRET tds_alloc_row(TDSRESULTINFO * res_info);
diff --git a/src/odbc/connectparams.c b/src/odbc/connectparams.c
index b287311..0a910d3 100644
--- a/src/odbc/connectparams.c
+++ b/src/odbc/connectparams.c
@@ -226,12 +226,12 @@ odbc_get_dsn_info(TDS_ERRS *errs, const char
*DSN, TDSLOGIN * login)
if (myGetPrivateProfileString(DSN, odbc_param_ServerSPN, tmp) > 0)
tds_parse_conf_section(TDS_STR_SPN, tmp, login);

- if (myGetPrivateProfileString(DSN, odbc_param_Trusted_Connection,
tmp) > 0 && tds_config_boolean(tmp)) {
+ if (myGetPrivateProfileString(DSN, odbc_param_Trusted_Connection,
tmp) > 0 && tds_config_boolean(tmp, login)) {
tds_dstr_copy(&login->user_name, "");
tds_dstr_copy(&login->password, "");
}

- if (myGetPrivateProfileString(DSN, odbc_param_MARS_Connection,
tmp) > 0 && tds_config_boolean(tmp)) {
+ if (myGetPrivateProfileString(DSN, odbc_param_MARS_Connection,
tmp) > 0 && tds_config_boolean(tmp, login)) {
login->mars = 1;
}

@@ -389,12 +389,12 @@ odbc_parse_connect_string(TDS_ERRS *errs, const
char *connect_string, const char
} else if (CHK_PARAM(ServerSPN)) {
tds_parse_conf_section(TDS_STR_SPN, tds_dstr_cstr(&value), login);
} else if (CHK_PARAM(Trusted_Connection)) {
- trusted = tds_config_boolean(tds_dstr_cstr(&value));
+ trusted = tds_config_boolean(tds_dstr_cstr(&value), login);
tdsdump_log(TDS_DBG_INFO1, "trusted %s -> %d\n",
tds_dstr_cstr(&value), trusted);
num_param = -1;
/* TODO odbc_param_Address field */
} else if (CHK_PARAM(MARS_Connection)) {
- if (tds_config_boolean(tds_dstr_cstr(&value)))
+ if (tds_config_boolean(tds_dstr_cstr(&value), login))
login->mars = 1;
}

diff --git a/src/tds/config.c b/src/tds/config.c
index c4d38e7..41386e5 100644
--- a/src/tds/config.c
+++ b/src/tds/config.c
@@ -399,6 +399,8 @@ tds_read_conf_sections(FILE * in, const char
*server, TDSLOGIN * login)
default_port = login->port;

found = tds_read_conf_section(in, server, tds_parse_conf_section, login);
+ if (login->invalid_configuration)
+ return 0;

/*
* If both instance and port are specified and neither one came
from the default, it's an error
@@ -428,7 +430,7 @@ static const struct {
};

int
-tds_config_boolean(const char *value)
+tds_config_boolean(const char *value, TDSLOGIN *login)
{
int p;

@@ -436,7 +438,7 @@ tds_config_boolean(const char *value)
if (!strcasecmp(value, boolean_values[p].value))
return boolean_values[p].to_return;
}
- tdsdump_log(TDS_DBG_INFO1, "UNRECOGNIZED boolean value: '%s'.
Treating as 'no'.\n", value);
+ login->invalid_configuration = 1;
return 0;
}

@@ -451,8 +453,10 @@ tds_config_encryption(const char * value, TDSLOGIN * login)
lvl = TDS_ENCRYPTION_REQUEST;
else if (!strcasecmp(value, TDS_STR_ENCRYPTION_REQUIRE))
lvl = TDS_ENCRYPTION_REQUIRE;
- else
+ else {
tdsdump_log(TDS_DBG_INFO1, "UNRECOGNIZED option value
'%s'...ignoring.\n", value);
+ login->invalid_configuration = 1;
+ }

login->encryption_level = lvl;
}
@@ -568,10 +572,10 @@ tds_parse_conf_section(const char *option, const
char *value, void *param)
if (val >= 512 && val < 65536)
login->block_size = val;
} else if (!strcmp(option, TDS_STR_SWAPDT)) {
- login->broken_dates = tds_config_boolean(value);
+ login->broken_dates = tds_config_boolean(value, login);
} else if (!strcmp(option, TDS_GSSAPI_DELEGATION)) {
/* gssapi flag addition */
- login->gssapi_use_delegation = tds_config_boolean(value);
+ login->gssapi_use_delegation = tds_config_boolean(value, login);
} else if (!strcmp(option, TDS_STR_DUMPFILE)) {
tds_dstr_copy(&login->dump_file, value);
} else if (!strcmp(option, TDS_STR_DEBUGFLAGS)) {
@@ -604,7 +608,7 @@ tds_parse_conf_section(const char *option, const
char *value, void *param)
if (atoi(value))
login->port = atoi(value);
} else if (!strcmp(option, TDS_STR_EMUL_LE)) {
- login->emul_little_endian = tds_config_boolean(value);
+ login->emul_little_endian = tds_config_boolean(value, login);
} else if (!strcmp(option, TDS_STR_TEXTSZ)) {
if (atoi(value))
login->text_size = atoi(value);
@@ -617,7 +621,7 @@ tds_parse_conf_section(const char *option, const
char *value, void *param)
} else if (!strcmp(option, TDS_STR_LANGUAGE)) {
tds_dstr_copy(&login->language, value);
} else if (!strcmp(option, TDS_STR_APPENDMODE)) {
- tds_g_append_mode = tds_config_boolean(value);
+ tds_g_append_mode = tds_config_boolean(value, login);
} else if (!strcmp(option, TDS_STR_INSTANCE)) {
tds_dstr_copy(&login->instance_name, value);
} else if (!strcmp(option, TDS_STR_ENCRYPTION)) {
@@ -625,9 +629,9 @@ tds_parse_conf_section(const char *option, const
char *value, void *param)
} else if (!strcmp(option, TDS_STR_ASA_DATABASE)) {
tds_dstr_copy(&login->server_name, value);
} else if (!strcmp(option, TDS_STR_USENTLMV2)) {
- login->use_ntlmv2 = tds_config_boolean(value);
+ login->use_ntlmv2 = tds_config_boolean(value, login);
} else if (!strcmp(option, TDS_STR_USELANMAN)) {
- login->use_lanman = tds_config_boolean(value);
+ login->use_lanman = tds_config_boolean(value, login);
} else if (!strcmp(option, TDS_STR_REALM)) {
tds_dstr_copy(&login->server_realm_name, value);
} else if (!strcmp(option, TDS_STR_SPN)) {
diff --git a/src/tds/login.c b/src/tds/login.c
index ec8eb5a..813290b 100644
--- a/src/tds/login.c
+++ b/src/tds/login.c
@@ -333,6 +333,9 @@ tds_connect(TDSSOCKET * tds, TDSLOGIN * login, int *p_oserr)
, 0x402
};

+ if (login->invalid_configuration)
+ return TDS_FAIL;
+
if (TDS_MAJOR(login) == 0) {
unsigned int i;
TDSSAVECONTEXT save_ctx;
--
1.7.9.5
Post by James K. Lowden
Post by Frediano Ziglio
I like tdserror. I don't like printf or an additional option. Printf
cause it
Post by Frediano Ziglio
can go nowhere or in dangerous places, an additional option cause it force
different users to recompile the same library with different options
based on
Post by Frediano Ziglio
the usage. Using options to deal with debugging or different system
configuration is fine but this option would be ok for an application but
not
Post by Frediano Ziglio
for a library used in different places.
What I would do, also suggested by James is something strong. If you pass
a
Post by Frediano Ziglio
wrong option just try to notify upper layer (as tdserror does) and do not
connect! Surely user will fix the configuration! Of course should be
documented in the version changelog and UG. I'll try to write a partial,
not
Post by Frediano Ziglio
that tested patch. Does it sound reasonable?
I had another branch where I did something like that actually. I just
cleaned it up and made another merge request.
https://gitorious.org/freetds/freetds/merge_requests/18
If that resembles or is useful towards implementing what you suggested,
feel free to use it, or if it's different, you can go ahead and do your own
thing. The idea certainly sounds reasonable.
Post by Frediano Ziglio
I like tdserror. I don't like printf or an additional option. Printf cause
it can go nowhere or in dangerous places, an additional option cause it
force different users to recompile the same library with different options
based on the usage. Using options to deal with debugging or different
system configuration is fine but this option would be ok for an application
but not for a library used in different places.
What I would do, also suggested by James is something strong. If you pass
a wrong option just try to notify upper layer (as tdserror does) and do not
connect! Surely user will fix the configuration! Of course should be
documented in the version changelog and UG. I'll try to write a partial,
not that tested patch. Does it sound reasonable?
Ps: we have still to sort out the Travis patch. Does the configuration
file need to be in the main directory? Cannot you change the name? What
happen if another developers want to use Travis too?
Frediano Ziglio
Il giorno 21/ott/2013, alle ore 21:48, Marc Abramowitz <msabramo at gmail.com>
Post by Marc Abramowitz
That makes sense.
OK, I took another stab at creating a louder notification when the
*https://gitorious.org/freetds/freetds/merge_requests/16* ("Improve
handling of invalid encryption setting")
The goal here is to make it much more noticeable when there is an invalid
- calling tdserror with a new error code (TDSECONF = 20214 = "Local
configuration error")
- print a message to stderr in case in case someone is looking there
- exiting the process if the ENABLE_EXIT_ON_INVALID_CONFIG is defined
(defined by passing --enable-exit-on-invalid-config to./configure).
On Sat, Oct 19, 2013 at 11:49 AM, James K. Lowden <jklowden at freetds.org
Post by James K. Lowden
On Sat, 19 Oct 2013 09:48:12 -0700
Post by Marc Abramowitz
Post by James K. Lowden
Post by Marc Abramowitz
https://gitorious.org/freetds/freetds/merge_requests/10
Ouch indeed. I think the next release of FreeTDS should probably
reject all incorrect configurations, both invalid keywords and
out-of-range values. Diagnosing configuration errors is easier if
the error is obvious, and the remedy is simple.
Freddy pointed out that my merge request wasn't acceptable because
printing to stderr and exit aren't appropriate for a library.
...
Post by Marc Abramowitz
Is there some middle ground - some way of indicating a config error
(esp. a serious config error like an invalid encryption setting) that
is very noticeable but doesn't overstep what a library should do?
The Right Way is to hook into the error handler. Add something like
#define SYBECONF 2501 /* local configuration error */
and to the dblib.c::dblib_error_messages table
and invoke that error with tdserror or dbperror.
It's not obvious to me that printing to standard error and exiting is
all that bad, though.
1. logic errors, caused by the programmer getting something wrong.
Examples include off-by-one errors and failure to check for a NULL
pointer. These kinds of problems can and do cause the library to
exit via e.g. assert(3). There is no point in continuing, because the
program has reached an "impossible" state that the program did not
anticipate and thus cannot handle.
2. runtime errors, caused by invalid user input or dynamic failures
such as network disconnection or the server not responding. These
should always return an error to the user, who is the only person in a
position to deal with it.
Is an invalid configuration a logic or runtime error? is the
administrator a programmer or a user?
I would say the configuration file is more like program text. It is
static data isolated from the program in such a way that it can be
modified without requiring recompilation. The very first time it is
read -- compiled, if you will -- it will be validated. If it is
invalid, the library exits. Once the configuration is valid, no amount
of other invalid input will cause it to become invalid.
Printing to stderr is unfortunately not robust, because some
applications will not have that descriptor open. Invoking the error
handler is is more general, but will go unseen if the application
doesn't install and error handler. On the other hand, the problem can be
instantantly diagnosed by using any command-line tool such as tsql.
So, IMO printing to stderr and exiting is fine, and invoking the error
handler is in some ways friendlier (perhaps in addition to writing to
stderr). Exiting is OK, too, even if it does force the admin to clean
up errors that might otherwise be innocuous.
HTH.
--jkl
_______________________________________________
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
2013-10-22 20:46:58 UTC
Permalink
That looks fairly similar but better than mine as you put the invalid_flag
in the tds_login struct instead of a global.

If you email me a file with a patch that's based off master, I can try it
out if you'd like. I had trouble copying and pasting the patch because of
line breaks (from Gmail?) and even after fixing those, the patch didn't
seem to apply cleanly. I'd be glad to try it though.
Post by Frediano Ziglio
I wrote something like this
Date: Tue, 22 Oct 2013 21:12:13 +0100
Subject: [PATCH] Simple and probably incomplete patch to check
configuration
values.
Signed-off-by: Frediano Ziglio <freddy77 at gmail.com>
---
include/freetds/tds.h | 3 ++-
src/odbc/connectparams.c | 8 ++++----
src/tds/config.c | 22 +++++++++++++---------
src/tds/login.c | 3 +++
4 files changed, 22 insertions(+), 14 deletions(-)
diff --git a/include/freetds/tds.h b/include/freetds/tds.h
index b27fcf6..f4a2621 100644
--- a/include/freetds/tds.h
+++ b/include/freetds/tds.h
@@ -578,6 +578,7 @@ typedef struct tds_login
unsigned int use_ntlmv2:1;
unsigned int use_lanman:1;
unsigned int mars:1;
+ unsigned int invalid_configuration:1;
} TDSLOGIN;
typedef struct tds_locale
@@ -1187,7 +1188,7 @@ const char *tds_addrinfo2str(struct tds_addrinfo
*addr, char *name, int namemax)
TDSRET tds_set_interfaces_file_loc(const char *interfloc);
extern const char STD_DATETIME_FMT[];
-int tds_config_boolean(const char *value);
+int tds_config_boolean(const char *value, TDSLOGIN * login);
TDSLOCALE *tds_get_locale(void);
TDSRET tds_alloc_row(TDSRESULTINFO * res_info);
diff --git a/src/odbc/connectparams.c b/src/odbc/connectparams.c
index b287311..0a910d3 100644
--- a/src/odbc/connectparams.c
+++ b/src/odbc/connectparams.c
@@ -226,12 +226,12 @@ odbc_get_dsn_info(TDS_ERRS *errs, const char
*DSN, TDSLOGIN * login)
if (myGetPrivateProfileString(DSN, odbc_param_ServerSPN, tmp) > 0)
tds_parse_conf_section(TDS_STR_SPN, tmp, login);
- if (myGetPrivateProfileString(DSN, odbc_param_Trusted_Connection,
tmp) > 0 && tds_config_boolean(tmp)) {
+ if (myGetPrivateProfileString(DSN, odbc_param_Trusted_Connection,
tmp) > 0 && tds_config_boolean(tmp, login)) {
tds_dstr_copy(&login->user_name, "");
tds_dstr_copy(&login->password, "");
}
- if (myGetPrivateProfileString(DSN, odbc_param_MARS_Connection,
tmp) > 0 && tds_config_boolean(tmp)) {
+ if (myGetPrivateProfileString(DSN, odbc_param_MARS_Connection,
tmp) > 0 && tds_config_boolean(tmp, login)) {
login->mars = 1;
}
@@ -389,12 +389,12 @@ odbc_parse_connect_string(TDS_ERRS *errs, const
char *connect_string, const char
} else if (CHK_PARAM(ServerSPN)) {
tds_parse_conf_section(TDS_STR_SPN, tds_dstr_cstr(&value), login);
} else if (CHK_PARAM(Trusted_Connection)) {
- trusted = tds_config_boolean(tds_dstr_cstr(&value));
+ trusted = tds_config_boolean(tds_dstr_cstr(&value), login);
tdsdump_log(TDS_DBG_INFO1, "trusted %s -> %d\n",
tds_dstr_cstr(&value), trusted);
num_param = -1;
/* TODO odbc_param_Address field */
} else if (CHK_PARAM(MARS_Connection)) {
- if (tds_config_boolean(tds_dstr_cstr(&value)))
+ if (tds_config_boolean(tds_dstr_cstr(&value), login))
login->mars = 1;
}
diff --git a/src/tds/config.c b/src/tds/config.c
index c4d38e7..41386e5 100644
--- a/src/tds/config.c
+++ b/src/tds/config.c
@@ -399,6 +399,8 @@ tds_read_conf_sections(FILE * in, const char
*server, TDSLOGIN * login)
default_port = login->port;
found = tds_read_conf_section(in, server, tds_parse_conf_section, login);
+ if (login->invalid_configuration)
+ return 0;
/*
* If both instance and port are specified and neither one came
from the default, it's an error
@@ -428,7 +430,7 @@ static const struct {
};
int
-tds_config_boolean(const char *value)
+tds_config_boolean(const char *value, TDSLOGIN *login)
{
int p;
@@ -436,7 +438,7 @@ tds_config_boolean(const char *value)
if (!strcasecmp(value, boolean_values[p].value))
return boolean_values[p].to_return;
}
- tdsdump_log(TDS_DBG_INFO1, "UNRECOGNIZED boolean value: '%s'.
Treating as 'no'.\n", value);
+ login->invalid_configuration = 1;
return 0;
}
@@ -451,8 +453,10 @@ tds_config_encryption(const char * value, TDSLOGIN * login)
lvl = TDS_ENCRYPTION_REQUEST;
else if (!strcasecmp(value, TDS_STR_ENCRYPTION_REQUIRE))
lvl = TDS_ENCRYPTION_REQUIRE;
- else
+ else {
tdsdump_log(TDS_DBG_INFO1, "UNRECOGNIZED option value
'%s'...ignoring.\n", value);
+ login->invalid_configuration = 1;
+ }
login->encryption_level = lvl;
}
@@ -568,10 +572,10 @@ tds_parse_conf_section(const char *option, const
char *value, void *param)
if (val >= 512 && val < 65536)
login->block_size = val;
} else if (!strcmp(option, TDS_STR_SWAPDT)) {
- login->broken_dates = tds_config_boolean(value);
+ login->broken_dates = tds_config_boolean(value, login);
} else if (!strcmp(option, TDS_GSSAPI_DELEGATION)) {
/* gssapi flag addition */
- login->gssapi_use_delegation = tds_config_boolean(value);
+ login->gssapi_use_delegation = tds_config_boolean(value, login);
} else if (!strcmp(option, TDS_STR_DUMPFILE)) {
tds_dstr_copy(&login->dump_file, value);
} else if (!strcmp(option, TDS_STR_DEBUGFLAGS)) {
@@ -604,7 +608,7 @@ tds_parse_conf_section(const char *option, const
char *value, void *param)
if (atoi(value))
login->port = atoi(value);
} else if (!strcmp(option, TDS_STR_EMUL_LE)) {
- login->emul_little_endian = tds_config_boolean(value);
+ login->emul_little_endian = tds_config_boolean(value, login);
} else if (!strcmp(option, TDS_STR_TEXTSZ)) {
if (atoi(value))
login->text_size = atoi(value);
@@ -617,7 +621,7 @@ tds_parse_conf_section(const char *option, const
char *value, void *param)
} else if (!strcmp(option, TDS_STR_LANGUAGE)) {
tds_dstr_copy(&login->language, value);
} else if (!strcmp(option, TDS_STR_APPENDMODE)) {
- tds_g_append_mode = tds_config_boolean(value);
+ tds_g_append_mode = tds_config_boolean(value, login);
} else if (!strcmp(option, TDS_STR_INSTANCE)) {
tds_dstr_copy(&login->instance_name, value);
} else if (!strcmp(option, TDS_STR_ENCRYPTION)) {
@@ -625,9 +629,9 @@ tds_parse_conf_section(const char *option, const
char *value, void *param)
} else if (!strcmp(option, TDS_STR_ASA_DATABASE)) {
tds_dstr_copy(&login->server_name, value);
} else if (!strcmp(option, TDS_STR_USENTLMV2)) {
- login->use_ntlmv2 = tds_config_boolean(value);
+ login->use_ntlmv2 = tds_config_boolean(value, login);
} else if (!strcmp(option, TDS_STR_USELANMAN)) {
- login->use_lanman = tds_config_boolean(value);
+ login->use_lanman = tds_config_boolean(value, login);
} else if (!strcmp(option, TDS_STR_REALM)) {
tds_dstr_copy(&login->server_realm_name, value);
} else if (!strcmp(option, TDS_STR_SPN)) {
diff --git a/src/tds/login.c b/src/tds/login.c
index ec8eb5a..813290b 100644
--- a/src/tds/login.c
+++ b/src/tds/login.c
@@ -333,6 +333,9 @@ tds_connect(TDSSOCKET * tds, TDSLOGIN * login, int *p_oserr)
, 0x402
};
+ if (login->invalid_configuration)
+ return TDS_FAIL;
+
if (TDS_MAJOR(login) == 0) {
unsigned int i;
TDSSAVECONTEXT save_ctx;
--
1.7.9.5
Post by James K. Lowden
Post by Frediano Ziglio
I like tdserror. I don't like printf or an additional option. Printf
cause it
Post by Frediano Ziglio
can go nowhere or in dangerous places, an additional option cause it
force
Post by James K. Lowden
Post by Frediano Ziglio
different users to recompile the same library with different options
based on
Post by Frediano Ziglio
the usage. Using options to deal with debugging or different system
configuration is fine but this option would be ok for an application but
not
Post by Frediano Ziglio
for a library used in different places.
What I would do, also suggested by James is something strong. If you
pass
Post by James K. Lowden
a
Post by Frediano Ziglio
wrong option just try to notify upper layer (as tdserror does) and do
not
Post by James K. Lowden
Post by Frediano Ziglio
connect! Surely user will fix the configuration! Of course should be
documented in the version changelog and UG. I'll try to write a partial,
not
Post by Frediano Ziglio
that tested patch. Does it sound reasonable?
I had another branch where I did something like that actually. I just
cleaned it up and made another merge request.
https://gitorious.org/freetds/freetds/merge_requests/18
If that resembles or is useful towards implementing what you suggested,
feel free to use it, or if it's different, you can go ahead and do your
own
Post by James K. Lowden
thing. The idea certainly sounds reasonable.
On Mon, Oct 21, 2013 at 4:51 PM, Frediano Ziglio <freddy77 at gmail.com>
Post by Frediano Ziglio
I like tdserror. I don't like printf or an additional option. Printf
cause
Post by James K. Lowden
Post by Frediano Ziglio
it can go nowhere or in dangerous places, an additional option cause it
force different users to recompile the same library with different
options
Post by James K. Lowden
Post by Frediano Ziglio
based on the usage. Using options to deal with debugging or different
system configuration is fine but this option would be ok for an
application
Post by James K. Lowden
Post by Frediano Ziglio
but not for a library used in different places.
What I would do, also suggested by James is something strong. If you
pass
Post by James K. Lowden
Post by Frediano Ziglio
a wrong option just try to notify upper layer (as tdserror does) and do
not
Post by James K. Lowden
Post by Frediano Ziglio
connect! Surely user will fix the configuration! Of course should be
documented in the version changelog and UG. I'll try to write a partial,
not that tested patch. Does it sound reasonable?
Ps: we have still to sort out the Travis patch. Does the configuration
file need to be in the main directory? Cannot you change the name? What
happen if another developers want to use Travis too?
Frediano Ziglio
Il giorno 21/ott/2013, alle ore 21:48, Marc Abramowitz <
msabramo at gmail.com>
Post by James K. Lowden
Post by Frediano Ziglio
Post by Marc Abramowitz
That makes sense.
OK, I took another stab at creating a louder notification when the
*https://gitorious.org/freetds/freetds/merge_requests/16* ("Improve
handling of invalid encryption setting")
The goal here is to make it much more noticeable when there is an
invalid
Post by James K. Lowden
Post by Frediano Ziglio
Post by Marc Abramowitz
- calling tdserror with a new error code (TDSECONF = 20214 = "Local
configuration error")
- print a message to stderr in case in case someone is looking there
- exiting the process if the ENABLE_EXIT_ON_INVALID_CONFIG is
defined
Post by James K. Lowden
Post by Frediano Ziglio
Post by Marc Abramowitz
(defined by passing --enable-exit-on-invalid-config to./configure).
On Sat, Oct 19, 2013 at 11:49 AM, James K. Lowden <
jklowden at freetds.org
Post by James K. Lowden
Post by Frediano Ziglio
Post by Marc Abramowitz
Post by James K. Lowden
On Sat, 19 Oct 2013 09:48:12 -0700
Post by Marc Abramowitz
Post by James K. Lowden
Post by Marc Abramowitz
https://gitorious.org/freetds/freetds/merge_requests/10
Ouch indeed. I think the next release of FreeTDS should probably
reject all incorrect configurations, both invalid keywords and
out-of-range values. Diagnosing configuration errors is easier if
the error is obvious, and the remedy is simple.
Freddy pointed out that my merge request wasn't acceptable because
printing to stderr and exit aren't appropriate for a library.
...
Post by Marc Abramowitz
Is there some middle ground - some way of indicating a config error
(esp. a serious config error like an invalid encryption setting)
that
Post by James K. Lowden
Post by Frediano Ziglio
Post by Marc Abramowitz
Post by James K. Lowden
Post by Marc Abramowitz
is very noticeable but doesn't overstep what a library should do?
The Right Way is to hook into the error handler. Add something like
#define SYBECONF 2501 /* local configuration error */
and to the dblib.c::dblib_error_messages table
and invoke that error with tdserror or dbperror.
It's not obvious to me that printing to standard error and exiting is
all that bad, though.
1. logic errors, caused by the programmer getting something wrong.
Examples include off-by-one errors and failure to check for a NULL
pointer. These kinds of problems can and do cause the library to
exit via e.g. assert(3). There is no point in continuing, because
the
Post by James K. Lowden
Post by Frediano Ziglio
Post by Marc Abramowitz
Post by James K. Lowden
program has reached an "impossible" state that the program did not
anticipate and thus cannot handle.
2. runtime errors, caused by invalid user input or dynamic failures
such as network disconnection or the server not responding. These
should always return an error to the user, who is the only person in
a
Post by James K. Lowden
Post by Frediano Ziglio
Post by Marc Abramowitz
Post by James K. Lowden
position to deal with it.
Is an invalid configuration a logic or runtime error? is the
administrator a programmer or a user?
I would say the configuration file is more like program text. It is
static data isolated from the program in such a way that it can be
modified without requiring recompilation. The very first time it is
read -- compiled, if you will -- it will be validated. If it is
invalid, the library exits. Once the configuration is valid, no
amount
Post by James K. Lowden
Post by Frediano Ziglio
Post by Marc Abramowitz
Post by James K. Lowden
of other invalid input will cause it to become invalid.
Printing to stderr is unfortunately not robust, because some
applications will not have that descriptor open. Invoking the error
handler is is more general, but will go unseen if the application
doesn't install and error handler. On the other hand, the problem
can be
Post by James K. Lowden
Post by Frediano Ziglio
Post by Marc Abramowitz
Post by James K. Lowden
instantantly diagnosed by using any command-line tool such as tsql.
So, IMO printing to stderr and exiting is fine, and invoking the
error
Post by James K. Lowden
Post by Frediano Ziglio
Post by Marc Abramowitz
Post by James K. Lowden
handler is in some ways friendlier (perhaps in addition to writing to
stderr). Exiting is OK, too, even if it does force the admin to
clean
Post by James K. Lowden
Post by Frediano Ziglio
Post by Marc Abramowitz
Post by James K. Lowden
up errors that might otherwise be innocuous.
HTH.
--jkl
_______________________________________________
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
2013-10-22 21:50:36 UTC
Permalink
Even gmail seems to dislike text mail...

See https://gitorious.org/freetds/mars-freetds/commits/config_error

I think it would be great to integrate the tdserror stuff so patch
would be more verbose.

Frediano
Post by Marc Abramowitz
That looks fairly similar but better than mine as you put the invalid_flag
in the tds_login struct instead of a global.
If you email me a file with a patch that's based off master, I can try it
out if you'd like. I had trouble copying and pasting the patch because of
line breaks (from Gmail?) and even after fixing those, the patch didn't
seem to apply cleanly. I'd be glad to try it though.
Post by Frediano Ziglio
I wrote something like this
Date: Tue, 22 Oct 2013 21:12:13 +0100
Subject: [PATCH] Simple and probably incomplete patch to check configuration
values.
Signed-off-by: Frediano Ziglio <freddy77 at gmail.com>
---
include/freetds/tds.h | 3 ++-
src/odbc/connectparams.c | 8 ++++----
src/tds/config.c | 22 +++++++++++++---------
src/tds/login.c | 3 +++
4 files changed, 22 insertions(+), 14 deletions(-)
diff --git a/include/freetds/tds.h b/include/freetds/tds.h
index b27fcf6..f4a2621 100644
--- a/include/freetds/tds.h
+++ b/include/freetds/tds.h
@@ -578,6 +578,7 @@ typedef struct tds_login
unsigned int use_ntlmv2:1;
unsigned int use_lanman:1;
unsigned int mars:1;
+ unsigned int invalid_configuration:1;
} TDSLOGIN;
typedef struct tds_locale
@@ -1187,7 +1188,7 @@ const char *tds_addrinfo2str(struct tds_addrinfo
*addr, char *name, int namemax)
TDSRET tds_set_interfaces_file_loc(const char *interfloc);
extern const char STD_DATETIME_FMT[];
-int tds_config_boolean(const char *value);
+int tds_config_boolean(const char *value, TDSLOGIN * login);
TDSLOCALE *tds_get_locale(void);
TDSRET tds_alloc_row(TDSRESULTINFO * res_info);
diff --git a/src/odbc/connectparams.c b/src/odbc/connectparams.c
index b287311..0a910d3 100644
--- a/src/odbc/connectparams.c
+++ b/src/odbc/connectparams.c
@@ -226,12 +226,12 @@ odbc_get_dsn_info(TDS_ERRS *errs, const char
*DSN, TDSLOGIN * login)
if (myGetPrivateProfileString(DSN, odbc_param_ServerSPN, tmp) > 0)
tds_parse_conf_section(TDS_STR_SPN, tmp, login);
- if (myGetPrivateProfileString(DSN, odbc_param_Trusted_Connection,
tmp) > 0 && tds_config_boolean(tmp)) {
+ if (myGetPrivateProfileString(DSN, odbc_param_Trusted_Connection,
tmp) > 0 && tds_config_boolean(tmp, login)) {
tds_dstr_copy(&login->user_name, "");
tds_dstr_copy(&login->password, "");
}
- if (myGetPrivateProfileString(DSN, odbc_param_MARS_Connection,
tmp) > 0 && tds_config_boolean(tmp)) {
+ if (myGetPrivateProfileString(DSN, odbc_param_MARS_Connection,
tmp) > 0 && tds_config_boolean(tmp, login)) {
login->mars = 1;
}
@@ -389,12 +389,12 @@ odbc_parse_connect_string(TDS_ERRS *errs, const
char *connect_string, const char
} else if (CHK_PARAM(ServerSPN)) {
tds_parse_conf_section(TDS_STR_SPN, tds_dstr_cstr(&value), login);
} else if (CHK_PARAM(Trusted_Connection)) {
- trusted = tds_config_boolean(tds_dstr_cstr(&value));
+ trusted = tds_config_boolean(tds_dstr_cstr(&value), login);
tdsdump_log(TDS_DBG_INFO1, "trusted %s -> %d\n",
tds_dstr_cstr(&value), trusted);
num_param = -1;
/* TODO odbc_param_Address field */
} else if (CHK_PARAM(MARS_Connection)) {
- if (tds_config_boolean(tds_dstr_cstr(&value)))
+ if (tds_config_boolean(tds_dstr_cstr(&value), login))
login->mars = 1;
}
diff --git a/src/tds/config.c b/src/tds/config.c
index c4d38e7..41386e5 100644
--- a/src/tds/config.c
+++ b/src/tds/config.c
@@ -399,6 +399,8 @@ tds_read_conf_sections(FILE * in, const char
*server, TDSLOGIN * login)
default_port = login->port;
found = tds_read_conf_section(in, server, tds_parse_conf_section, login);
+ if (login->invalid_configuration)
+ return 0;
/*
* If both instance and port are specified and neither one came
from the default, it's an error
@@ -428,7 +430,7 @@ static const struct {
};
int
-tds_config_boolean(const char *value)
+tds_config_boolean(const char *value, TDSLOGIN *login)
{
int p;
@@ -436,7 +438,7 @@ tds_config_boolean(const char *value)
if (!strcasecmp(value, boolean_values[p].value))
return boolean_values[p].to_return;
}
- tdsdump_log(TDS_DBG_INFO1, "UNRECOGNIZED boolean value: '%s'.
Treating as 'no'.\n", value);
+ login->invalid_configuration = 1;
return 0;
}
@@ -451,8 +453,10 @@ tds_config_encryption(const char * value, TDSLOGIN * login)
lvl = TDS_ENCRYPTION_REQUEST;
else if (!strcasecmp(value, TDS_STR_ENCRYPTION_REQUIRE))
lvl = TDS_ENCRYPTION_REQUIRE;
- else
+ else {
tdsdump_log(TDS_DBG_INFO1, "UNRECOGNIZED option value
'%s'...ignoring.\n", value);
+ login->invalid_configuration = 1;
+ }
login->encryption_level = lvl;
}
@@ -568,10 +572,10 @@ tds_parse_conf_section(const char *option, const
char *value, void *param)
if (val >= 512 && val < 65536)
login->block_size = val;
} else if (!strcmp(option, TDS_STR_SWAPDT)) {
- login->broken_dates = tds_config_boolean(value);
+ login->broken_dates = tds_config_boolean(value, login);
} else if (!strcmp(option, TDS_GSSAPI_DELEGATION)) {
/* gssapi flag addition */
- login->gssapi_use_delegation = tds_config_boolean(value);
+ login->gssapi_use_delegation = tds_config_boolean(value, login);
} else if (!strcmp(option, TDS_STR_DUMPFILE)) {
tds_dstr_copy(&login->dump_file, value);
} else if (!strcmp(option, TDS_STR_DEBUGFLAGS)) {
@@ -604,7 +608,7 @@ tds_parse_conf_section(const char *option, const
char *value, void *param)
if (atoi(value))
login->port = atoi(value);
} else if (!strcmp(option, TDS_STR_EMUL_LE)) {
- login->emul_little_endian = tds_config_boolean(value);
+ login->emul_little_endian = tds_config_boolean(value, login);
} else if (!strcmp(option, TDS_STR_TEXTSZ)) {
if (atoi(value))
login->text_size = atoi(value);
@@ -617,7 +621,7 @@ tds_parse_conf_section(const char *option, const
char *value, void *param)
} else if (!strcmp(option, TDS_STR_LANGUAGE)) {
tds_dstr_copy(&login->language, value);
} else if (!strcmp(option, TDS_STR_APPENDMODE)) {
- tds_g_append_mode = tds_config_boolean(value);
+ tds_g_append_mode = tds_config_boolean(value, login);
} else if (!strcmp(option, TDS_STR_INSTANCE)) {
tds_dstr_copy(&login->instance_name, value);
} else if (!strcmp(option, TDS_STR_ENCRYPTION)) {
@@ -625,9 +629,9 @@ tds_parse_conf_section(const char *option, const
char *value, void *param)
} else if (!strcmp(option, TDS_STR_ASA_DATABASE)) {
tds_dstr_copy(&login->server_name, value);
} else if (!strcmp(option, TDS_STR_USENTLMV2)) {
- login->use_ntlmv2 = tds_config_boolean(value);
+ login->use_ntlmv2 = tds_config_boolean(value, login);
} else if (!strcmp(option, TDS_STR_USELANMAN)) {
- login->use_lanman = tds_config_boolean(value);
+ login->use_lanman = tds_config_boolean(value, login);
} else if (!strcmp(option, TDS_STR_REALM)) {
tds_dstr_copy(&login->server_realm_name, value);
} else if (!strcmp(option, TDS_STR_SPN)) {
diff --git a/src/tds/login.c b/src/tds/login.c
index ec8eb5a..813290b 100644
--- a/src/tds/login.c
+++ b/src/tds/login.c
@@ -333,6 +333,9 @@ tds_connect(TDSSOCKET * tds, TDSLOGIN * login, int *p_oserr)
, 0x402
};
+ if (login->invalid_configuration)
+ return TDS_FAIL;
+
if (TDS_MAJOR(login) == 0) {
unsigned int i;
TDSSAVECONTEXT save_ctx;
--
1.7.9.5
Post by James K. Lowden
Post by Frediano Ziglio
I like tdserror. I don't like printf or an additional option. Printf
cause it
Post by Frediano Ziglio
can go nowhere or in dangerous places, an additional option cause it
force
Post by James K. Lowden
Post by Frediano Ziglio
different users to recompile the same library with different options
based on
Post by Frediano Ziglio
the usage. Using options to deal with debugging or different system
configuration is fine but this option would be ok for an application but
not
Post by Frediano Ziglio
for a library used in different places.
What I would do, also suggested by James is something strong. If you
pass
Post by James K. Lowden
a
Post by Frediano Ziglio
wrong option just try to notify upper layer (as tdserror does) and do
not
Post by James K. Lowden
Post by Frediano Ziglio
connect! Surely user will fix the configuration! Of course should be
documented in the version changelog and UG. I'll try to write a partial,
not
Post by Frediano Ziglio
that tested patch. Does it sound reasonable?
I had another branch where I did something like that actually. I just
cleaned it up and made another merge request.
https://gitorious.org/freetds/freetds/merge_requests/18
If that resembles or is useful towards implementing what you suggested,
feel free to use it, or if it's different, you can go ahead and do your
own
Post by James K. Lowden
thing. The idea certainly sounds reasonable.
On Mon, Oct 21, 2013 at 4:51 PM, Frediano Ziglio <freddy77 at gmail.com>
Post by Frediano Ziglio
I like tdserror. I don't like printf or an additional option. Printf
cause
Post by James K. Lowden
Post by Frediano Ziglio
it can go nowhere or in dangerous places, an additional option cause it
force different users to recompile the same library with different
options
Post by James K. Lowden
Post by Frediano Ziglio
based on the usage. Using options to deal with debugging or different
system configuration is fine but this option would be ok for an
application
Post by James K. Lowden
Post by Frediano Ziglio
but not for a library used in different places.
What I would do, also suggested by James is something strong. If you
pass
Post by James K. Lowden
Post by Frediano Ziglio
a wrong option just try to notify upper layer (as tdserror does) and do
not
Post by James K. Lowden
Post by Frediano Ziglio
connect! Surely user will fix the configuration! Of course should be
documented in the version changelog and UG. I'll try to write a partial,
not that tested patch. Does it sound reasonable?
Ps: we have still to sort out the Travis patch. Does the configuration
file need to be in the main directory? Cannot you change the name? What
happen if another developers want to use Travis too?
Frediano Ziglio
Il giorno 21/ott/2013, alle ore 21:48, Marc Abramowitz <
msabramo at gmail.com>
Post by James K. Lowden
Post by Frediano Ziglio
Post by Marc Abramowitz
That makes sense.
OK, I took another stab at creating a louder notification when the
*https://gitorious.org/freetds/freetds/merge_requests/16* ("Improve
handling of invalid encryption setting")
The goal here is to make it much more noticeable when there is an
invalid
Post by James K. Lowden
Post by Frediano Ziglio
Post by Marc Abramowitz
- calling tdserror with a new error code (TDSECONF = 20214 = "Local
configuration error")
- print a message to stderr in case in case someone is looking there
- exiting the process if the ENABLE_EXIT_ON_INVALID_CONFIG is
defined
Post by James K. Lowden
Post by Frediano Ziglio
Post by Marc Abramowitz
(defined by passing --enable-exit-on-invalid-config to./configure).
On Sat, Oct 19, 2013 at 11:49 AM, James K. Lowden <
jklowden at freetds.org
Post by James K. Lowden
Post by Frediano Ziglio
Post by Marc Abramowitz
Post by James K. Lowden
On Sat, 19 Oct 2013 09:48:12 -0700
Post by Marc Abramowitz
Post by James K. Lowden
Post by Marc Abramowitz
https://gitorious.org/freetds/freetds/merge_requests/10
Ouch indeed. I think the next release of FreeTDS should probably
reject all incorrect configurations, both invalid keywords and
out-of-range values. Diagnosing configuration errors is easier if
the error is obvious, and the remedy is simple.
Freddy pointed out that my merge request wasn't acceptable because
printing to stderr and exit aren't appropriate for a library.
...
Post by Marc Abramowitz
Is there some middle ground - some way of indicating a config error
(esp. a serious config error like an invalid encryption setting)
that
Post by James K. Lowden
Post by Frediano Ziglio
Post by Marc Abramowitz
Post by James K. Lowden
Post by Marc Abramowitz
is very noticeable but doesn't overstep what a library should do?
The Right Way is to hook into the error handler. Add something like
#define SYBECONF 2501 /* local configuration error */
and to the dblib.c::dblib_error_messages table
and invoke that error with tdserror or dbperror.
It's not obvious to me that printing to standard error and exiting is
all that bad, though.
1. logic errors, caused by the programmer getting something wrong.
Examples include off-by-one errors and failure to check for a NULL
pointer. These kinds of problems can and do cause the library to
exit via e.g. assert(3). There is no point in continuing, because
the
Post by James K. Lowden
Post by Frediano Ziglio
Post by Marc Abramowitz
Post by James K. Lowden
program has reached an "impossible" state that the program did not
anticipate and thus cannot handle.
2. runtime errors, caused by invalid user input or dynamic failures
such as network disconnection or the server not responding. These
should always return an error to the user, who is the only person in
a
Post by James K. Lowden
Post by Frediano Ziglio
Post by Marc Abramowitz
Post by James K. Lowden
position to deal with it.
Is an invalid configuration a logic or runtime error? is the
administrator a programmer or a user?
I would say the configuration file is more like program text. It is
static data isolated from the program in such a way that it can be
modified without requiring recompilation. The very first time it is
read -- compiled, if you will -- it will be validated. If it is
invalid, the library exits. Once the configuration is valid, no
amount
Post by James K. Lowden
Post by Frediano Ziglio
Post by Marc Abramowitz
Post by James K. Lowden
of other invalid input will cause it to become invalid.
Printing to stderr is unfortunately not robust, because some
applications will not have that descriptor open. Invoking the error
handler is is more general, but will go unseen if the application
doesn't install and error handler. On the other hand, the problem
can be
Post by James K. Lowden
Post by Frediano Ziglio
Post by Marc Abramowitz
Post by James K. Lowden
instantantly diagnosed by using any command-line tool such as tsql.
So, IMO printing to stderr and exiting is fine, and invoking the
error
Post by James K. Lowden
Post by Frediano Ziglio
Post by Marc Abramowitz
Post by James K. Lowden
handler is in some ways friendlier (perhaps in addition to writing to
stderr). Exiting is OK, too, even if it does force the admin to
clean
Post by James K. Lowden
Post by Frediano Ziglio
Post by Marc Abramowitz
Post by James K. Lowden
up errors that might otherwise be innocuous.
HTH.
--jkl
_______________________________________________
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
James K. Lowden
2013-10-23 02:54:59 UTC
Permalink
On Tue, 22 Oct 2013 21:13:30 +0100
Post by Frediano Ziglio
+ unsigned int invalid_configuration:1;
Recommend

unsigned int valid_configuration:1;

instead. I don't like missing no double negative flags reset.
Confusing they are when values set be.

--jkl
Marc Abramowitz
2013-10-24 17:00:52 UTC
Permalink
Freddy,

I just submitted a merge request to your config_error branch with James's
suggestion to flip the boolean and a few other tweaks centered on making it
more apparent to the user what went wrong. Check it out when you get a
chance:

https://gitorious.org/freetds/mars-freetds/merge_requests/1

Marc
Post by James K. Lowden
On Tue, 22 Oct 2013 21:13:30 +0100
Post by Frediano Ziglio
+ unsigned int invalid_configuration:1;
Recommend
unsigned int valid_configuration:1;
instead. I don't like missing no double negative flags reset.
Confusing they are when values set be.
--jkl
_______________________________________________
FreeTDS mailing list
FreeTDS at lists.ibiblio.org
http://lists.ibiblio.org/mailman/listinfo/freetds
Frediano Ziglio
2013-11-03 18:20:03 UTC
Permalink
I found some time to merge your request. I just did some additional
style changes (see
https://gitorious.org/freetds/mars-freetds/commits/7da9b46a73ab638776c140a9c4570280b9989f5a).

I would merge but I would prefer a nicer set of changesets or better a
single one.

As you are the main author do you mind squash them all in another
branch and send the request (or the patch if you prefer) ?

I'm actually executing our tests with your changes bu I don't think
there will be problems (if my configuration is ok :) )

Frediano
Post by Marc Abramowitz
Freddy,
I just submitted a merge request to your config_error branch with James's
suggestion to flip the boolean and a few other tweaks centered on making it
more apparent to the user what went wrong. Check it out when you get a
https://gitorious.org/freetds/mars-freetds/merge_requests/1
Marc
Post by James K. Lowden
On Tue, 22 Oct 2013 21:13:30 +0100
Post by Frediano Ziglio
+ unsigned int invalid_configuration:1;
Recommend
unsigned int valid_configuration:1;
instead. I don't like missing no double negative flags reset.
Confusing they are when values set be.
--jkl
_______________________________________________
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
2013-11-10 20:02:32 UTC
Permalink
Hey Freddy,

I just squashed all of our commits for the config error stuff into a merge
request with two commits -- one from you and one from me. This way people
can see from the history that both you and I worked on it.

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

Feel free to merge or if you want it tweaked, let me know.

Marc
http://marc-abramowitz.com/
Post by Frediano Ziglio
I found some time to merge your request. I just did some additional
style changes (see
https://gitorious.org/freetds/mars-freetds/commits/7da9b46a73ab638776c140a9c4570280b9989f5a
).
I would merge but I would prefer a nicer set of changesets or better a
single one.
As you are the main author do you mind squash them all in another
branch and send the request (or the patch if you prefer) ?
I'm actually executing our tests with your changes bu I don't think
there will be problems (if my configuration is ok :) )
Frediano
Post by Marc Abramowitz
Freddy,
I just submitted a merge request to your config_error branch with James's
suggestion to flip the boolean and a few other tweaks centered on making
it
Post by Marc Abramowitz
more apparent to the user what went wrong. Check it out when you get a
https://gitorious.org/freetds/mars-freetds/merge_requests/1
Marc
On Tue, Oct 22, 2013 at 7:54 PM, James K. Lowden <jklowden at freetds.org
Post by James K. Lowden
On Tue, 22 Oct 2013 21:13:30 +0100
Post by Frediano Ziglio
+ unsigned int invalid_configuration:1;
Recommend
unsigned int valid_configuration:1;
instead. I don't like missing no double negative flags reset.
Confusing they are when values set be.
--jkl
_______________________________________________
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
2013-11-12 20:35:31 UTC
Permalink
Merged to master!

Frediano
Post by Marc Abramowitz
Hey Freddy,
I just squashed all of our commits for the config error stuff into a merge
request with two commits -- one from you and one from me. This way people
can see from the history that both you and I worked on it.
https://gitorious.org/freetds/freetds/merge_requests/19
Feel free to merge or if you want it tweaked, let me know.
Marc
http://marc-abramowitz.com/
Post by Frediano Ziglio
I found some time to merge your request. I just did some additional
style changes (see
https://gitorious.org/freetds/mars-freetds/commits/7da9b46a73ab638776c140a9c4570280b9989f5a
).
I would merge but I would prefer a nicer set of changesets or better a
single one.
As you are the main author do you mind squash them all in another
branch and send the request (or the patch if you prefer) ?
I'm actually executing our tests with your changes bu I don't think
there will be problems (if my configuration is ok :) )
Frediano
Post by Marc Abramowitz
Freddy,
I just submitted a merge request to your config_error branch with James's
suggestion to flip the boolean and a few other tweaks centered on making
it
Post by Marc Abramowitz
more apparent to the user what went wrong. Check it out when you get a
https://gitorious.org/freetds/mars-freetds/merge_requests/1
Marc
On Tue, Oct 22, 2013 at 7:54 PM, James K. Lowden <jklowden at freetds.org
Post by James K. Lowden
On Tue, 22 Oct 2013 21:13:30 +0100
Post by Frediano Ziglio
+ unsigned int invalid_configuration:1;
Recommend
unsigned int valid_configuration:1;
instead. I don't like missing no double negative flags reset.
Confusing they are when values set be.
--jkl
_______________________________________________
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
2013-11-13 21:36:04 UTC
Permalink
Awesome! Thanks!
Post by Frediano Ziglio
Merged to master!
Frediano
Post by Marc Abramowitz
Hey Freddy,
I just squashed all of our commits for the config error stuff into a
merge
Post by Marc Abramowitz
request with two commits -- one from you and one from me. This way people
can see from the history that both you and I worked on it.
https://gitorious.org/freetds/freetds/merge_requests/19
Feel free to merge or if you want it tweaked, let me know.
Marc
http://marc-abramowitz.com/
On Sun, Nov 3, 2013 at 10:20 AM, Frediano Ziglio <freddy77 at gmail.com>
Post by Frediano Ziglio
I found some time to merge your request. I just did some additional
style changes (see
https://gitorious.org/freetds/mars-freetds/commits/7da9b46a73ab638776c140a9c4570280b9989f5a
Post by Marc Abramowitz
Post by Frediano Ziglio
).
I would merge but I would prefer a nicer set of changesets or better a
single one.
As you are the main author do you mind squash them all in another
branch and send the request (or the patch if you prefer) ?
I'm actually executing our tests with your changes bu I don't think
there will be problems (if my configuration is ok :) )
Frediano
Post by Marc Abramowitz
Freddy,
I just submitted a merge request to your config_error branch with
James's
Post by Marc Abramowitz
Post by Frediano Ziglio
Post by Marc Abramowitz
suggestion to flip the boolean and a few other tweaks centered on
making
Post by Marc Abramowitz
Post by Frediano Ziglio
it
Post by Marc Abramowitz
more apparent to the user what went wrong. Check it out when you get a
https://gitorious.org/freetds/mars-freetds/merge_requests/1
Marc
On Tue, Oct 22, 2013 at 7:54 PM, James K. Lowden <
jklowden at freetds.org
Post by Marc Abramowitz
Post by Frediano Ziglio
Post by Marc Abramowitz
Post by James K. Lowden
On Tue, 22 Oct 2013 21:13:30 +0100
Post by Frediano Ziglio
+ unsigned int invalid_configuration:1;
Recommend
unsigned int valid_configuration:1;
instead. I don't like missing no double negative flags reset.
Confusing they are when values set be.
--jkl
_______________________________________________
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
2013-10-19 19:14:52 UTC
Permalink
Just thought of a very easy thing to do that is arguably a tiny improvement on the current behavior:

https://gitorious.org/freetds/freetds/merge_requests/15
("src/tds/config.c: If we see an unknown option value for 'encryption' in config, assume it to be 'required', because that's safer than assuming 'no'.")

I see no harm in doing this and it seems slightly better if FreeTDS is built with OpenSSL or GnuTLS and the user specifies "encryption = hell yes!" and then gets encryption instead of not getting it.
Loading...