Re: Way to get connection params on failure?

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
>> >> >
>> >> _______________________________________________
>> >> FreeTDS mailing list
>> >> [email protected]
>> >> http://lists.ibiblio.org/mailman/listinfo/freetds
>> >>
>> > _______________________________________________
>> > FreeTDS mailing list
>> > [email protected]
>> > http://lists.ibiblio.org/mailman/listinfo/freetds
>> _______________________________________________
>> FreeTDS mailing list
>> [email protected]
>> http://lists.ibiblio.org/mailman/listinfo/freetds
>>
> _______________________________________________
> FreeTDS mailing list
> [email protected]
> http://lists.ibiblio.org/mailman/listinfo/freetds