Skip to Content.
Sympa Menu

freetds - Re: [freetds] Way to get connection params on failure?

freetds AT lists.ibiblio.org

Subject: FreeTDS Development Group

List archive

Chronological Thread  
  • From: Marc Abramowitz <msabramo AT gmail.com>
  • To: FreeTDS Development Group <freetds AT lists.ibiblio.org>
  • Subject: Re: [freetds] Way to get connection params on failure?
  • Date: Sat, 18 Jan 2014 04:12:22 -0800

Great comments, thanks!

I updated the merge request and also went ahead and created a squashed
version with a single commit:

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

Let me know if you want other changes.


On Mon, Jan 13, 2014 at 5:22 AM, Frediano Ziglio <freddy77 AT gmail.com> wrote:

> Hi,
> patch is nicer. However is construct_message is already used you get
> a leak as you not free the buffer allocated before (your buffer is
> then freed after calling the error handler). Also are you sure you
> want to add the server name details every time you have a login? Note
> that login field is NULL if you are connected so probably you will
> have a core for errors after the connection! I would also use asprintf
> to make buffer computation easier.
>
> Something like
>
> if (dbproc->tds_socket->login &&
> !tds_dstr_isempty(&dbproc->tds_socket->login->server_name)) {
> char * buffer = NULL;
> if (asprintf(&buffer, "%s (%s)", msg->msgtext,
> tds_dstr_cstr(&dbproc->tds_socket->login->server_name)) >= 0) {
> free((char*) constructed_message.msgtext);
> constructed_message.msgtext = buffer;
> constructed_message.severity = msg->severity;
> msg = &constructed_message;
> }
> }
>
> Frediano
>
> 2014/1/10 Marc Abramowitz <msabramo AT gmail.com>:
> > Hi Freddy,
> >
> > I just updated the merge request so that I don't access the DSTR fields
> > directly.
> >
> > As for the buffer being freed, I also had concerns about that, but I
> think
> > it's okay. You will notice that this code is very similar to the code
> right
> > above it for doing the dynamic parameter substitution -- they both do:
> >
> > ```
> > constructed_message.msgtext = buffer;
> > ```
> >
> > Then later on this happens:
> >
> > ```
> > /* we're done with the dynamic string now. */
> > free((char*) constructed_message.msgtext);
> > ```
> >
> > so that should free the buffer. So I think it's good.
> >
> > I have not looked at libtds. pymssql only uses dblib, so I don't have a
> use
> > case and it would be harder for me to test. Perhaps someone else would be
> > better suited for moving it to libtds?
> >
> > Let me know if other changes are needed or if you want the commits
> > squashed, etc.
> >
> > Cheers,
> > Marc
> >
> >
> > On Thu, Jan 9, 2014 at 3:19 AM, Frediano Ziglio <freddy77 AT gmail.com>
> wrote:
> >
> >> Hi,
> >> Dstr fields should not be acccessed directly. Are you sure buffer is
> >> freed?
> >>
> >> Beside this patch looks good. Perhaps should be moved in libtds so all
> >> layers will use this code.
> >>
> >> About dynamic parameters values should be controlled as are only errors
> >> from our library, not from server
> >>
> >> Frediano
> >> Il 06/gen/2014 19:08 "Marc Abramowitz" <msabramo AT gmail.com> ha scritto:
> >>
> >> > On Sun, Jan 5, 2014 at 11:32 AM, Frediano Ziglio <freddy77 AT gmail.com>
> >> > wrote:
> >> >
> >> > > The problem of your implementation is that TDSECONN is passed by
> >> > > libTDS and not all libraries add the required parameter so you can
> >> > > have a not formatted string in other libraries.
> >> > >
> >> > > Mixing normal string and string with format looks a bit security
> >> suspect
> >> > > to me.
> >> > >
> >> >
> >> > Yeah, that smelled a little funny to me too.
> >> >
> >> > I updated my PR so that dbperror itself appends the server_name, if
> >> > available, to the error message. So no need to pass it into dbperror.
> >> >
> >> > https://gitorious.org/freetds/freetds/merge_requests/24/diffs
> >> >
> >> > Hopefully, that's better.
> >> >
> >> > Marc
> >> > _______________________________________________
> >> > 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
>




Archive powered by MHonArc 2.6.24.

Top of Page