Skip to Content.
Sympa Menu

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

freetds AT lists.ibiblio.org

Subject: FreeTDS Development Group

List archive

Chronological Thread  
  • From: Frediano Ziglio <freddy77 AT gmail.com>
  • To: FreeTDS Development Group <freetds AT lists.ibiblio.org>
  • Subject: [freetds] R: Re: Way to get connection params on failure?
  • Date: Sun, 19 Jan 2014 14:24:52 +0000

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 <msabramo AT gmail.com>
Data: 18/01/2014 18:00 (GMT+00:00)
A: FreeTDS Development Group <freetds AT lists.ibiblio.org>
Cc: FreeTDS Development Group <freetds AT lists.ibiblio.org>
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 <freddy77 AT gmail.com> wrote:

> Committed!
>
> Frediano
>
> 2014/1/18 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
>>>>>>



Archive powered by MHonArc 2.6.24.

Top of Page