Thread: Re: 7.2 fe-exec.c patch to PQescapeString()
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)
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
>
> ---------------------------------------------------------------------------
>
> 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
>>
>
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 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
>>
>>
>
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