Re: [Monetdb-developers] [Monetdb-checkins] MonetDB5/src/modules/kernel batcast.mx, Feb2010, 1.31.2.7, 1.31.2.8
On Sat, Mar 20, 2010 at 12:53:12PM +0000, Martin Kersten wrote:
Update of /cvsroot/monetdb/MonetDB5/src/modules/kernel In directory sfp-cvsdas-1.v30.ch3.sourceforge.com:/tmp/cvs-serv10537
Modified Files: Tag: Feb2010 batcast.mx Log Message: Backport of possible dangerous double frees. No roll forward needed.
While your revised code looks indeed more efficient to me (as it allows ATOMfromstr() --- well, at least strFromStr(); I cannot tell about other "extern" (varsized) atoms --- to reuse a allocated temporary buffer), I wonder whether/why the original experienced double frees? Called with v==NULL, ATOMfromstr() --- again, I refer only to strFromStr(); FromStr functions for other "extern" (varsized) atoms might behave differently; IMHO, we don't have any "contract" or "policies" for FromStr() implementations --- would allocate a buffer for the result, ATOMput() copies the value from the buffer to the BAT heap, and then the buffer is (was) freed. Looks very sound to me. What am I missing? I would be thankful for any enlightment. Stefan
Index: batcast.mx =================================================================== RCS file: /cvsroot/monetdb/MonetDB5/src/modules/kernel/batcast.mx,v retrieving revision 1.31.2.7 retrieving revision 1.31.2.8 diff -u -d -r1.31.2.7 -r1.31.2.8 --- batcast.mx 18 Feb 2010 01:04:05 -0000 1.31.2.7 +++ batcast.mx 20 Mar 2010 12:53:10 -0000 1.31.2.8 @@ -248,6 +248,8 @@ BAT *b,*bn; BATiter bi; BUN p, q, r = 0; + ptr v = NULL; + int len = 0;
@:getBATdescriptor(bid, b, "batcalc.@1")@ @:voidresultBAT(TYPE_@1,"batcalc.@1")@ @@ -255,15 +257,12 @@
BATaccessBegin(b, USE_TAIL, MMAP_SEQUENTIAL); BATloop(b, p, q) { - ptr v = NULL; - int len = 0; - ATOMfromstr(TYPE_@1, &v, &len, (char *) BUNtvar(bi, p)); ATOMput(TYPE_@1, bn->T->vheap, Tloc(bn, r), v); - if (ATOMextern(TYPE_@1)) - GDKfree(v); r++; } + if (ATOMextern(TYPE_@1)) + GDKfree(v); BATsetcount(bn, BATcount(b)); bunins_failed: BATaccessEnd(b, USE_TAIL, MMAP_SEQUENTIAL);
------------------------------------------------------------------------------ Download Intel® Parallel Studio Eval Try the new software tools for yourself. Speed compiling, find bugs proactively, and fine-tune applications for parallel performance. See why Intel Parallel Studio got high marks during beta. http://p.sf.net/sfu/intel-sw-dev _______________________________________________ Monetdb-checkins mailing list Monetdb-checkins@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/monetdb-checkins
-- | Dr. Stefan Manegold | mailto:Stefan.Manegold@cwi.nl | | CWI, P.O.Box 94079 | http://www.cwi.nl/~manegold/ | | 1090 GB Amsterdam | Tel.: +31 (20) 592-4212 | | The Netherlands | Fax : +31 (20) 592-4199 |
Stefan Manegold wrote:
On Sat, Mar 20, 2010 at 12:53:12PM +0000, Martin Kersten wrote:
Update of /cvsroot/monetdb/MonetDB5/src/modules/kernel In directory sfp-cvsdas-1.v30.ch3.sourceforge.com:/tmp/cvs-serv10537
Modified Files: Tag: Feb2010 batcast.mx Log Message: Backport of possible dangerous double frees. No roll forward needed.
While your revised code looks indeed more efficient to me (as it allows ATOMfromstr() --- well, at least strFromStr(); I cannot tell about other "extern" (varsized) atoms --- to reuse a allocated temporary buffer), I wonder whether/why the original experienced double frees? The implicit contract in gdk_atoms is to use v+len, i.e. pass a pre-alloced buffer and realloc upon need. Just freeing v without resetting it to NULL is bound to give problems with larger values becoming stored.
Setting v=NULL would have been the other option.
Called with v==NULL, ATOMfromstr() --- again, I refer only to strFromStr(); FromStr functions for other "extern" (varsized) atoms might behave differently; IMHO, we don't have any "contract" or "policies" for FromStr() implementations --- would allocate a buffer for the result, ATOMput() copies the value from the buffer to the BAT heap, and then the buffer is (was) freed.
Looks very sound to me.
What am I missing?
I would be thankful for any enlightment.
Stefan
Index: batcast.mx =================================================================== RCS file: /cvsroot/monetdb/MonetDB5/src/modules/kernel/batcast.mx,v retrieving revision 1.31.2.7 retrieving revision 1.31.2.8 diff -u -d -r1.31.2.7 -r1.31.2.8 --- batcast.mx 18 Feb 2010 01:04:05 -0000 1.31.2.7 +++ batcast.mx 20 Mar 2010 12:53:10 -0000 1.31.2.8 @@ -248,6 +248,8 @@ BAT *b,*bn; BATiter bi; BUN p, q, r = 0; + ptr v = NULL; + int len = 0;
@:getBATdescriptor(bid, b, "batcalc.@1")@ @:voidresultBAT(TYPE_@1,"batcalc.@1")@ @@ -255,15 +257,12 @@
BATaccessBegin(b, USE_TAIL, MMAP_SEQUENTIAL); BATloop(b, p, q) { - ptr v = NULL; - int len = 0; - ATOMfromstr(TYPE_@1, &v, &len, (char *) BUNtvar(bi, p)); ATOMput(TYPE_@1, bn->T->vheap, Tloc(bn, r), v); - if (ATOMextern(TYPE_@1)) - GDKfree(v); r++; } + if (ATOMextern(TYPE_@1)) + GDKfree(v); BATsetcount(bn, BATcount(b)); bunins_failed: BATaccessEnd(b, USE_TAIL, MMAP_SEQUENTIAL);
------------------------------------------------------------------------------ Download Intel® Parallel Studio Eval Try the new software tools for yourself. Speed compiling, find bugs proactively, and fine-tune applications for parallel performance. See why Intel Parallel Studio got high marks during beta. http://p.sf.net/sfu/intel-sw-dev _______________________________________________ Monetdb-checkins mailing list Monetdb-checkins@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/monetdb-checkins
On Sat, Mar 20, 2010 at 02:56:11PM +0100, Martin Kersten wrote:
Stefan Manegold wrote:
On Sat, Mar 20, 2010 at 12:53:12PM +0000, Martin Kersten wrote:
Update of /cvsroot/monetdb/MonetDB5/src/modules/kernel In directory sfp-cvsdas-1.v30.ch3.sourceforge.com:/tmp/cvs-serv10537
Modified Files: Tag: Feb2010 batcast.mx Log Message: Backport of possible dangerous double frees. No roll forward needed.
While your revised code looks indeed more efficient to me (as it allows ATOMfromstr() --- well, at least strFromStr(); I cannot tell about other "extern" (varsized) atoms --- to reuse a allocated temporary buffer), I wonder whether/why the original experienced double frees? The implicit contract in gdk_atoms is to use v+len, i.e. pass a pre-alloced buffer and realloc upon need. Just freeing v without resetting it to NULL is bound to give problems with larger values becoming stored.
Setting v=NULL would have been the other option.
v was declared as ptr v = NULL; with in the look body, and hence should have been initialized with each iteration, shouldn't it? Stefan
Called with v==NULL, ATOMfromstr() --- again, I refer only to strFromStr(); FromStr functions for other "extern" (varsized) atoms might behave differently; IMHO, we don't have any "contract" or "policies" for FromStr() implementations --- would allocate a buffer for the result, ATOMput() copies the value from the buffer to the BAT heap, and then the buffer is (was) freed.
Looks very sound to me.
What am I missing?
I would be thankful for any enlightment.
Stefan
Index: batcast.mx =================================================================== RCS file: /cvsroot/monetdb/MonetDB5/src/modules/kernel/batcast.mx,v retrieving revision 1.31.2.7 retrieving revision 1.31.2.8 diff -u -d -r1.31.2.7 -r1.31.2.8 --- batcast.mx 18 Feb 2010 01:04:05 -0000 1.31.2.7 +++ batcast.mx 20 Mar 2010 12:53:10 -0000 1.31.2.8 @@ -248,6 +248,8 @@ BAT *b,*bn; BATiter bi; BUN p, q, r = 0; + ptr v = NULL; + int len = 0;
@:getBATdescriptor(bid, b, "batcalc.@1")@ @:voidresultBAT(TYPE_@1,"batcalc.@1")@ @@ -255,15 +257,12 @@
BATaccessBegin(b, USE_TAIL, MMAP_SEQUENTIAL); BATloop(b, p, q) { - ptr v = NULL; - int len = 0; - ATOMfromstr(TYPE_@1, &v, &len, (char *) BUNtvar(bi, p)); ATOMput(TYPE_@1, bn->T->vheap, Tloc(bn, r), v); - if (ATOMextern(TYPE_@1)) - GDKfree(v); r++; } + if (ATOMextern(TYPE_@1)) + GDKfree(v); BATsetcount(bn, BATcount(b)); bunins_failed: BATaccessEnd(b, USE_TAIL, MMAP_SEQUENTIAL);
------------------------------------------------------------------------------ Download Intel® Parallel Studio Eval Try the new software tools for yourself. Speed compiling, find bugs proactively, and fine-tune applications for parallel performance. See why Intel Parallel Studio got high marks during beta. http://p.sf.net/sfu/intel-sw-dev _______________________________________________ Monetdb-checkins mailing list Monetdb-checkins@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/monetdb-checkins
------------------------------------------------------------------------------ Download Intel® Parallel Studio Eval Try the new software tools for yourself. Speed compiling, find bugs proactively, and fine-tune applications for parallel performance. See why Intel Parallel Studio got high marks during beta. http://p.sf.net/sfu/intel-sw-dev _______________________________________________ Monetdb-checkins mailing list Monetdb-checkins@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/monetdb-checkins
-- | Dr. Stefan Manegold | mailto:Stefan.Manegold@cwi.nl | | CWI, P.O.Box 94079 | http://www.cwi.nl/~manegold/ | | 1090 GB Amsterdam | Tel.: +31 (20) 592-4212 | | The Netherlands | Fax : +31 (20) 592-4199 |
On Sat, Mar 20, 2010 at 02:37:25PM +0100, Stefan Manegold wrote:
On Sat, Mar 20, 2010 at 12:53:12PM +0000, Martin Kersten wrote:
Update of /cvsroot/monetdb/MonetDB5/src/modules/kernel In directory sfp-cvsdas-1.v30.ch3.sourceforge.com:/tmp/cvs-serv10537
Modified Files: Tag: Feb2010 batcast.mx Log Message: Backport of possible dangerous double frees. No roll forward needed.
While your revised code looks indeed more efficient to me (as it allows ATOMfromstr() --- well, at least strFromStr(); I cannot tell about other "extern" (varsized) atoms --- to reuse a allocated temporary buffer), I wonder whether/why the original experienced double frees?
Called with v==NULL, ATOMfromstr() --- again, I refer only to strFromStr(); FromStr functions for other "extern" (varsized) atoms might behave differently; IMHO, we don't have any "contract" or "policies" for FromStr() implementations --- would allocate a buffer for the result,
In fact, also ATOMfromstr() for fixed size ATOMs needs to allocate a buffer for the results, in case it is call with a NULL pointer instead of a pointer to a valid destination (e.g., a value record). Hence, why isn't the buffer then also free for these and not only for "extern" ATOMs? Getting even more puzzled ... Stefan
ATOMput() copies the value from the buffer to the BAT heap, and then the buffer is (was) freed.
Looks very sound to me.
What am I missing?
I would be thankful for any enlightment.
Stefan
Index: batcast.mx =================================================================== RCS file: /cvsroot/monetdb/MonetDB5/src/modules/kernel/batcast.mx,v retrieving revision 1.31.2.7 retrieving revision 1.31.2.8 diff -u -d -r1.31.2.7 -r1.31.2.8 --- batcast.mx 18 Feb 2010 01:04:05 -0000 1.31.2.7 +++ batcast.mx 20 Mar 2010 12:53:10 -0000 1.31.2.8 @@ -248,6 +248,8 @@ BAT *b,*bn; BATiter bi; BUN p, q, r = 0; + ptr v = NULL; + int len = 0;
@:getBATdescriptor(bid, b, "batcalc.@1")@ @:voidresultBAT(TYPE_@1,"batcalc.@1")@ @@ -255,15 +257,12 @@
BATaccessBegin(b, USE_TAIL, MMAP_SEQUENTIAL); BATloop(b, p, q) { - ptr v = NULL; - int len = 0; - ATOMfromstr(TYPE_@1, &v, &len, (char *) BUNtvar(bi, p)); ATOMput(TYPE_@1, bn->T->vheap, Tloc(bn, r), v); - if (ATOMextern(TYPE_@1)) - GDKfree(v); r++; } + if (ATOMextern(TYPE_@1)) + GDKfree(v); BATsetcount(bn, BATcount(b)); bunins_failed: BATaccessEnd(b, USE_TAIL, MMAP_SEQUENTIAL);
------------------------------------------------------------------------------ Download Intel® Parallel Studio Eval Try the new software tools for yourself. Speed compiling, find bugs proactively, and fine-tune applications for parallel performance. See why Intel Parallel Studio got high marks during beta. http://p.sf.net/sfu/intel-sw-dev _______________________________________________ Monetdb-checkins mailing list Monetdb-checkins@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/monetdb-checkins
-- | Dr. Stefan Manegold | mailto:Stefan.Manegold@cwi.nl | | CWI, P.O.Box 94079 | http://www.cwi.nl/~manegold/ | | 1090 GB Amsterdam | Tel.: +31 (20) 592-4212 | | The Netherlands | Fax : +31 (20) 592-4199 |
------------------------------------------------------------------------------ Download Intel® Parallel Studio Eval Try the new software tools for yourself. Speed compiling, find bugs proactively, and fine-tune applications for parallel performance. See why Intel Parallel Studio got high marks during beta. http://p.sf.net/sfu/intel-sw-dev _______________________________________________ Monetdb-checkins mailing list Monetdb-checkins@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/monetdb-checkins
-- | Dr. Stefan Manegold | mailto:Stefan.Manegold@cwi.nl | | CWI, P.O.Box 94079 | http://www.cwi.nl/~manegold/ | | 1090 GB Amsterdam | Tel.: +31 (20) 592-4212 | | The Netherlands | Fax : +31 (20) 592-4199 |
participants (2)
-
Martin Kersten -
Stefan Manegold