Re: [PATCH] mysql_real_escape_string() usage in gw/dlr_mysql.c

Hi,

Under all circumstances, the patch is better than the one I was forced 
to make myself in a hurry when I encountered the problem. I ended up not 
using the real_escape version because I didn't have time to dig into how 
the DB pool was working, and I didn't use the Octstr struct.
This is also why I didn't offered you my own patch. You would probably 
spend more time fixing my patch, than writing your own :D

But it is good to see that fixes are posted this fast. With certain 
other projects, it may take months before you even get a response!

Best regards

Peter Christensen
(ungod in the bug report system)


Stipe Tolj wrote:
> Rene Kluwen wrote:
>>
>> IMHO consuming DBPoolConns is not a bad thing. After all, that is why the
>> pool even exist. So consuming and producing sql connections is not so
>> expensive.
> 
> ok, that's true. The main issue is that in some circumstances you may 
> need 2 connections for "one-and-the-same" job, where 1 would be enought. 
> Think of thread overlays. Now, on the other hand I agree and actually we 
> shouldn't make life inside dlr_mysql too hard in order to pass mysql 
> connection structs to lower function call layers.
> 
>> In your patch: Where exactly do you escpape the parameters to the sql
>> string? I couldnt find that so quick.
> 
> I use mysql_real_escape_string() within gw/dlr_mysql.c:mysql_escape().
> 
>> In most of the Chimit code, we use something like this:
>>
>> Octstr *sql = octstr_sql_format("SELECT * from table where pk = '%s'",
>> PrimaryKey);
>>
>> The function octstr_sql_format is crafted so that before replacing the 
>> value
>> of PrimaryKey on the place that says %s in the format string, 
>> automatically
>> it is sql-escaped.
>> This way, you never have to worry about escaping stuff - everything goes
>> automagically. This a clean and generic solution that generates little
>> coding effort.
> 
> yep, +1 on this approach... but I'd see the octstr_sql_format() renamed 
> to mysql_octstr_format() in order to semantically scope this to the 
> mysql layer and not fitting it to the gwlib/ itself.
> 
> Another thing. It seems to me that you use "only" mysql_escape_string() 
> from the mysql client lib here, right? You don't use 
> mysql_real_escapee_string() which implies that you aren't aware of the 
> charset encoding and still the escaping "could fail". At least most of 
> the significant projects using the mysql client lib changed to call the 
> mysql_real_... function instead of the other one. Even the mysql docs 
> advice this to do.
> 
> The whole dilema about the previously needed DBPoolConn wouldn't be 
> necessary if we "simply" pick the mysql_escape_string() instead of the 
> sophisticated one ;)
> 
> Stipe
> 
> mailto:stolj_{at}_wapme-group.de
> -------------------------------------------------------------------
> Wapme Systems AG
> 
> Vogelsanger Weg 80
> 40470 Düsseldorf, NRW, Germany
> 
> phone: +49.211.74845.0
> fax: +49.211.74845.299
> 
> mailto:info_{at}_wapme-systems.de
> http://www.wapme-systems.de/
> -------------------------------------------------------------------
>