Skip to Content.
Sympa Menu

freetds - Re: Crash in tsql

freetds AT lists.ibiblio.org

Subject: FreeTDS Development Group

List archive

Chronological Thread  
  • From: "Brian Bruns" <camber AT ais.org>
  • To: freetds AT franklin.oit.unc.edu
  • Subject: Re: Crash in tsql
  • Date: Thu, 15 Aug 2002 09:30:45 -0400


> Brian,
>
> I did a clean build from CVS head sources, fails to build due to
> missing tdsiconv.h, I assume thats a new one you built but didn't
> check into CVS yet. (Either that or I still haven't figured out
> cvs properly :)

Ah yes, my bad.

Re: your patch. I was pretty limited on time when I put this in (always a
bad idea), so I had been going down one path, got your patch, and
basically jammed it in. I'm going to revisit it this weekend. Your
observations are correct though.

Brian

> I few comments in support of my iconv changes that were lost in
> the patch tho :)
>
> 1. I think that keeping the implementation details opaque within
> iconv.c and accessing via function calls, rather than by
> exposing through headers, is a "good thing". Its a good way
> to avoid future compatability problems. (No need for a
> tdsiconv.c, for a start!)
> For example if the code in mem.c doesn't know anything about
> the contents of the void pointer to the TDSICONVINFO structure
> then iconv.c can play with the details without anything else
> needing to be changed.
>
> 2. The current code references unchecked malloc()d memory in
> several places. I know this is done throughout the code,
> but I had tidied up this area! [My background in small,
> embedded, limited memory systems makes me shudder at the
> sight of such code :) ]
>
> 3. Current code also reintroduces the possibility of iconv_open()
> without a matching iconv_close() which I had removed. (If the
> first iconv_open succeeds and the second fails. use_iconv is
> only set if BOTH succeed, but tds_iconv_close is only called
> if use_iconv is set. Another example where its better to call
> tds_iconv_close and let it figure out the details :)
>
> Cheers,
>
> David
>
>
> > -----Original Message-----
> > From: Brian Bruns [mailto:camber AT ais.org]
> > Sent: Wednesday, 14 August 2002 8:17 PM
> > To: TDS Development Group
> > Subject: [freetds] Re: Crash in tsql
> >
> >
> >
> > I checked in a fix (hopefully it's a fix ;-) I had had one
> > brewing when I
> > got your patch, but the final was a blend of the two.
> > Someone want to
> > give it a try?
> >
> > Cheers,
> >
> > Brian
> >
> > On Wed, 14 Aug 2002, Varley, David(CBorn at Alcoa) wrote:
> >
> > >
> > > Dave,
> > >
> > > I agree with your proposed solution, in fact I'd suggest taking
> > > it a step further and replacing the entry in the TDSSOCKET structure
> > > with a void pointer for the use of the iconv functions. This enables
> > > us to hide all the iconv details within functions in iconv.c, and
> > > gets the conditional stuff right out of tds.h. The
> > functions themselves
> > > then take the appropriate actions depending whether the
> > system supports
> > > it. The only files needing modification would be tds.h, iconv.c and
> > > mem.c.
> > >
> > > Brian, I'm happy to make those mods if you agree with this.
> > >
> > > David
> > >
> > > > -----Original Message-----
> > > > From: Coleman, Dave [mailto:DColeman AT US.TIAuto.com]
> > > > Sent: Tuesday, 13 August 2002 10:14 PM
> > > > To: TDS Development Group
> > > > Subject: [freetds] Re: Crash in tsql
> > > >
> > > >
> > > > It's a day late, but if you're still looking for suggestions
> > > > to help with
> > > > the ICONV mis-match, I think this might work. What does
> > > > anyone else think?
> > > >
> > > > Set up the ICONV stuff be a small structure pointer in
> > TDSSOCKET.
> > > >
> > > > Something like:
> > > >
> > > > struct tds_iconv_info
> > > > {
> > > > int use_iconv;
> > > > iconv_t cdto;
> > > > iconv_t cdfrom;
> > > > } TDSICONVINFO;
> > > >
> > > > then TDSSOCKET would look something like this
> > > >
> > > > ...
> > > > TDSICONVINFO *tds_iconv;
> > > > char *date_fmt;
> > > > TDSCONTEXT *tds_ctx;
> > > > } TDSSOCKET;
> > > >
> > > > modify tds_alloc_socket(...)
> > > >
> > > > 371: tds_socket->parent = (char*)NULL;
> > > > + tds_socket->tds_iconv = (char*)NULL;
> > > > 372: tds_socket->env = tds_alloc_env(tds_socket);
> > > >
> > > > add a few functions:
> > > >
> > > > TDSICONVINFO* tds_alloc_iconv(TDSSOCKET *tds);
> > > > void tds_free_iconv(TDSSOCKET *tds);
> > > > and
> > > > TDSICONVINFO* tds_geticonv(TDSSOCKET *tds)
> > > > {
> > > > /*
> > > > 1) do some environment stuff to find if can use iconv on the
> > > > system...
> > > > might be sloppy, but keeps ICONV out of TDSSOCKET.
> > > > Someone with a better knowledge of
> > > > configuration stuff than
> > > > myself may know a good way to get this information out
> > > > of the system at run time.
> > > > */
> > > > if( /* has iconv stuff */)
> > > > {
> > > > tds->tds_iconv = tds_alloc_iconv(tds);
> > > > }
> > > > /*
> > > > do what ever else you need to do to set things up
> > > > */
> > > > return tds->tds_iconv;
> > > > }
> > > >
> > > > add a call to tds_free_iconv(...) from tds_free_socket(...)
> > > > and you should
> > > > be done unless I've missed something dumb.
> > > >
> > > > It doesn't solve the root problem of needing to determine if
> > > > a system has
> > > > ICONV support at run-time, but it does allow the TDSSOCKET
> > > > structure (and
> > > > the library) to be independent of ICONV. It could simplify
> > > > the functions
> > > > dealing with it as well. The prototpe would not change at
> > > > all, they would
> > > > still get a TDSSOCKET. Compile the library with the
> > > > tds_iconv[xxx](...)
> > > > functions if the headers exist on the system, and the
> > > > functions can test for
> > > > if ( tds_socket->tds_iconv ) and as long as
> > > > tds_alloc_socket(...) sets the
> > > > tds_iconv pointer to NULL, any tds_iconvblah(...) function
> > > > can fail safely
> > > > if no support is present, or it has not been initialized.
> > This could
> > > > prevent people from trying to use the features if they had
> > > > not been compiled
> > > > into the library without including config.h. The library
> > > > would compile but
> > > > the application would not if it tried to link in the ICONV
> > > > functions on a
> > > > system with an ICONV-less OS or freeTDS distribution. It's
> > > > still a link
> > > > error and not completely clean, but that's a heck of a lot
> > > > better than a
> > > > seg-fault.
> > > >
> > > > Thoughts, comments?
> > > >
> > > > Dave
> > > >
> > > > > -----Original Message-----
> > > > > From: Brian Bruns [SMTP:camber AT ais.org]
> > > > > Sent: Monday, August 12, 2002 10:37 AM
> > > > > To: TDS Development Group
> > > > > Subject: [freetds] Re: Crash in tsql
> > > > >
> > > > > Actually, this fix is bad, since we don't distribute
> > > > config.h (nor should
> > > > > we) things won't build. If we don't do it in the header
> > > > file however, we
> > > > > are in trouble of having mismatched structure definitions.
> > > > We didn't
> > > > > really notice it before because iconv_t cdto, cdfrom was
> > > > the last entry.
> > > > >
> > > > > We simply can't be checking HAVE_ICONV in tds.h (or any
> > > > distributable
> > > > > header). We could substitute from configure but that
> > strikes me as
> > > > > sloppy. Any other options?
> > > > >
> > > > > Brian
> > > > >
> > > > > > Ok, I was on the right track, sizeof(*tds) differed
> > > > between the two, I
> > > > > > just didn't have time to finish it off. config.h should
> > > > be included
> > > > > from
> > > > > > tds.h (tds.h.in) otherwise we'll have this problem again
> > > > in the future.
> > > > > >
> > > > > > Checking the fix in now.
> > > > > >
> > > > > > Brian
> > > > > >
> > > > > > On Sun, 11 Aug 2002 freddyz77 AT tin.it wrote:
> > > > > >
> > > > > > > > This has been the topic of discussion for the past
> > > > couple of days.
> > > > > For
> > > > > > > > some reason tds->tds_ctx is NULL after the call to
> > > > tds_connect(),
> > > > > although
> > > > > > > > it is set within tds_connect(). I haven't had a
> > > > chance to track it
> > > > > any
> > > > > > > > further.
> > > > > > > >
> > > > > > > > Brian
> > > > > > > >
> > > > > > >
> > > > > > > Ok, after a bit debug (assembly level at the end) I
> > > > found the problem.
> > > > > > > You do not include config.h in tsql, so tds_socket
> > > > appear to tsql
> > > > > without
> > > > > > > iconv stuff.
> > > > > > >
> > > > > > > Add "#include <config.h>" on top of tsql.c and it
> > run correctly.
> > > > > > > Perhaps is best to include config.h from tds.h.in ...
> > > > > > >
> > > > > > > freddy77
> > > > > > >
> > > > >
> > > > > ---
> > > > > You are currently subscribed to freetds as:
> > [dColeman AT us.tiauto.com]
> > > > > To unsubscribe, forward this message to
> > > > > $subst('Email.Unsub')
> > > >
> > > > ---
> > > > You are currently subscribed to freetds as:
> > > > [David.Varley AT alcoa.com.au]
> > > > To unsubscribe, forward this message to
> > > > $subst('Email.Unsub')
> > > >
> > >
> > > ---
> > > You are currently subscribed to freetds as: [camber AT ais.org]
> > > To unsubscribe, forward this message to
> > $subst('Email.Unsub')
> > >
> > >
> >
> >
> >
> > ---
> > You are currently subscribed to freetds as:
> > [David.Varley AT alcoa.com.au]
> > To unsubscribe, forward this message to
> > $subst('Email.Unsub')
> >




Archive powered by MHonArc 2.6.24.

Top of Page