FreeTDS Development Group

[Frediano Ziglio <freddy77 AT gmail.com>]

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

2013/10/22 Marc Abramowitz <msabramo AT gmail.com>:
>> 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?
>
> 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.
>
>
> On Mon, Oct 21, 2013 at 4:51 PM, Frediano Ziglio <freddy77 AT gmail.com> wrote:
>
>> 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>
>> ha scritto:
>>
>> > 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).
>> >
>> >
>> >
>> > On Sat, Oct 19, 2013 at 11:49 AM, James K. Lowden <jklowden AT freetds.org
>> >wrote:
>> >
>> >> On Sat, 19 Oct 2013 09:48:12 -0700
>> >> Marc Abramowitz <msabramo AT gmail.com> wrote:
>> >>
>> >>>>> Merge request to make an invalid encryption setting a fatal error:
>> >>>>> 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.
>> >> ...
>> >>> 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
>> >> _______________________________________________
>> >> 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