Re: 7.1 odbc bug & patch - Mailing list pgsql-patches

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