Re: R: Re: Way to get connection params on failure?

Ah ok. Oops. sorry 'bout that!


On Sun, Jan 19, 2014 at 6:24 AM, Frediano Ziglio <[email protected]> wrote:

> Hi,
>   I had to do a commit as your patch caused a crash as dbproc or
> tds_socket could be null
>
> Frediano
>
> -------- Messaggio originale --------
> Da: Marc Abramowitz <[email protected]>
> Data: 18/01/2014  18:00  (GMT+00:00)
> A: FreeTDS Development Group <[email protected]>
> Cc: FreeTDS Development Group <[email protected]>
> Oggetto: Re: [freetds] Way to get connection params on failure?
>
> Cool! Thanks!
>
> -Marc
> http://marc-abramowitz.com
> Sent from my iPhone 4S
>
>
> On Jan 18, 2014, at 5:53 AM, Frediano Ziglio <[email protected]> wrote:
>
> > Committed!
> >
> > Frediano
> >
> > 2014/1/18 Marc Abramowitz <[email protected]>:
> >> 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 <[email protected]>
> 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 <[email protected]>:
> >>>> 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 <[email protected]>
> >>> 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" <[email protected]> ha
> scritto:
> >>>>>
> >>>>>> On Sun, Jan 5, 2014 at 11:32 AM, Frediano Ziglio <
> [email protected]>
> >>>>>> 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
> [email protected]
> http://lists.ibiblio.org/mailman/listinfo/freetds
>