Re: 7.2 fe-exec.c patch to PQescapeString()

I am not sure about this patch. If they pass NULL as string pointers,
but a positive length, I think we should crash rather than assuming
everything is OK. The code already works OK for length = 0. In fact,
the patch makes length=0 do nothing, rather than having it execute this
line:

/* Write the terminating NUL character. */
*target = '\0';

---------------------------------------------------------------------------

Ed Loehr wrote:

This patch makes PQescapeString() guard against null input

ptrs and/or length == 0 input. If any of these occur, the

function returns 0.

*** fe-exec.c.orig	Thu Apr  4 16:06:38 2002
--- fe-exec.c	Thu Apr  4 16:07:30 2002
***************
*** 75,76 ****
--- 75,80 ----

+ 	if ( ! to || ! from || ! length ) {
+ 		return 0;
+ 	}
+ 
while (remaining > 0)

---------------------------(end of broadcast)---------------------------
TIP 5: Have you checked our extensive FAQ?

http://www.postgresql.org/users-lounge/docs/faq.html

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Bruce Momjian wrote:

I am not sure about this patch. If they pass NULL as string pointers,
but a positive length, I think we should crash rather than assuming
everything is OK. The code already works OK for length = 0. In fact,
the patch makes length=0 do nothing, rather than having it execute this
line:

/* Write the terminating NUL character. */
*target = '\0';

Good catch on the length = 0 case. If length is 0 we should still assume
'to' satisfies (2 * length + 1) and allow the target write.

I still think it ought not to crash on null 'from' string with a positive
length if there is sufficient definition of the function to expect the caller
to recover. Absence of a crash does not imply everything is OK, just as the
absence of a backend crash on a malformed query does not imply the query
succeeded. It is the caller's responsibility (and privilege) to check the
return value. If they don't do that, they get what they deserve. But if you
crash, you take that ability away from the caller and nuke possibilities for
error handling and recovery. In the case of length > 0 and 'from' == NULL,
returning 0 rather than crashing gives the caller a chance to check the input
length against the return. So I would change the patch to:

if ( length > 0 && ! from ) { return 0; }

Ed

Our general libpq behavior is not to mask programming errors in the
library, i.e. masking a clearly invalid NULL pointer. What if the
caller doesn't check the returned length?

---------------------------------------------------------------------------

Ed Loehr wrote:

Bruce Momjian wrote:

I am not sure about this patch. If they pass NULL as string pointers,
but a positive length, I think we should crash rather than assuming
everything is OK. The code already works OK for length = 0. In fact,
the patch makes length=0 do nothing, rather than having it execute this
line:

/* Write the terminating NUL character. */
*target = '\0';

Good catch on the length = 0 case. If length is 0 we should still assume
'to' satisfies (2 * length + 1) and allow the target write.

I still think it ought not to crash on null 'from' string with a positive
length if there is sufficient definition of the function to expect the caller
to recover. Absence of a crash does not imply everything is OK, just as the
absence of a backend crash on a malformed query does not imply the query
succeeded. It is the caller's responsibility (and privilege) to check the
return value. If they don't do that, they get what they deserve. But if you
crash, you take that ability away from the caller and nuke possibilities for
error handling and recovery. In the case of length > 0 and 'from' == NULL,
returning 0 rather than crashing gives the caller a chance to check the input
length against the return. So I would change the patch to:

if ( length > 0 && ! from ) { return 0; }

Ed

---------------------------------------------------------------------------

Ed Loehr wrote:

This patch makes PQescapeString() guard against null input

ptrs and/or length == 0 input. If any of these occur, the

function returns 0.

*** fe-exec.c.orig	Thu Apr  4 16:06:38 2002
--- fe-exec.c	Thu Apr  4 16:07:30 2002
***************
*** 75,76 ****
--- 75,80 ----

+ 	if ( ! to || ! from || ! length ) {
+ 		return 0;
+ 	}
+ 
while (remaining > 0)

---------------------------(end of broadcast)---------------------------
TIP 5: Have you checked our extensive FAQ?

http://www.postgresql.org/users-lounge/docs/faq.html

---------------------------(end of broadcast)---------------------------
TIP 6: Have you searched our list archives?

http://archives.postgresql.org

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Bruce Momjian wrote:

Our general libpq behavior is not to mask programming errors in the
library, i.e. masking a clearly invalid NULL pointer. What if the
caller doesn't check the returned length?

They get occasional unexpected results? Your point is well-taken

regarding masking programming errors, but guarding against a null

ptr crash is not 'masking the error' when the function interface

provides a means to communicate the error to the caller as in this

case. Even if it didn't, I still have no affection for low-level

functions that decide to abort the entire system rather than giving

their callers a chance for error-handling.

Ed

Ed Loehr <pgpatches@bluepolka.net> writes:

They get occasional unexpected results? Your point is well-taken
regarding masking programming errors, but guarding against a null
ptr crash is not 'masking the error'

I agree with Bruce on this one. I think the right analogy is not
one of "let's be friendly if he passes a null pointer" but "should
we try to detect a bogus input pointer". If we are passed a random
bit-pattern for the 'from' pointer, we will almost certainly core
dump on trying to dereference it. We have no reasonable or portable
way to defend against that. I tend to think that being passed a null
pointer is a member of this class of events, not something that we
should have a special-case defense against. It is a caller bug and
the caller should fix it, just the same as if the caller passed us
a bogus non-null pointer.

regards, tom lane

Tom Lane wrote:

I agree with Bruce on this one. I think the right analogy is not
one of "let's be friendly if he passes a null pointer" but "should
we try to detect a bogus input pointer". If we are passed a random
bit-pattern for the 'from' pointer, we will almost certainly core
dump on trying to dereference it. We have no reasonable or portable
way to defend against that. I tend to think that being passed a null
pointer is a member of this class of events, not something that we
should have a special-case defense against. It is a caller bug and
the caller should fix it, just the same as if the caller passed us
a bogus non-null pointer.

Well, I can see your perspective and it sounds reasonable. Null ptrs are a
member of the general class called "bogus input pointers." But the fact that
you *can* detect a null ptr while you *cannot* detect a random bit pattern is
precisely why I think it ought not to be sub-classified in the same
things-we-defend-against category as the random bit pattern. You *do* have a
reasonable and portable way to defend against null, unlike the random bit
pattern.

Ed

Ed Loehr wrote:

Tom Lane wrote:

I agree with Bruce on this one. I think the right analogy is not
one of "let's be friendly if he passes a null pointer" but "should
we try to detect a bogus input pointer". If we are passed a random
bit-pattern for the 'from' pointer, we will almost certainly core
dump on trying to dereference it. We have no reasonable or portable
way to defend against that. I tend to think that being passed a null
pointer is a member of this class of events, not something that we
should have a special-case defense against. It is a caller bug and
the caller should fix it, just the same as if the caller passed us
a bogus non-null pointer.

Well, I can see your perspective and it sounds reasonable. Null ptrs are a
member of the general class called "bogus input pointers." But the fact that
you *can* detect a null ptr while you *cannot* detect a random bit pattern is
precisely why I think it ought not to be sub-classified in the same
things-we-defend-against category as the random bit pattern. You *do* have a
reasonable and portable way to defend against null, unlike the random bit
pattern.

Basically, if there is a problem with a libpq parameter, and it is
possible the client code will not detect the failure, I would rather
crash in the libpq routine than to pass back a value.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026