FreeTDS Development Group

[Marc Abramowitz <msabramo AT gmail.com>]

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
>