FreeTDS Development Group

[Marc Abramowitz <msabramo AT gmail.com>]

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
>>>>>> _______________________________________________
>>>>>> 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
>> _______________________________________________
>> 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