Thread: 7.1 odbc bug & patch
Your name : Dave Bodenstab Your email address : imdave@mcs.net System Configuration --------------------- Architecture (example: Intel Pentium) : (shouldn't matter) Intel x86 Operating System (example: Linux 2.0.26 ELF) : (shouldn't matter) FreeBSD PostgreSQL version (example: PostgreSQL-7.1): PostgreSQL-7.1 Compiler used (example: gcc 2.8.0) : (shouldn't matter) gcc 2.7.2.1 Please enter a FULL description of your problem: ------------------------------------------------ I installed postgres 7.1 with --enable-odbc. I then installed tclodbc (http://sourceforge.net/projects/tclodbc) and libiodbc-2.50.3 (http://www.iodbc.org/dist/libiodbc-2.50.3.tar.gz). I could not get either to work... postgres would not find the global odbcinst.ini file. I traced this to src/interfaces/odbc/gpps.c -- here are the many things I think are wrong: 40 GetPrivateProfileString(char *theSection, /* section name */ : 50 { 51 char buf[MAXPGPATH]; 52 char *ptr = 0; : 63 int j = 0; 64 j = strlen(theIniFileName) + 1; 65 ptr = (char *) getpwuid(getuid()); /* get user info */ 66 if (ptr == NULL) 67 { 68 if (MAXPGPATH - 1 < j) 69 theIniFileName[MAXPGPATH - 1] = '\0'; 70 sprintf(buf, "%s", theIniFileName); 71 } 72 ptr = ((struct passwd *) ptr)->pw_dir; /* get user home dir */ 73 if (ptr == NULL || *ptr == '\0') 74 ptr = "/home"; 75 /* 76 * This doesn't make it so we find an ini file but allows normal 77 * processing to continue further on down. The likelihood is that the 78 * file won't be found and thus the default value will be returned. 79 */ 80 if (MAXPGPATH - 1 < strlen(ptr) + j) 81 { 82 if (MAXPGPATH - 1 < strlen(ptr)) 83 ptr[MAXPGPATH - 1] = '\0'; 84 else 85 theIniFileName[MAXPGPATH - 1 - strlen(ptr)] = '\0'; 86 } 87 sprintf(buf, "%s/%s", ptr, theIniFileName); 88 /* 89 * This code makes it so that a file in the users home dir overrides a 90 * the "default" file as passed in 91 */ 92 aFile = (FILE *) (buf ? fopen(buf, PG_BINARY_R) : NULL); 93 if (!aFile) 94 { 95 sprintf(buf, "%s", theIniFileName); 96 aFile = (FILE *) (buf ? fopen(buf, PG_BINARY_R) : NULL); 97 } + Line 65 sets ptr, line 66 checks for NULL but line 72 dereferences ptr anyway. Then line 73 checks for NULL again!? + Line 87 unconditionally ignores what line 70 did. + Lines 69, 83 and 85 modify the value passed to the function unbeknownst to the calling function. The values could be a static string or might be further used by the calling function. + Truncating the strings might result in a string that accidently matches another, totally non-related file. + Line 96 checks ``buf?'', but buf[] is an array and the test will *always* be true. + The globally installed odbcinst.ini file is never checked. This is why I couldn't get things to work -- it was looking for odbcinst.ini only in my home directory or the current directory. Please describe a way to repeat the problem. Please try to provide a concise reproducible example, if at all possible: ---------------------------------------------------------------------- Run tclodbc and do a ``database db <DSNname>'' where ``DSNname'' is one of the DSN's in /usr/local/etc/odbcinst.ini (or wherever the global ini file is installed.) The result is always the error message that ``one of server,port,database,etc. are missing''. Run libiodbc-2.50.3/samples/odbctest <DSNname>. The command fails to connect to the database and just exits. If you know how this problem might be fixed, list the solution below: --------------------------------------------------------------------- I couldn't find any documentation as to what the correct behavior should be, and the comments aren't any clearer. I changed the code to look for the odbcinst.ini file in 1) $HOME, 2) the current directory and 3) the globally installed file. I also fixed all the problems I mentioned above. Here is the patch, it works for me. Use it any way you want. --- gpps.c 2001/06/06 02:53:05 7.1 +++ gpps.c 2001/06/06 17:23:39 7.1.1.3 @@ -35,6 +35,7 @@ #include <string.h> #include "misc.h" #include "gpps.h" +#include "dlg_specific.h" #ifndef TRUE #define TRUE ((BOOL)1) @@ -44,6 +45,12 @@ #endif +/* + * theIniFileName is searched for in: + * $HOME/theIniFileName + * theIniFileName + * ODBCINST_INI + */ DWORD GetPrivateProfileString(char *theSection, /* section name */ char *theKey, /* search key name */ @@ -68,48 +75,36 @@ size_t aReturnLength = 0; BOOL aSectionFound = FALSE; BOOL aKeyFound = FALSE; - int j = 0; - j = strlen(theIniFileName) + 1; ptr = (char *) getpwuid(getuid()); /* get user info */ - if (ptr == NULL) - { - if (MAXPGPATH - 1 < j) - theIniFileName[MAXPGPATH - 1] = '\0'; - - sprintf(buf, "%s", theIniFileName); - } - ptr = ((struct passwd *) ptr)->pw_dir; /* get user home dir */ - if (ptr == NULL || *ptr == '\0') + if (ptr == NULL || (((struct passwd *) ptr)->pw_dir) == NULL || *(((struct passwd *) ptr)->pw_dir) == '\0') ptr = "/home"; + else + ptr = ((struct passwd *) ptr)->pw_dir; /* get user home dir */ /* - * This doesn't make it so we find an ini file but allows normal - * processing to continue further on down. The likelihood is that the - * file won't be found and thus the default value will be returned. + * If it can't be opened because the paths are too long, then + * skip it, don't just truncate the path string... The truncated path + * might accidently be an existing file. The default value will be + * returned instead. */ - if (MAXPGPATH - 1 < strlen(ptr) + j) + if (MAXPGPATH - 1 >= strlen(ptr) + 1 + strlen(theIniFileName)) { - if (MAXPGPATH - 1 < strlen(ptr)) - ptr[MAXPGPATH - 1] = '\0'; - else - theIniFileName[MAXPGPATH - 1 - strlen(ptr)] = '\0'; + sprintf(buf, "%s/%s", ptr, theIniFileName); + aFile = (FILE *) fopen(buf, PG_BINARY_R); } - sprintf(buf, "%s/%s", ptr, theIniFileName); - /* * This code makes it so that a file in the users home dir overrides a * the "default" file as passed in */ - aFile = (FILE *) (buf ? fopen(buf, PG_BINARY_R) : NULL); if (!aFile) { - sprintf(buf, "%s", theIniFileName); - aFile = (FILE *) (buf ? fopen(buf, PG_BINARY_R) : NULL); + aFile = (FILE *) fopen(theIniFileName, PG_BINARY_R); + if (!aFile) + aFile = (FILE *) fopen(ODBCINST_INI, PG_BINARY_R); } - aLength = (theDefault == NULL) ? 0 : strlen(theDefault);
Wow, if all our code was as bad as this ODBC snipped we be in serious trouble. :-) > > Your name : Dave Bodenstab > Your email address : imdave@mcs.net > > > System Configuration > --------------------- > Architecture (example: Intel Pentium) : (shouldn't matter) Intel x86 > > Operating System (example: Linux 2.0.26 ELF) : (shouldn't matter) FreeBSD > > PostgreSQL version (example: PostgreSQL-7.1): PostgreSQL-7.1 > > Compiler used (example: gcc 2.8.0) : (shouldn't matter) gcc 2.7.2.1 > > > > Please enter a FULL description of your problem: > ------------------------------------------------ > I installed postgres 7.1 with --enable-odbc. I then installed > tclodbc (http://sourceforge.net/projects/tclodbc) and libiodbc-2.50.3 > (http://www.iodbc.org/dist/libiodbc-2.50.3.tar.gz). I could not > get either to work... postgres would not find the global odbcinst.ini > file. I traced this to src/interfaces/odbc/gpps.c -- here are the > many things I think are wrong: > > 40 GetPrivateProfileString(char *theSection, /* section name */ > : > 50 { > 51 char buf[MAXPGPATH]; > 52 char *ptr = 0; > : > 63 int j = 0; > > 64 j = strlen(theIniFileName) + 1; > 65 ptr = (char *) getpwuid(getuid()); /* get user info */ > > 66 if (ptr == NULL) > 67 { > 68 if (MAXPGPATH - 1 < j) > 69 theIniFileName[MAXPGPATH - 1] = '\0'; > > 70 sprintf(buf, "%s", theIniFileName); > 71 } > 72 ptr = ((struct passwd *) ptr)->pw_dir; /* get user home dir */ > 73 if (ptr == NULL || *ptr == '\0') > 74 ptr = "/home"; > > 75 /* > 76 * This doesn't make it so we find an ini file but allows normal > 77 * processing to continue further on down. The likelihood is that the > 78 * file won't be found and thus the default value will be returned. > 79 */ > 80 if (MAXPGPATH - 1 < strlen(ptr) + j) > 81 { > 82 if (MAXPGPATH - 1 < strlen(ptr)) > 83 ptr[MAXPGPATH - 1] = '\0'; > 84 else > 85 theIniFileName[MAXPGPATH - 1 - strlen(ptr)] = '\0'; > 86 } > > 87 sprintf(buf, "%s/%s", ptr, theIniFileName); > > 88 /* > 89 * This code makes it so that a file in the users home dir overrides a > 90 * the "default" file as passed in > 91 */ > 92 aFile = (FILE *) (buf ? fopen(buf, PG_BINARY_R) : NULL); > 93 if (!aFile) > 94 { > 95 sprintf(buf, "%s", theIniFileName); > 96 aFile = (FILE *) (buf ? fopen(buf, PG_BINARY_R) : NULL); > 97 } > > + Line 65 sets ptr, line 66 checks for NULL but line 72 dereferences > ptr anyway. Then line 73 checks for NULL again!? > + Line 87 unconditionally ignores what line 70 did. > + Lines 69, 83 and 85 modify the value passed to the function unbeknownst > to the calling function. The values could be a static string or might > be further used by the calling function. > + Truncating the strings might result in a string that accidently matches > another, totally non-related file. > + Line 96 checks ``buf?'', but buf[] is an array and the test will *always* > be true. > + The globally installed odbcinst.ini file is never checked. This is > why I couldn't get things to work -- it was looking for odbcinst.ini > only in my home directory or the current directory. > > > > Please describe a way to repeat the problem. Please try to provide a > concise reproducible example, if at all possible: > ---------------------------------------------------------------------- > Run tclodbc and do a ``database db <DSNname>'' where ``DSNname'' is > one of the DSN's in /usr/local/etc/odbcinst.ini (or wherever the > global ini file is installed.) The result is always the error > message that ``one of server,port,database,etc. are missing''. > > Run libiodbc-2.50.3/samples/odbctest <DSNname>. The command fails > to connect to the database and just exits. > > > > If you know how this problem might be fixed, list the solution below: > --------------------------------------------------------------------- > I couldn't find any documentation as to what the correct behavior should > be, and the comments aren't any clearer. I changed the code to look for > the odbcinst.ini file in 1) $HOME, 2) the current directory and 3) the > globally installed file. I also fixed all the problems I mentioned > above. Here is the patch, it works for me. Use it any way you want. > > --- gpps.c 2001/06/06 02:53:05 7.1 > +++ gpps.c 2001/06/06 17:23:39 7.1.1.3 > @@ -35,6 +35,7 @@ > #include <string.h> > #include "misc.h" > #include "gpps.h" > +#include "dlg_specific.h" > > #ifndef TRUE > #define TRUE ((BOOL)1) > @@ -44,6 +45,12 @@ > #endif > > > +/* > + * theIniFileName is searched for in: > + * $HOME/theIniFileName > + * theIniFileName > + * ODBCINST_INI > + */ > DWORD > GetPrivateProfileString(char *theSection, /* section name */ > char *theKey, /* search key name */ > @@ -68,48 +75,36 @@ > size_t aReturnLength = 0; > BOOL aSectionFound = FALSE; > BOOL aKeyFound = FALSE; > - int j = 0; > > - j = strlen(theIniFileName) + 1; > ptr = (char *) getpwuid(getuid()); /* get user info */ > > - if (ptr == NULL) > - { > - if (MAXPGPATH - 1 < j) > - theIniFileName[MAXPGPATH - 1] = '\0'; > - > - sprintf(buf, "%s", theIniFileName); > - } > - ptr = ((struct passwd *) ptr)->pw_dir; /* get user home dir */ > - if (ptr == NULL || *ptr == '\0') > + if (ptr == NULL || (((struct passwd *) ptr)->pw_dir) == NULL || *(((struct passwd *) ptr)->pw_dir) == '\0') > ptr = "/home"; > + else > + ptr = ((struct passwd *) ptr)->pw_dir; /* get user home dir */ > > /* > - * This doesn't make it so we find an ini file but allows normal > - * processing to continue further on down. The likelihood is that the > - * file won't be found and thus the default value will be returned. > + * If it can't be opened because the paths are too long, then > + * skip it, don't just truncate the path string... The truncated path > + * might accidently be an existing file. The default value will be > + * returned instead. > */ > - if (MAXPGPATH - 1 < strlen(ptr) + j) > + if (MAXPGPATH - 1 >= strlen(ptr) + 1 + strlen(theIniFileName)) > { > - if (MAXPGPATH - 1 < strlen(ptr)) > - ptr[MAXPGPATH - 1] = '\0'; > - else > - theIniFileName[MAXPGPATH - 1 - strlen(ptr)] = '\0'; > + sprintf(buf, "%s/%s", ptr, theIniFileName); > + aFile = (FILE *) fopen(buf, PG_BINARY_R); > } > > - sprintf(buf, "%s/%s", ptr, theIniFileName); > - > /* > * This code makes it so that a file in the users home dir overrides a > * the "default" file as passed in > */ > - aFile = (FILE *) (buf ? fopen(buf, PG_BINARY_R) : NULL); > if (!aFile) > { > - sprintf(buf, "%s", theIniFileName); > - aFile = (FILE *) (buf ? fopen(buf, PG_BINARY_R) : NULL); > + aFile = (FILE *) fopen(theIniFileName, PG_BINARY_R); > + if (!aFile) > + aFile = (FILE *) fopen(ODBCINST_INI, PG_BINARY_R); > } > - > > aLength = (theDefault == NULL) ? 0 : strlen(theDefault); > > > > > > ---------------------------(end of broadcast)--------------------------- > TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Your patch has been added to the PostgreSQL unapplied patches list at:
http://candle.pha.pa.us/cgi-bin/pgpatches
I will try to apply it within the next 48 hours.
>
> Your name : Dave Bodenstab
> Your email address : imdave@mcs.net
>
>
> System Configuration
> ---------------------
> Architecture (example: Intel Pentium) : (shouldn't matter) Intel x86
>
> Operating System (example: Linux 2.0.26 ELF) : (shouldn't matter) FreeBSD
>
> PostgreSQL version (example: PostgreSQL-7.1): PostgreSQL-7.1
>
> Compiler used (example: gcc 2.8.0) : (shouldn't matter) gcc 2.7.2.1
>
>
>
> Please enter a FULL description of your problem:
> ------------------------------------------------
> I installed postgres 7.1 with --enable-odbc. I then installed
> tclodbc (http://sourceforge.net/projects/tclodbc) and libiodbc-2.50.3
> (http://www.iodbc.org/dist/libiodbc-2.50.3.tar.gz). I could not
> get either to work... postgres would not find the global odbcinst.ini
> file. I traced this to src/interfaces/odbc/gpps.c -- here are the
> many things I think are wrong:
>
> 40 GetPrivateProfileString(char *theSection, /* section name */
> :
> 50 {
> 51 char buf[MAXPGPATH];
> 52 char *ptr = 0;
> :
> 63 int j = 0;
>
> 64 j = strlen(theIniFileName) + 1;
> 65 ptr = (char *) getpwuid(getuid()); /* get user info */
>
> 66 if (ptr == NULL)
> 67 {
> 68 if (MAXPGPATH - 1 < j)
> 69 theIniFileName[MAXPGPATH - 1] = '\0';
>
> 70 sprintf(buf, "%s", theIniFileName);
> 71 }
> 72 ptr = ((struct passwd *) ptr)->pw_dir; /* get user home dir */
> 73 if (ptr == NULL || *ptr == '\0')
> 74 ptr = "/home";
>
> 75 /*
> 76 * This doesn't make it so we find an ini file but allows normal
> 77 * processing to continue further on down. The likelihood is that the
> 78 * file won't be found and thus the default value will be returned.
> 79 */
> 80 if (MAXPGPATH - 1 < strlen(ptr) + j)
> 81 {
> 82 if (MAXPGPATH - 1 < strlen(ptr))
> 83 ptr[MAXPGPATH - 1] = '\0';
> 84 else
> 85 theIniFileName[MAXPGPATH - 1 - strlen(ptr)] = '\0';
> 86 }
>
> 87 sprintf(buf, "%s/%s", ptr, theIniFileName);
>
> 88 /*
> 89 * This code makes it so that a file in the users home dir overrides a
> 90 * the "default" file as passed in
> 91 */
> 92 aFile = (FILE *) (buf ? fopen(buf, PG_BINARY_R) : NULL);
> 93 if (!aFile)
> 94 {
> 95 sprintf(buf, "%s", theIniFileName);
> 96 aFile = (FILE *) (buf ? fopen(buf, PG_BINARY_R) : NULL);
> 97 }
>
> + Line 65 sets ptr, line 66 checks for NULL but line 72 dereferences
> ptr anyway. Then line 73 checks for NULL again!?
> + Line 87 unconditionally ignores what line 70 did.
> + Lines 69, 83 and 85 modify the value passed to the function unbeknownst
> to the calling function. The values could be a static string or might
> be further used by the calling function.
> + Truncating the strings might result in a string that accidently matches
> another, totally non-related file.
> + Line 96 checks ``buf?'', but buf[] is an array and the test will *always*
> be true.
> + The globally installed odbcinst.ini file is never checked. This is
> why I couldn't get things to work -- it was looking for odbcinst.ini
> only in my home directory or the current directory.
>
>
>
> Please describe a way to repeat the problem. Please try to provide a
> concise reproducible example, if at all possible:
> ----------------------------------------------------------------------
> Run tclodbc and do a ``database db <DSNname>'' where ``DSNname'' is
> one of the DSN's in /usr/local/etc/odbcinst.ini (or wherever the
> global ini file is installed.) The result is always the error
> message that ``one of server,port,database,etc. are missing''.
>
> Run libiodbc-2.50.3/samples/odbctest <DSNname>. The command fails
> to connect to the database and just exits.
>
>
>
> If you know how this problem might be fixed, list the solution below:
> ---------------------------------------------------------------------
> I couldn't find any documentation as to what the correct behavior should
> be, and the comments aren't any clearer. I changed the code to look for
> the odbcinst.ini file in 1) $HOME, 2) the current directory and 3) the
> globally installed file. I also fixed all the problems I mentioned
> above. Here is the patch, it works for me. Use it any way you want.
>
> --- gpps.c 2001/06/06 02:53:05 7.1
> +++ gpps.c 2001/06/06 17:23:39 7.1.1.3
> @@ -35,6 +35,7 @@
> #include <string.h>
> #include "misc.h"
> #include "gpps.h"
> +#include "dlg_specific.h"
>
> #ifndef TRUE
> #define TRUE ((BOOL)1)
> @@ -44,6 +45,12 @@
> #endif
>
>
> +/*
> + * theIniFileName is searched for in:
> + * $HOME/theIniFileName
> + * theIniFileName
> + * ODBCINST_INI
> + */
> DWORD
> GetPrivateProfileString(char *theSection, /* section name */
> char *theKey, /* search key name */
> @@ -68,48 +75,36 @@
> size_t aReturnLength = 0;
> BOOL aSectionFound = FALSE;
> BOOL aKeyFound = FALSE;
> - int j = 0;
>
> - j = strlen(theIniFileName) + 1;
> ptr = (char *) getpwuid(getuid()); /* get user info */
>
> - if (ptr == NULL)
> - {
> - if (MAXPGPATH - 1 < j)
> - theIniFileName[MAXPGPATH - 1] = '\0';
> -
> - sprintf(buf, "%s", theIniFileName);
> - }
> - ptr = ((struct passwd *) ptr)->pw_dir; /* get user home dir */
> - if (ptr == NULL || *ptr == '\0')
> + if (ptr == NULL || (((struct passwd *) ptr)->pw_dir) == NULL || *(((struct passwd *) ptr)->pw_dir) == '\0')
> ptr = "/home";
> + else
> + ptr = ((struct passwd *) ptr)->pw_dir; /* get user home dir */
>
> /*
> - * This doesn't make it so we find an ini file but allows normal
> - * processing to continue further on down. The likelihood is that the
> - * file won't be found and thus the default value will be returned.
> + * If it can't be opened because the paths are too long, then
> + * skip it, don't just truncate the path string... The truncated path
> + * might accidently be an existing file. The default value will be
> + * returned instead.
> */
> - if (MAXPGPATH - 1 < strlen(ptr) + j)
> + if (MAXPGPATH - 1 >= strlen(ptr) + 1 + strlen(theIniFileName))
> {
> - if (MAXPGPATH - 1 < strlen(ptr))
> - ptr[MAXPGPATH - 1] = '\0';
> - else
> - theIniFileName[MAXPGPATH - 1 - strlen(ptr)] = '\0';
> + sprintf(buf, "%s/%s", ptr, theIniFileName);
> + aFile = (FILE *) fopen(buf, PG_BINARY_R);
> }
>
> - sprintf(buf, "%s/%s", ptr, theIniFileName);
> -
> /*
> * This code makes it so that a file in the users home dir overrides a
> * the "default" file as passed in
> */
> - aFile = (FILE *) (buf ? fopen(buf, PG_BINARY_R) : NULL);
> if (!aFile)
> {
> - sprintf(buf, "%s", theIniFileName);
> - aFile = (FILE *) (buf ? fopen(buf, PG_BINARY_R) : NULL);
> + aFile = (FILE *) fopen(theIniFileName, PG_BINARY_R);
> + if (!aFile)
> + aFile = (FILE *) fopen(ODBCINST_INI, PG_BINARY_R);
> }
> -
>
> aLength = (theDefault == NULL) ? 0 : strlen(theDefault);
>
>
>
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org
>
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026
> -----Original Message----- > From: Bruce Momjian > > Wow, if all our code was as bad as this ODBC snipped we be in serious > trouble. :-) > Probably gpps.c has caused ODBC troubles under unix( gpps.c isn't needed at all under Windows). I'm happy if someone maintains it and would reply to the inquiries about ODBC under unix. [snip] > > > > Please enter a FULL description of your problem: > > ------------------------------------------------ > > I installed postgres 7.1 with --enable-odbc. I then installed > > tclodbc (http://sourceforge.net/projects/tclodbc) and libiodbc-2.50.3 > > (http://www.iodbc.org/dist/libiodbc-2.50.3.tar.gz). I could not > > get either to work... postgres would not find the global odbcinst.ini > > file. AFAIK iODBC doesn't need odbcinst.ini though I don't know about tclodbc. In addtion psqlodbc searches odbcinst.ini in a directory. regards, Hiroshi Inoue
Thanks. Patch applied. > > Your name : Dave Bodenstab > Your email address : imdave@mcs.net > > > System Configuration > --------------------- > Architecture (example: Intel Pentium) : (shouldn't matter) Intel x86 > > Operating System (example: Linux 2.0.26 ELF) : (shouldn't matter) FreeBSD > > PostgreSQL version (example: PostgreSQL-7.1): PostgreSQL-7.1 > > Compiler used (example: gcc 2.8.0) : (shouldn't matter) gcc 2.7.2.1 > > > > Please enter a FULL description of your problem: > ------------------------------------------------ > I installed postgres 7.1 with --enable-odbc. I then installed > tclodbc (http://sourceforge.net/projects/tclodbc) and libiodbc-2.50.3 > (http://www.iodbc.org/dist/libiodbc-2.50.3.tar.gz). I could not > get either to work... postgres would not find the global odbcinst.ini > file. I traced this to src/interfaces/odbc/gpps.c -- here are the > many things I think are wrong: > > 40 GetPrivateProfileString(char *theSection, /* section name */ > : > 50 { > 51 char buf[MAXPGPATH]; > 52 char *ptr = 0; > : > 63 int j = 0; > > 64 j = strlen(theIniFileName) + 1; > 65 ptr = (char *) getpwuid(getuid()); /* get user info */ > > 66 if (ptr == NULL) > 67 { > 68 if (MAXPGPATH - 1 < j) > 69 theIniFileName[MAXPGPATH - 1] = '\0'; > > 70 sprintf(buf, "%s", theIniFileName); > 71 } > 72 ptr = ((struct passwd *) ptr)->pw_dir; /* get user home dir */ > 73 if (ptr == NULL || *ptr == '\0') > 74 ptr = "/home"; > > 75 /* > 76 * This doesn't make it so we find an ini file but allows normal > 77 * processing to continue further on down. The likelihood is that the > 78 * file won't be found and thus the default value will be returned. > 79 */ > 80 if (MAXPGPATH - 1 < strlen(ptr) + j) > 81 { > 82 if (MAXPGPATH - 1 < strlen(ptr)) > 83 ptr[MAXPGPATH - 1] = '\0'; > 84 else > 85 theIniFileName[MAXPGPATH - 1 - strlen(ptr)] = '\0'; > 86 } > > 87 sprintf(buf, "%s/%s", ptr, theIniFileName); > > 88 /* > 89 * This code makes it so that a file in the users home dir overrides a > 90 * the "default" file as passed in > 91 */ > 92 aFile = (FILE *) (buf ? fopen(buf, PG_BINARY_R) : NULL); > 93 if (!aFile) > 94 { > 95 sprintf(buf, "%s", theIniFileName); > 96 aFile = (FILE *) (buf ? fopen(buf, PG_BINARY_R) : NULL); > 97 } > > + Line 65 sets ptr, line 66 checks for NULL but line 72 dereferences > ptr anyway. Then line 73 checks for NULL again!? > + Line 87 unconditionally ignores what line 70 did. > + Lines 69, 83 and 85 modify the value passed to the function unbeknownst > to the calling function. The values could be a static string or might > be further used by the calling function. > + Truncating the strings might result in a string that accidently matches > another, totally non-related file. > + Line 96 checks ``buf?'', but buf[] is an array and the test will *always* > be true. > + The globally installed odbcinst.ini file is never checked. This is > why I couldn't get things to work -- it was looking for odbcinst.ini > only in my home directory or the current directory. > > > > Please describe a way to repeat the problem. Please try to provide a > concise reproducible example, if at all possible: > ---------------------------------------------------------------------- > Run tclodbc and do a ``database db <DSNname>'' where ``DSNname'' is > one of the DSN's in /usr/local/etc/odbcinst.ini (or wherever the > global ini file is installed.) The result is always the error > message that ``one of server,port,database,etc. are missing''. > > Run libiodbc-2.50.3/samples/odbctest <DSNname>. The command fails > to connect to the database and just exits. > > > > If you know how this problem might be fixed, list the solution below: > --------------------------------------------------------------------- > I couldn't find any documentation as to what the correct behavior should > be, and the comments aren't any clearer. I changed the code to look for > the odbcinst.ini file in 1) $HOME, 2) the current directory and 3) the > globally installed file. I also fixed all the problems I mentioned > above. Here is the patch, it works for me. Use it any way you want. > > --- gpps.c 2001/06/06 02:53:05 7.1 > +++ gpps.c 2001/06/06 17:23:39 7.1.1.3 > @@ -35,6 +35,7 @@ > #include <string.h> > #include "misc.h" > #include "gpps.h" > +#include "dlg_specific.h" > > #ifndef TRUE > #define TRUE ((BOOL)1) > @@ -44,6 +45,12 @@ > #endif > > > +/* > + * theIniFileName is searched for in: > + * $HOME/theIniFileName > + * theIniFileName > + * ODBCINST_INI > + */ > DWORD > GetPrivateProfileString(char *theSection, /* section name */ > char *theKey, /* search key name */ > @@ -68,48 +75,36 @@ > size_t aReturnLength = 0; > BOOL aSectionFound = FALSE; > BOOL aKeyFound = FALSE; > - int j = 0; > > - j = strlen(theIniFileName) + 1; > ptr = (char *) getpwuid(getuid()); /* get user info */ > > - if (ptr == NULL) > - { > - if (MAXPGPATH - 1 < j) > - theIniFileName[MAXPGPATH - 1] = '\0'; > - > - sprintf(buf, "%s", theIniFileName); > - } > - ptr = ((struct passwd *) ptr)->pw_dir; /* get user home dir */ > - if (ptr == NULL || *ptr == '\0') > + if (ptr == NULL || (((struct passwd *) ptr)->pw_dir) == NULL || *(((struct passwd *) ptr)->pw_dir) == '\0') > ptr = "/home"; > + else > + ptr = ((struct passwd *) ptr)->pw_dir; /* get user home dir */ > > /* > - * This doesn't make it so we find an ini file but allows normal > - * processing to continue further on down. The likelihood is that the > - * file won't be found and thus the default value will be returned. > + * If it can't be opened because the paths are too long, then > + * skip it, don't just truncate the path string... The truncated path > + * might accidently be an existing file. The default value will be > + * returned instead. > */ > - if (MAXPGPATH - 1 < strlen(ptr) + j) > + if (MAXPGPATH - 1 >= strlen(ptr) + 1 + strlen(theIniFileName)) > { > - if (MAXPGPATH - 1 < strlen(ptr)) > - ptr[MAXPGPATH - 1] = '\0'; > - else > - theIniFileName[MAXPGPATH - 1 - strlen(ptr)] = '\0'; > + sprintf(buf, "%s/%s", ptr, theIniFileName); > + aFile = (FILE *) fopen(buf, PG_BINARY_R); > } > > - sprintf(buf, "%s/%s", ptr, theIniFileName); > - > /* > * This code makes it so that a file in the users home dir overrides a > * the "default" file as passed in > */ > - aFile = (FILE *) (buf ? fopen(buf, PG_BINARY_R) : NULL); > if (!aFile) > { > - sprintf(buf, "%s", theIniFileName); > - aFile = (FILE *) (buf ? fopen(buf, PG_BINARY_R) : NULL); > + aFile = (FILE *) fopen(theIniFileName, PG_BINARY_R); > + if (!aFile) > + aFile = (FILE *) fopen(ODBCINST_INI, PG_BINARY_R); > } > - > > aLength = (theDefault == NULL) ? 0 : strlen(theDefault); > > > > > > ---------------------------(end of broadcast)--------------------------- > TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
> > -----Original Message----- > > From: Bruce Momjian > > > > Wow, if all our code was as bad as this ODBC snipped we be in serious > > trouble. :-) > > > > Probably gpps.c has caused ODBC troubles under unix( > gpps.c isn't needed at all under Windows). I'm happy if > someone maintains it and would reply to the inquiries > about ODBC under unix. Thanks for the info. I will apply the patch now. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Bruce Momjian wrote: > > > > -----Original Message----- > > > From: Bruce Momjian > > > > > > Wow, if all our code was as bad as this ODBC snipped we be in serious > > > trouble. :-) > > > > > > > Probably gpps.c has caused ODBC troubles under unix( > > gpps.c isn't needed at all under Windows). I'm happy if > > someone maintains it and would reply to the inquiries > > about ODBC under unix. > > Thanks for the info. I will apply the patch now. > Hmm I don't think the patch is proper. Someone must maintain ODBC stuff under unix. regards, Hiroshi Inoue
> Bruce Momjian wrote: > > > > > > -----Original Message----- > > > > From: Bruce Momjian > > > > > > > > Wow, if all our code was as bad as this ODBC snipped we be in serious > > > > trouble. :-) > > > > > > > > > > Probably gpps.c has caused ODBC troubles under unix( > > > gpps.c isn't needed at all under Windows). I'm happy if > > > someone maintains it and would reply to the inquiries > > > about ODBC under unix. > > > > Thanks for the info. I will apply the patch now. > > > > Hmm I don't think the patch is proper. > Someone must maintain ODBC stuff under unix. Oh, I thought you liked it. Is the new better than the old? I assumed it was. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Bruce Momjian wrote: > > > Bruce Momjian wrote: > > > > > > > > -----Original Message----- > > > > > From: Bruce Momjian > > > > > > > > > > Wow, if all our code was as bad as this ODBC snipped we be in serious > > > > > trouble. :-) > > > > > > > > > > > > > Probably gpps.c has caused ODBC troubles under unix( > > > > gpps.c isn't needed at all under Windows). I'm happy if > > > > someone maintains it and would reply to the inquiries > > > > about ODBC under unix. > > > > > > Thanks for the info. I will apply the patch now. > > > > > > > Hmm I don't think the patch is proper. > > Someone must maintain ODBC stuff under unix. > > Oh, I thought you liked it. Is the new better than the old? Partly maybe. But the handling of odbcinst.ini seems to be different from its original intension. What I expect is that someone who knows well about iODBC rewrites gpps.c from the first. I don't know if there's such person in pg community. regards, Hiroshi Inoue
> > > Hmm I don't think the patch is proper. > > > Someone must maintain ODBC stuff under unix. > > > > Oh, I thought you liked it. Is the new better than the old? > > Partly maybe. But the handling of odbcinst.ini seems > to be different from its original intension. > > What I expect is that someone who knows well about iODBC > rewrites gpps.c from the first. I don't know if there's > such person in pg community. Yes, certainly a rewrite would be ideal, but when I get a patch that seems to make things marginally better, I apply it. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Dave Bodenstab wrote: > > As long as I'm on the cc list here... let me add $.02 > > What little documentation I found was in: > > postgresql-7.1/doc/postgres/odbc-install.html > > In section 7.2 Installation, it states: > > The installation-wide configuration file odbcinst.ini will be > installed into the directory /usr/local/pgsql/etc/, or equivalent, > depending on what --prefix and/or --sysconfdir options you supplied > to configure. Since this file can also be shared between different > ODBC drivers you can also install it in a shared location. To do > that, override the location of this file with the --with-odbcinst > option. > Could someone tell me where the above doc come from ? Certainly both windows-ODBC and unixODBC need odbcinst.ini but I couldn't find *odbcinst* in any iODBC doc/src. In fact psqlodbc driver works with iODBC without odbcinst.ini. I'm not familiar with ODBC under unix and am happy if someone explains it. > This led *me* to believe that when I configure and install postgress > with: > > --enable-odbc \ > --with-odbcinst=/usr/local/etc \ > > then postgress would actually *use* /usr/local/etc/odbcinst.ini. > It didn't. There was no mention of looking in the current directory > or looking in $HOME. When I ktrace'd my test program, I found that > an odbcinst.ini file *was* being searched for, but only in . and > $HOME. Finally tracking it down, I found the reason in gpps.c (and > found questionable and buggy code also.) In fact, the only reference > to the installed odbcinst.ini (ODBCINST_INI) is in dlg_specific.c, > in code that is never called unless WIN32 is #defined, and even if > #defined, it's attempting to *update* the file. No psqlodbc really uses ODBCINST_INI under Windows and probably under unix also. I could see getGlobalDefaults(DBMS_NAME, ODBCINST_INI, FALSE) in psqlodbc.c and environ.c. [snip] > > There are, perhaps, other problems... I don't know. I only played > with this stuff in an attempt to learn about ``odbc'' stuff. When > I ktrace'd libiodbc, it wanted its own "odbc.ini" file. I would > think that they would use the same file name... but since I don't > have any doc's on how things are supposed to work, I have not investigated > further. Yes you are right. There must be a documentation about how to let the psqlodbc driver work with iDOBC though I don't know who could do it. And someone should improve gpps.c. Thanks, Hiroshi Inoue
As long as I'm on the cc list here... let me add $.02 What little documentation I found was in: postgresql-7.1/doc/postgres/odbc-install.html In section 7.2 Installation, it states: The installation-wide configuration file odbcinst.ini will be installed into the directory /usr/local/pgsql/etc/, or equivalent, depending on what --prefix and/or --sysconfdir options you supplied to configure. Since this file can also be shared between different ODBC drivers you can also install it in a shared location. To do that, override the location of this file with the --with-odbcinst option. This led *me* to believe that when I configure and install postgress with: --enable-odbc \ --with-odbcinst=/usr/local/etc \ then postgress would actually *use* /usr/local/etc/odbcinst.ini. It didn't. There was no mention of looking in the current directory or looking in $HOME. When I ktrace'd my test program, I found that an odbcinst.ini file *was* being searched for, but only in . and $HOME. Finally tracking it down, I found the reason in gpps.c (and found questionable and buggy code also.) In fact, the only reference to the installed odbcinst.ini (ODBCINST_INI) is in dlg_specific.c, in code that is never called unless WIN32 is #defined, and even if #defined, it's attempting to *update* the file. It was obvious that the code did not match the available doc's. Since it wasn't obvious whether or not odbcinst.ini should be searched for in . and $HOME, I left that logic alone. I only added a final search for ODBCINST_INI which, finally, makes it do what the documentation states that it should. (The buggy code is another story... that should be removed/fixed regardless of whether the search is augmented.) End of story... the patch got things working. There are, perhaps, other problems... I don't know. I only played with this stuff in an attempt to learn about ``odbc'' stuff. When I ktrace'd libiodbc, it wanted its own "odbc.ini" file. I would think that they would use the same file name... but since I don't have any doc's on how things are supposed to work, I have not investigated further. In any event, both libiodbc and tclodbc require database specific libraries -- for postgres, src/interfaces/odbc/ -- so I would expect that postgres would supply working (with respect to its own documentation) code. Dave Bodenstab