Re: [PATCHES] tsearch2 memory alloc checks
On Sat, 27 Sep 2003, Bruce Momjian wrote: > > This has been saved for the 7.5 release: > > http:/momjian.postgresql.org/cgi-bin/pgpatches2 Are you sure Bruce? I'd say this is bug fixing. Although it doesn't fix the underlying cause of the fault it does cover the specific fault generated and reported by someone. Athough that was running in a 7.3 server I believe, this addresses the same fault in the 7.4 contrib. > > ----------- > > Nigel J. Andrews wrote: > > > > This should apply cleanly to cvs tip. > > > > I've not changed any malloc/calloc to palloc. It looks to me that these memory > > areas are for the lifetime of the backend and in the interests of not breaking > > something that's not broken I left alone. > > > > Note for anyone reading this and wanting it for tsearch-v2-stable (i.e. for 7.3 > > backend) this patch probably will not apply cleanly to that source. It should > > be simple enough to see what's going on and apply the changes by hand if need > > be. ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faqs/FAQ.html
[PATCHES] tsearch2 memory alloc checks
This should apply cleanly to cvs tip.
I've not changed any malloc/calloc to palloc. It looks to me that these memory
areas are for the lifetime of the backend and in the interests of not breaking
something that's not broken I left alone.
Note for anyone reading this and wanting it for tsearch-v2-stable (i.e. for 7.3
backend) this patch probably will not apply cleanly to that source. It should
be simple enough to see what's going on and apply the changes by hand if need
be.
--
Nigel J. Andrews
Index: contrib/tsearch2/ts_cfg.c
===
RCS file: /projects/cvsroot/pgsql-server/contrib/tsearch2/ts_cfg.c,v
retrieving revision 1.3
diff -c -r1.3 ts_cfg.c
*** contrib/tsearch2/ts_cfg.c 4 Aug 2003 00:43:11 - 1.3
--- contrib/tsearch2/ts_cfg.c 23 Sep 2003 23:43:44 -
***
*** 112,117
--- 110,118
cfg->map[lexid].len = ARRNELEMS(a);
cfg->map[lexid].dict_id = (Datum *) malloc(sizeof(Datum) *
cfg->map[lexid].len);
+ if (!cfg->map[lexid].dict_id)
+ ts_error(ERROR, "No memory");
+
memset(cfg->map[lexid].dict_id, 0, sizeof(Datum) *
cfg->map[lexid].len);
ptr = (text *) ARR_DATA_PTR(a);
oldcontext = MemoryContextSwitchTo(TopMemoryContext);
Index: contrib/tsearch2/snowball/api.c
===
RCS file: /projects/cvsroot/pgsql-server/contrib/tsearch2/snowball/api.c,v
retrieving revision 1.3
diff -c -r1.3 api.c
*** contrib/tsearch2/snowball/api.c 4 Aug 2003 00:43:11 - 1.3
--- contrib/tsearch2/snowball/api.c 23 Sep 2003 23:43:55 -
***
*** 6,44
SN_create_env(int S_size, int I_size, int B_size)
{
struct SN_env *z = (struct SN_env *) calloc(1, sizeof(struct SN_env));
z->p = create_s();
! if (S_size)
{
! z->S = (symbol * *) calloc(S_size, sizeof(symbol *));
{
int i;
for (i = 0; i < S_size; i++)
! z->S[i] = create_s();
}
! z->S_size = S_size;
}
! if (I_size)
{
z->I = (int *) calloc(I_size, sizeof(int));
! z->I_size = I_size;
}
! if (B_size)
{
z->B = (symbol *) calloc(B_size, sizeof(symbol));
! z->B_size = B_size;
}
return z;
}
extern void
SN_close_env(struct SN_env * z)
{
! if (z->S_size)
{
{
int i;
--- 6,68
SN_create_env(int S_size, int I_size, int B_size)
{
struct SN_env *z = (struct SN_env *) calloc(1, sizeof(struct SN_env));
+ struct SN_env *z2 = z;
+
+ if (!z)
+ return z;
z->p = create_s();
! if (!z->p)
! z = NULL;
!
! if (z && S_size)
{
! if ((z->S = (symbol * *) calloc(S_size, sizeof(symbol *
{
int i;
for (i = 0; i < S_size; i++)
! {
! if (!(z->S[i] = create_s()))
! {
! z = NULL;
! break;
! }
! }
! z2->S_size = i;
}
! else
! z = NULL;
}
! if (z && I_size)
{
z->I = (int *) calloc(I_size, sizeof(int));
! if (z->I)
! z->I_size = I_size;
! else
! z = NULL;
}
! if (z && B_size)
{
z->B = (symbol *) calloc(B_size, sizeof(symbol));
! if (z->B)
! z->B_size = B_size;
! else
! z = NULL;
}
+ if (!z)
+ SN_close_env(z2);
+
return z;
}
extern void
SN_close_env(struct SN_env * z)
{
! if (z->S && z->S_size)
{
{
int i;
Index: contrib/tsearch2/snowball/utilities.c
===
RCS file: /projects/cvsroot/pgsql-server/contrib/tsearch2/snowball/utilities.c,v
retrieving revision 1.3
diff -c -r1.3 utilities.c
*** contrib/tsearch2/snowball/utilities.c 8 Aug 2003 21:41:24 - 1.3
--- contrib/tsearch2/snowball/utilities.c 23 Sep 2003 23:43:57 -
***
*** 14,19
--- 14,21 ---
Re: [PATCHES] [GENERAL] backend crashing despite tsearch2 patch
I'll withdraw this particular patch and resubmit later on. (Sorry for the individual reply Tom, I hit the wrong keys before noticing) Nigel On Thu, 18 Sep 2003, Nigel J. Andrews wrote: > On Thu, 18 Sep 2003, Tom Lane wrote: > > > "Nigel J. Andrews" <[EMAIL PROTECTED]> writes: > > > On a matter of style, it's been a while since I've seriously considered cross > > > platform C. Is it the done thing to expect: > > > int *i = (int *)calloc(1,sizeof(int)); > > > to give the condition *i == 0 (assuming the memory allocation worked)? > > > > calloc is defined to zero out the block of memory it returns (as opposed > > to malloc which may return a block containing any random junk). > > I was thinking more of any odd cpu that might be around which, for some strange > reason, doesn't read, for example 32 bits of zero as an integer of zero. > Obviously it's probably taking paranoid programming to an extreme and in the > real world probably not worth worrying about but there's always a chance. > > > > > A more serious question is whether any of this code should be using > > calloc/malloc as opposed to palloc. I'd prefer to see it rewritten to > > use palloc wherever possible; but that begs the question of what the > > required lifespan of the allocations is. > > I wasn't sure of the life time needed and in the interests of not making > changes that broke a workign arrangment I left it not using palloc. > > > > > + #ifndef NULL > > + #define NULL ((void *)0) > > + #endif > > > > It has been roughly twenty years since a C platform existed that didn't > > predefine NULL ... and the ones that did not would likely not recognize > > the ANSI-C-ism "void *". So the above is unhelpful by any measure. > > Fair point. I didn't include postgres.h on purpose, again to avoid introducing > new things that broke an existing working arrangement. Obviously I didn't pay > too much attention to the portability of what I did put in. > > I'll take another look at this tonight, along with the formating style used, > which I see has been made more normal looking in cvs head. > > ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] [GENERAL] backend crashing despite tsearch2 patch
On Wed, 17 Sep 2003, Nigel J. Andrews wrote: > Also, I've not tested the amended code since I don't know that much about the > configuration of tsearch2 and specifically what this snowball stuff > is. However, it builds for me and a test query didn't break. I'd appreciate if > someone could give the changes a quick once over for correctness and if someone > could actually test this (maybe [EMAIL PROTECTED]). In the meantime I'll > see if I can get the regression test to run. The installcheck passes. -- Nigel J. Andrews ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send "unregister YourEmailAddressHere" to [EMAIL PROTECTED])
Re: [PATCHES] [GENERAL] backend crashing despite tsearch2 patch
This replaces the previous patch in this thread.
On Wed, 17 Sep 2003, Nigel J. Andrews wrote:
>
> On Wed, 17 Sep 2003 [EMAIL PROTECTED] wrote:
>
> > > On Wed, 17 Sep 2003 [EMAIL PROTECTED] wrote:
> > >
> > > > #0 SN_create_env (S_size=0, I_size=2, B_size=1) at api.c:6
> > > > 6 z->p = create_s();
> > > > (gdb) bt
> > > > #0 SN_create_env (S_size=0, I_size=2, B_size=1) at api.c:6
> > > > #1 0x226be870 in SN_create_env (S_size=40770504, I_size=
> > > > 40509856,
> > > > B_size=1034) at api.c:6
> > >
> ...
> Having said that the z in the z->p = create_s() line mentioned as the place of
> the fault is the result of a calloc without checking for a null return from
> calloc. Here's a[nother simple] patch to fix that.
>
> It's not going to fix whatever is putting you into the situation that makes
> calloc fail though. It'll just make the failure less disasterous.
Here's a slightly more paranoid patch, i.e. it checks all the allocations done
in the routine instead of just the specific instance from the stack trace the
previous patch checked.
On a matter of style, it's been a while since I've seriously considered cross
platform C. Is it the done thing to expect:
int *i = (int *)calloc(1,sizeof(int));
to give the condition *i == 0 (assuming the memory allocation worked)?
Also, I've not tested the amended code since I don't know that much about the
configuration of tsearch2 and specifically what this snowball stuff
is. However, it builds for me and a test query didn't break. I'd appreciate if
someone could give the changes a quick once over for correctness and if someone
could actually test this (maybe [EMAIL PROTECTED]). In the meantime I'll
see if I can get the regression test to run.
--
Nigel J. Andrews
Only in /tmp/postgresql-7.3.4/contrib/tsearch2/snowball: SUBSYS.o
diff -rc tsearch2/snowball/api.c /tmp/postgresql-7.3.4/contrib/tsearch2/snowball/api.c
*** tsearch2/snowball/api.c Mon Jul 7 15:29:46 2003
--- /tmp/postgresql-7.3.4/contrib/tsearch2/snowball/api.c Wed Sep 17 23:35:34
2003
***
*** 2,33
#include "header.h"
extern struct SN_env * SN_create_env(int S_size, int I_size, int B_size)
! { struct SN_env * z = (struct SN_env *) calloc(1, sizeof(struct SN_env));
z->p = create_s();
! if (S_size)
! { z->S = (symbol * *) calloc(S_size, sizeof(symbol *));
{ int i;
! for (i = 0; i < S_size; i++) z->S[i] = create_s();
}
! z->S_size = S_size;
}
! if (I_size)
{ z->I = (int *) calloc(I_size, sizeof(int));
! z->I_size = I_size;
}
! if (B_size)
{ z->B = (symbol *) calloc(B_size, sizeof(symbol));
! z->B_size = B_size;
}
return z;
}
extern void SN_close_env(struct SN_env * z)
{
! if (z->S_size)
{
{ int i;
for (i = 0; i < z->S_size; i++) lose_s(z->S[i]);
--- 2,51
#include "header.h"
extern struct SN_env * SN_create_env(int S_size, int I_size, int B_size)
! {
! struct SN_env * z = (struct SN_env *) calloc(1, sizeof(struct SN_env));
!struct SN_env * z2 = z;
! if (!z) return z;
!
z->p = create_s();
! if (!z->p) z = NULL;
!
! if (z && S_size)
! { if ((z->S = (symbol * *) calloc(S_size, sizeof(symbol *
{ int i;
! for (i = 0; i < S_size; i++)
! { if (!(z->S[i] = create_s()))
! { z = NULL;
! break;
! }
! }
! z2->S_size = i;
}
! else
! z = NULL;
}
! if (z && I_size)
{ z->I = (int *) calloc(I_size, sizeof(int));
! if (z->I) z->I_size = I_size;
! else z = NULL;
}
! if (z && B_size)
{ z->B = (symbol *) calloc(B_size, sizeof(symbol));
! if (z->B) z->B_size = B_size;
! else z = NULL;
}
+if (!z)
+SN_close_env(z2);
+
return z;
}
extern void SN_close_env(struct SN_env * z)
{
! if (z->S && z->S_size)
{
{ int i;
for (i = 0; i < z->S_size; i++) lose_s(z->S[i]);
Only in /tmp/postgresql-7.3.4/contrib/tsearch2/snowball: api.o
Only in /tmp/postgresql-7.3.4/contrib/tsearch2/snowball: english_stem.o
diff -rc tsearch2/snowball/header.h
/tmp/postgresql-7.3.4/contrib/tsearch2/snowball/header.h
*** tsearch2/snowball/header.h Mon Jul 7 15:29:46 2003
--- /tmp/postgresql-7.3.4/contrib/tsearch2/snowball/header.hWed Sep 17 23:20:55
2003
***
*** 6,11
--- 6,15
#define MAXINT INT_MAX
#define MININT
Re: [PATCHES] [GENERAL] backend crashing despite tsearch2 patch
On Wed, 17 Sep 2003 [EMAIL PROTECTED] wrote:
> > On Wed, 17 Sep 2003 [EMAIL PROTECTED] wrote:
> >
> > > I have applied the recent tsearch2 patch and recompiled the
> tsearch2
> > > module but I am still experiencing the same backend crashes as I
> > > previously described.
> >
> > I didn't think your problem was the same as mine.
> >
> > > #0 SN_create_env (S_size=0, I_size=2, B_size=1) at api.c:6
> > > 6 z->p = create_s();
> > > (gdb) bt
> > > #0 SN_create_env (S_size=0, I_size=2, B_size=1) at api.c:6
> > > #1 0x226be870 in SN_create_env (S_size=40770504, I_size=
> > > 40509856,
> > > B_size=1034) at api.c:6
> >
> > Is that the full backtrace?
>
> The gdb session above is quoted above start to finish as displayed on
> screen. I'm not very famialiar with gdb so please say if I need to do
> things differently.
>
> So i think it is the full backtrace - i certainly haven't edited
> anything.
Trouble is it doesn't look like a decently deep stack. I would have expected to
see a lot more output from the backtrace.
Having said that the z in the z->p = create_s() line mentioned as the place of
the fault is the result of a calloc without checking for a null return from
calloc. Here's a[nother simple] patch to fix that.
It's not going to fix whatever is putting you into the situation that makes
calloc fail though. It'll just make the failure less disasterous.
--
Nigel J. Andrews
*** tsearch2/snowball/api.c Mon Jul 7 15:29:46 2003
--- /tmp/postgresql-7.3.4/contrib/tsearch2/snowball/api.c Wed Sep 17 22:21:55
2003
***
*** 2,8
#include "header.h"
extern struct SN_env * SN_create_env(int S_size, int I_size, int B_size)
! { struct SN_env * z = (struct SN_env *) calloc(1, sizeof(struct SN_env));
z->p = create_s();
if (S_size)
{ z->S = (symbol * *) calloc(S_size, sizeof(symbol *));
--- 2,10
#include "header.h"
extern struct SN_env * SN_create_env(int S_size, int I_size, int B_size)
! {
! struct SN_env * z = (struct SN_env *) calloc(1, sizeof(struct SN_env));
! if (!z) return z;
z->p = create_s();
if (S_size)
{ z->S = (symbol * *) calloc(S_size, sizeof(symbol *));
---(end of broadcast)---
TIP 2: you can get off all lists at once with the unregister command
(send "unregister YourEmailAddressHere" to [EMAIL PROTECTED])
Re: [PATCHES] tsearch pfree error
On Fri, 12 Sep 2003, Bruce Momjian wrote: > > ... > > I might have assumed the tsearch guys had it handled now that they have > CVS access. Good point. I'd completely forgotten I was also expecting them to pick it up, principally because it needs to be applied to the version for running with a 7.3 server and no one else seems to know how that is being maintained. Maybe Theodor is on holiday. Nigel ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] tsearch pfree error
On Fri, 12 Sep 2003, Bruce Momjian wrote: > > I hadn't seen this --- not sure why. Probably because it was in a rambling thread consisting mostly of posts by myself. Thanks for applying it chaps. Nigel (Sheesh, all this fuss for a one character change :) ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send "unregister YourEmailAddressHere" to [EMAIL PROTECTED])
[PATCHES] tsearch pfree error
I haven't seen a notice that Bruce noticed this patch I sent across general as part of a rambling thread of mine. There again, I'm not exactly up to date with all my email so could easily have missed it. However, in light of the beta3 notice I'm sending it to the correct list now. Fixes simple bug that was causing me problems using tsearch2. -- Nigel Andrews *** /tmp/postgresql-7.3.4/contrib/tsearch2/query.c Thu Aug 28 13:29:12 2003 --- query.c Sun Sep 7 18:53:17 2003 *** *** 837,843 PG_GETARG_DATUM(1) ); ! PG_FREE_IF_COPY(name,1); PG_RETURN_DATUM(res); } --- 837,843 PG_GETARG_DATUM(1) ); ! PG_FREE_IF_COPY(name,0); PG_RETURN_DATUM(res); } ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
