Re: [PATCH] pg_escape_literal/pg_escape_identifier for pgsql

On Tue, May 24, 2011 at 06:37, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote: > 2011/5/20 Hannes Magnusson <hannes.magnusson@gmail.com>: >> On Fri, May 20, 2011 at 03:24, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote: >>> Hi all >>> >>> Current PostgreSQL has new escape functions for literals and identifiers. >>> Since there is no function for that, I made patch for that. Attached patch is >>> for trunk and tested with PostgreSQL 8.4 and 9.0. >>> >>> This patch would also applied to 5.4 branch but how about 5.3? >>> It's pain not to have escape functions. >>> >>> Any comments? >> >> This won't compile if HAVE_PQESCAPE isn't declared since you only >> register the arginfo when it is, but use it even if it wasn't. > > I suppose it would be ancient PostgreSQL (6.5?) which would not have PQescape, > I'll fix it anyway. > >> >> There is also very inconsistent usage of space vs tab in the patch. > > For some reason, my emacs ignored the marker(?) > I'll fix it > >> >> +    if (!tmp) { >> +               php_error_docref(NULL TSRMLS_CC, E_WARNING,"Failed to >> escape"); >> +        RETURN_FALSE; >> +    } >> >> Won't this also trigger the error if there was no from passed? >> I suppose checking from_len wouldn't be stupid. > > This error occurs only if libpq could not allocate memory. > Empty string should be valid string for inserting db. So if I pass in an empty string, libpq will throw some escape characters around it? >> Should we really provide fallback if PQescape(Literal|Identifier) >> isn't available? Did you copy the escaping from postgresql directly? > > Wouldn't it nice to have the same escaping capability for 8.x? > It's not possible to copy the code, since it's binded to PostgreSQL's > multibyte function. It's possible to use mbstring function for thatbut it's > not default. I'm just worried about if there is a logic error there then we are creating security issues while pretending that the escaping is perfectly acceptable and working. > Thanks for the comment. > Is there any comment? Go for it -Hannes