Merge lp:~vkolesnikov/pbxt/pbxt-heap-corruption into lp:pbxt

Proposed by Vladimir Kolesnikov
Status: Merged
Merged at revision: not available
Proposed branch: lp:~vkolesnikov/pbxt/pbxt-heap-corruption
Merge into: lp:pbxt
Diff against target: None lines
To merge this branch: bzr merge lp:~vkolesnikov/pbxt/pbxt-heap-corruption
Reviewer Review Type Date Requested Status
PBXT Core Pending
Review via email: mp+6828@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Vladimir Kolesnikov (vkolesnikov) wrote :

All tests passed on dev8.

Revision history for this message
Paul McCullagh (paul-mccullagh) wrote :

Hi Vlad,

This patch:

On May 27, 2009, at 5:40 PM, Vladimir Kolesnikov wrote:

> if (table_pool->opt_locked && !table_pool->opt_flushing) {
> - table_pool->opt_total_open--;
> /* Table will be closed below: */
> - if (table_pool->opt_total_open > 0)
> + if (table_pool->opt_total_open == 1)
> flush_table = FALSE;
> }

reverse the original logic.

Previously the table was only flushed when the last open table object
was closed.

Now it has been changed to, the table is always flushed, but not when
the last open table object is closed.

Was there are reason for changing this?

--
Paul McCullagh
PrimeBase Technologies
www.primebase.org
www.blobstreaming.org
pbxt.blogspot.com

Revision history for this message
Paul McCullagh (paul-mccullagh) wrote :

Hi Vlad,

On May 27, 2009, at 5:40 PM, Vladimir Kolesnikov wrote:

> +
> + if (ot) {
> xt_close_table(ot, flush_table, FALSE);
>
> + /* assume that table_pool cannot be invalidated in between as we
> have table_pool->opt_total_open > 0 */
> + xt_lock_mutex_ns(&db->db_ot_pool.opt_lock);
> + table_pool->opt_total_open--;

We need to move this code to here:

 db_free_open_table_pool(NULL, table_pool);

 if (!xt_broadcast_cond_ns(&db->db_ot_pool.opt_cond))
  goto failed;

Because threads that wait on opt_cond are either waiting for
table_pool->opt_total_open == 0 or for table_pool->opt_locked == 0.

Although "table_pool->opt_locked" is not adjusted by this function.

>
> + xt_unlock_mutex_ns(&db->db_ot_pool.opt_lock);
> + }
> +

--
Paul McCullagh
PrimeBase Technologies
www.primebase.org
www.blobstreaming.org
pbxt.blogspot.com

Revision history for this message
Paul McCullagh (paul-mccullagh) wrote :

On May 27, 2009, at 5:40 PM, Vladimir Kolesnikov wrote:

> - if (file_ptr->fil_memmap) {
> + if (file_ptr->fil_memmap && !file_ptr->fil_handle_count) {
> fs_close_fmap(self, file_ptr->fil_memmap);
> file_ptr->fil_memmap = NULL;
> }
> @@ -169,7 +169,8 @@
> file_ptr->fil_path = NULL;
> }
>
> - xt_free(self, file_ptr);

Small optimization, "!file_ptr->fil_ref_count && " is not required
here (checked above).

>
> + if (!file_ptr->fil_ref_count && !file_ptr->fil_handle_count)
> + xt_free(self, file_ptr);
> }
> }

--
Paul McCullagh
PrimeBase Technologies
www.primebase.org
www.blobstreaming.org
pbxt.blogspot.com

Revision history for this message
Paul McCullagh (paul-mccullagh) wrote :

I don't think " && !file_ptr->fil_handle_count" is correct here.

The "fs_globals.fsg_open_files" file object list only depends on the
"fil_ref_count" reference count.

It is OK to remove it from the list when this count goes to zero.

The open table object will still have a reference to the object, but
it can no longer be found using the list.

On May 27, 2009, at 5:40 PM, Vladimir Kolesnikov wrote:

> @@ -500,7 +501,7 @@
> pushr_(xt_sl_unlock, fs_globals.fsg_open_files);
>
> file_ptr->fil_ref_count--;
> - if (!file_ptr->fil_ref_count) {
> + if (!file_ptr->fil_ref_count && !file_ptr->fil_handle_count) {
> xt_sl_delete(self, fs_globals.fsg_open_files, file_ptr->fil_path);
> }

--
Paul McCullagh
PrimeBase Technologies
www.primebase.org
www.blobstreaming.org
pbxt.blogspot.com

Revision history for this message
Vladimir Kolesnikov (vkolesnikov) wrote :

my bug...

Revision history for this message
Vladimir Kolesnikov (vkolesnikov) wrote :

yes, i see...

Revision history for this message
Vladimir Kolesnikov (vkolesnikov) wrote :

ok, i thought we should retain it in the list while it's bbeing referenced. will fix.

637. By Vladimir Kolesnikov

merged with upstream

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'ChangeLog'
--- ChangeLog 2009-05-11 11:50:46 +0000
+++ ChangeLog 2009-05-27 14:45:14 +0000
@@ -3,6 +3,8 @@
33
4------- 1.0.08 RC - Not yet released4------- 1.0.08 RC - Not yet released
55
6RN241: Fixed a heap corruption bug
7
6RN238: Added an option to wait for the sweeper to clean up old transactions on a particular connection. This prevents the sweeper from getting too far behind.8RN238: Added an option to wait for the sweeper to clean up old transactions on a particular connection. This prevents the sweeper from getting too far behind.
79
8RN237: Added an option to lazy delete fixed length index entries. This means the index entries are just marked for deletion, instead of removing the items from the index page. This has the advantage that an exclusive lock is not always required for deletion.10RN237: Added an option to lazy delete fixed length index entries. This means the index entries are just marked for deletion, instead of removing the items from the index page. This has the advantage that an exclusive lock is not always required for deletion.
911
=== modified file 'src/database_xt.cc'
--- src/database_xt.cc 2009-05-11 10:35:16 +0000
+++ src/database_xt.cc 2009-05-27 14:39:04 +0000
@@ -1133,9 +1133,8 @@
1133 goto failed;1133 goto failed;
11341134
1135 if (table_pool->opt_locked && !table_pool->opt_flushing) {1135 if (table_pool->opt_locked && !table_pool->opt_flushing) {
1136 table_pool->opt_total_open--;
1137 /* Table will be closed below: */1136 /* Table will be closed below: */
1138 if (table_pool->opt_total_open > 0)1137 if (table_pool->opt_total_open == 1)
1139 flush_table = FALSE;1138 flush_table = FALSE;
1140 }1139 }
1141 else {1140 else {
@@ -1166,9 +1165,16 @@
1166 if (!xt_broadcast_cond_ns(&db->db_ot_pool.opt_cond))1165 if (!xt_broadcast_cond_ns(&db->db_ot_pool.opt_cond))
1167 goto failed;1166 goto failed;
1168 xt_unlock_mutex_ns(&db->db_ot_pool.opt_lock);1167 xt_unlock_mutex_ns(&db->db_ot_pool.opt_lock);
1169 if (ot)1168
1169 if (ot) {
1170 xt_close_table(ot, flush_table, FALSE);1170 xt_close_table(ot, flush_table, FALSE);
11711171
1172 /* assume that table_pool cannot be invalidated in between as we have table_pool->opt_total_open > 0 */
1173 xt_lock_mutex_ns(&db->db_ot_pool.opt_lock);
1174 table_pool->opt_total_open--;
1175 xt_unlock_mutex_ns(&db->db_ot_pool.opt_lock);
1176 }
1177
1172 return;1178 return;
11731179
1174 failed:1180 failed:
11751181
=== modified file 'src/filesys_xt.cc'
--- src/filesys_xt.cc 2009-04-26 23:44:06 +0000
+++ src/filesys_xt.cc 2009-05-27 14:39:04 +0000
@@ -152,7 +152,7 @@
152 file_ptr->fil_filedes = XT_NULL_FD;152 file_ptr->fil_filedes = XT_NULL_FD;
153 }153 }
154154
155 if (file_ptr->fil_memmap) {155 if (file_ptr->fil_memmap && !file_ptr->fil_handle_count) {
156 fs_close_fmap(self, file_ptr->fil_memmap);156 fs_close_fmap(self, file_ptr->fil_memmap);
157 file_ptr->fil_memmap = NULL;157 file_ptr->fil_memmap = NULL;
158 }158 }
@@ -169,7 +169,8 @@
169 file_ptr->fil_path = NULL;169 file_ptr->fil_path = NULL;
170 }170 }
171171
172 xt_free(self, file_ptr);172 if (!file_ptr->fil_ref_count && !file_ptr->fil_handle_count)
173 xt_free(self, file_ptr);
173 }174 }
174}175}
175176
@@ -500,7 +501,7 @@
500 pushr_(xt_sl_unlock, fs_globals.fsg_open_files);501 pushr_(xt_sl_unlock, fs_globals.fsg_open_files);
501502
502 file_ptr->fil_ref_count--;503 file_ptr->fil_ref_count--;
503 if (!file_ptr->fil_ref_count) {504 if (!file_ptr->fil_ref_count && !file_ptr->fil_handle_count) {
504 xt_sl_delete(self, fs_globals.fsg_open_files, file_ptr->fil_path);505 xt_sl_delete(self, fs_globals.fsg_open_files, file_ptr->fil_path);
505 }506 }
506507
@@ -1262,20 +1263,13 @@
1262xtPublic void xt_close_fmap(XTThreadPtr self, XTMapFilePtr map)1263xtPublic void xt_close_fmap(XTThreadPtr self, XTMapFilePtr map)
1263{1264{
1264 if (map->fr_file) {1265 if (map->fr_file) {
1265 xt_fs_release_file(self, map->fr_file);
1266
1267 xt_sl_lock(self, fs_globals.fsg_open_files);1266 xt_sl_lock(self, fs_globals.fsg_open_files);
1268 pushr_(xt_sl_unlock, fs_globals.fsg_open_files);1267 pushr_(xt_sl_unlock, fs_globals.fsg_open_files);
1269
1270 map->fr_file->fil_handle_count--;1268 map->fr_file->fil_handle_count--;
1271 if (!map->fr_file->fil_handle_count)
1272 fs_free_file(self, NULL, &map->fr_file);
1273
1274 freer_();1269 freer_();
12751270
1271 xt_fs_release_file(self, map->fr_file);
1276 map->fr_file = NULL;1272 map->fr_file = NULL;
1277
1278
1279 }1273 }
1280 map->mf_memmap = NULL;1274 map->mf_memmap = NULL;
1281 xt_free(self, map);1275 xt_free(self, map);
12821276
=== modified file 'src/heap_xt.cc'
--- src/heap_xt.cc 2009-04-02 20:27:49 +0000
+++ src/heap_xt.cc 2009-05-27 14:42:16 +0000
@@ -31,7 +31,7 @@
31#undef xt_heap_new31#undef xt_heap_new
32#endif32#endif
3333
34#ifdef DEBUG34#ifdef DEBUG_MEMORY
35xtPublic XTHeapPtr xt_mm_heap_new(XTThreadPtr self, size_t size, XTFinalizeFunc finalize, u_int line, c_char *file, xtBool track)35xtPublic XTHeapPtr xt_mm_heap_new(XTThreadPtr self, size_t size, XTFinalizeFunc finalize, u_int line, c_char *file, xtBool track)
36#else36#else
37xtPublic XTHeapPtr xt_heap_new(XTThreadPtr self, size_t size, XTFinalizeFunc finalize)37xtPublic XTHeapPtr xt_heap_new(XTThreadPtr self, size_t size, XTFinalizeFunc finalize)
@@ -39,7 +39,7 @@
39{39{
40 volatile XTHeapPtr hp;40 volatile XTHeapPtr hp;
41 41
42#ifdef DEBUG42#ifdef DEBUG_MEMORY
43 hp = (XTHeapPtr) xt_mm_calloc(self, size, line, file);43 hp = (XTHeapPtr) xt_mm_calloc(self, size, line, file);
44 hp->h_track = track;44 hp->h_track = track;
45 if (track)45 if (track)
@@ -67,19 +67,19 @@
6767
68xtPublic void xt_check_heap(XTThreadPtr self __attribute__((unused)), XTHeapPtr hp __attribute__((unused)))68xtPublic void xt_check_heap(XTThreadPtr self __attribute__((unused)), XTHeapPtr hp __attribute__((unused)))
69{69{
70#ifdef DEBUG70#ifdef DEBUG_MEMORY
71 xt_mm_malloc_size(self, hp);71 xt_mm_malloc_size(self, hp);
72#endif72#endif
73}73}
7474
75#ifdef DEBUG75#ifdef DEBUG_MEMORY
76xtPublic void xt_mm_heap_reference(XTThreadPtr self, XTHeapPtr hp, u_int line, c_char *file)76xtPublic void xt_mm_heap_reference(XTThreadPtr self, XTHeapPtr hp, u_int line, c_char *file)
77#else77#else
78xtPublic void xt_heap_reference(XTThreadPtr, XTHeapPtr hp)78xtPublic void xt_heap_reference(XTThreadPtr, XTHeapPtr hp)
79#endif79#endif
80{80{
81 xt_spinlock_lock(&hp->h_lock);81 xt_spinlock_lock(&hp->h_lock);
82#ifdef DEBUG82#ifdef DEBUG_MEMORY
83 if (hp->h_track)83 if (hp->h_track)
84 printf("HEAP: +1 %d->%d %s:%d\n", (int) hp->h_ref_count, (int) hp->h_ref_count+1, file, (int) line);84 printf("HEAP: +1 %d->%d %s:%d\n", (int) hp->h_ref_count, (int) hp->h_ref_count+1, file, (int) line);
85#endif85#endif
@@ -91,7 +91,7 @@
91{ 91{
92 if (!hp)92 if (!hp)
93 return;93 return;
94#ifdef DEBUG94#ifdef DEBUG_MEMORY
95 xt_spinlock_lock(&hp->h_lock);95 xt_spinlock_lock(&hp->h_lock);
96 ASSERT(hp->h_ref_count != 0);96 ASSERT(hp->h_ref_count != 0);
97 xt_spinlock_unlock(&hp->h_lock);97 xt_spinlock_unlock(&hp->h_lock);
@@ -100,7 +100,7 @@
100 if (hp->h_onrelease)100 if (hp->h_onrelease)
101 (*hp->h_onrelease)(self, hp);101 (*hp->h_onrelease)(self, hp);
102 if (hp->h_ref_count > 0) {102 if (hp->h_ref_count > 0) {
103#ifdef DEBUG103#ifdef DEBUG_MEMORY
104 if (hp->h_track)104 if (hp->h_track)
105 printf("HEAP: -1 %d->%d\n", (int) hp->h_ref_count, (int) hp->h_ref_count-1);105 printf("HEAP: -1 %d->%d\n", (int) hp->h_ref_count, (int) hp->h_ref_count-1);
106#endif106#endif
107107
=== modified file 'src/heap_xt.h'
--- src/heap_xt.h 2009-03-24 16:24:48 +0000
+++ src/heap_xt.h 2009-05-27 14:42:16 +0000
@@ -25,6 +25,7 @@
2525
26#include "xt_defs.h"26#include "xt_defs.h"
27#include "lock_xt.h"27#include "lock_xt.h"
28#include "memory_xt.h"
2829
29struct XTThread;30struct XTThread;
3031
@@ -59,7 +60,7 @@
5960
60void xt_check_heap(struct XTThread *self, XTHeapPtr mem);61void xt_check_heap(struct XTThread *self, XTHeapPtr mem);
6162
62#ifdef DEBUG63#ifdef DEBUG_MEMORY
63#define xt_heap_new(t, s, f) xt_mm_heap_new(t, s, f, __LINE__, __FILE__, FALSE)64#define xt_heap_new(t, s, f) xt_mm_heap_new(t, s, f, __LINE__, __FILE__, FALSE)
64#define xt_heap_new_track(t, s, f) xt_mm_heap_new(t, s, f, __LINE__, __FILE__, TRUE)65#define xt_heap_new_track(t, s, f) xt_mm_heap_new(t, s, f, __LINE__, __FILE__, TRUE)
65#define xt_heap_reference(t, s) xt_mm_heap_reference(t, s, __LINE__, __FILE__)66#define xt_heap_reference(t, s) xt_mm_heap_reference(t, s, __LINE__, __FILE__)
6667
=== modified file 'src/locklist_xt.cc'
--- src/locklist_xt.cc 2009-02-20 14:26:09 +0000
+++ src/locklist_xt.cc 2009-05-27 14:41:19 +0000
@@ -77,6 +77,12 @@
77 ptr->li_lock_type = XTThreadLockInfo::ATOMIC_RW_LOCK;77 ptr->li_lock_type = XTThreadLockInfo::ATOMIC_RW_LOCK;
78}78}
7979
80void xt_thread_lock_info_init(XTThreadLockInfoPtr ptr, XTSkewRWLock *lock)
81{
82 ptr->li_skew_rwlock = lock;
83 ptr->li_lock_type = XTThreadLockInfo::SKEW_RW_LOCK;
84}
85
80void xt_thread_lock_info_free(XTThreadLockInfoPtr ptr)86void xt_thread_lock_info_free(XTThreadLockInfoPtr ptr)
81{87{
82 /* TODO: check to see if it's present in a thread's list */88 /* TODO: check to see if it's present in a thread's list */
@@ -168,7 +174,7 @@
168 break;174 break;
169 case XTThreadLockInfo::SPIN_RW_LOCK:175 case XTThreadLockInfo::SPIN_RW_LOCK:
170 lock_type = "XTSpinRWLock";176 lock_type = "XTSpinRWLock";
171 lock_name = li->li_spin_rwlock->srw_name;177 lock_name = li->li_spin_rwlock->nrw_name;
172 break;178 break;
173 case XTThreadLockInfo::ATOMIC_RW_LOCK:179 case XTThreadLockInfo::ATOMIC_RW_LOCK:
174 lock_type = "XTAtomicRWLock";180 lock_type = "XTAtomicRWLock";
175181
=== modified file 'src/locklist_xt.h'
--- src/locklist_xt.h 2009-02-20 14:26:09 +0000
+++ src/locklist_xt.h 2009-05-27 14:41:19 +0000
@@ -43,6 +43,7 @@
43struct XTFastRWLock;43struct XTFastRWLock;
44struct XTSpinRWLock;44struct XTSpinRWLock;
45struct XTAtomicRWLock;45struct XTAtomicRWLock;
46struct XTSkewRWLock;
4647
47#ifdef XT_THREAD_LOCK_INFO48#ifdef XT_THREAD_LOCK_INFO
4849
@@ -61,7 +62,7 @@
61 */62 */
62typedef struct XTThreadLockInfo {63typedef struct XTThreadLockInfo {
6364
64 enum LockType { SPIN_LOCK, RW_MUTEX, MUTEX, RW_LOCK, FAST_LOCK, FAST_RW_LOCK, SPIN_RW_LOCK, ATOMIC_RW_LOCK };65 enum LockType { SPIN_LOCK, RW_MUTEX, MUTEX, RW_LOCK, FAST_LOCK, FAST_RW_LOCK, SPIN_RW_LOCK, ATOMIC_RW_LOCK, SKEW_RW_LOCK };
6566
66 LockType li_lock_type;67 LockType li_lock_type;
6768
@@ -74,6 +75,7 @@
74 XTAtomicRWLock *li_atomic_rwlock; // ATOMIC_RW_LOCK75 XTAtomicRWLock *li_atomic_rwlock; // ATOMIC_RW_LOCK
75 xt_mutex_struct *li_mutex; // MUTEX76 xt_mutex_struct *li_mutex; // MUTEX
76 xt_rwlock_struct *li_rwlock; // RW_LOCK77 xt_rwlock_struct *li_rwlock; // RW_LOCK
78 XTSkewRWLock *li_skew_rwlock; // SKEW_RW_LOCK
77 };79 };
78} 80}
79XTThreadLockInfoRec, *XTThreadLockInfoPtr;81XTThreadLockInfoRec, *XTThreadLockInfoPtr;
@@ -86,6 +88,7 @@
86void xt_thread_lock_info_init(XTThreadLockInfoPtr ptr, XTAtomicRWLock *lock);88void xt_thread_lock_info_init(XTThreadLockInfoPtr ptr, XTAtomicRWLock *lock);
87void xt_thread_lock_info_init(XTThreadLockInfoPtr ptr, xt_mutex_struct *lock);89void xt_thread_lock_info_init(XTThreadLockInfoPtr ptr, xt_mutex_struct *lock);
88void xt_thread_lock_info_init(XTThreadLockInfoPtr ptr, xt_rwlock_struct *lock);90void xt_thread_lock_info_init(XTThreadLockInfoPtr ptr, xt_rwlock_struct *lock);
91void xt_thread_lock_info_init(XTThreadLockInfoPtr ptr, XTSkewRWLock *lock);
89void xt_thread_lock_info_free(XTThreadLockInfoPtr ptr);92void xt_thread_lock_info_free(XTThreadLockInfoPtr ptr);
9093
91void xt_thread_lock_info_add_owner (XTThreadLockInfoPtr ptr);94void xt_thread_lock_info_add_owner (XTThreadLockInfoPtr ptr);
9295
=== modified file 'src/memory_xt.cc'
--- src/memory_xt.cc 2009-03-19 11:28:00 +0000
+++ src/memory_xt.cc 2009-05-27 14:42:16 +0000
@@ -186,7 +186,7 @@
186 free(ptr);186 free(ptr);
187}187}
188188
189#ifdef DEBUG189#ifdef DEBUG_MEMORY
190190
191/*191/*
192 * -----------------------------------------------------------------------192 * -----------------------------------------------------------------------
@@ -849,7 +849,7 @@
849849
850xtPublic xtBool xt_init_memory(void)850xtPublic xtBool xt_init_memory(void)
851{851{
852#ifdef DEBUG852#ifdef DEBUG_MEMORY
853 XTThreadPtr self = NULL;853 XTThreadPtr self = NULL;
854854
855 if (!xt_init_mutex_with_autoname(NULL, &mm_mutex))855 if (!xt_init_mutex_with_autoname(NULL, &mm_mutex))
@@ -875,7 +875,7 @@
875875
876xtPublic void xt_exit_memory(void)876xtPublic void xt_exit_memory(void)
877{877{
878#ifdef DEBUG878#ifdef DEBUG_MEMORY
879 long mm;879 long mm;
880 int i;880 int i;
881881
@@ -919,7 +919,7 @@
919 * MEMORY ALLOCATION UTILITIES919 * MEMORY ALLOCATION UTILITIES
920 */920 */
921921
922#ifdef DEBUG922#ifdef DEBUG_MEMORY
923char *xt_mm_dup_string(XTThreadPtr self, c_char *str, u_int line, c_char *file)923char *xt_mm_dup_string(XTThreadPtr self, c_char *str, u_int line, c_char *file)
924#else924#else
925char *xt_dup_string(XTThreadPtr self, c_char *str)925char *xt_dup_string(XTThreadPtr self, c_char *str)
@@ -931,7 +931,7 @@
931 if (!str)931 if (!str)
932 return NULL;932 return NULL;
933 len = strlen(str);933 len = strlen(str);
934#ifdef DEBUG934#ifdef DEBUG_MEMORY
935 new_str = (char *) xt_mm_malloc(self, len + 1, line, file);935 new_str = (char *) xt_mm_malloc(self, len + 1, line, file);
936#else936#else
937 new_str = (char *) xt_malloc(self, len + 1);937 new_str = (char *) xt_malloc(self, len + 1);
938938
=== modified file 'src/memory_xt.h'
--- src/memory_xt.h 2008-11-21 11:30:42 +0000
+++ src/memory_xt.h 2009-05-27 14:42:16 +0000
@@ -30,6 +30,10 @@
30struct XTThread;30struct XTThread;
3131
32#ifdef DEBUG32#ifdef DEBUG
33#define DEBUG_MEMORY
34#endif
35
36#ifdef DEBUG_MEMORY
3337
34#define XT_MM_STACK_TRACE 20038#define XT_MM_STACK_TRACE 200
35#define XT_MM_TRACE_DEPTH 439#define XT_MM_TRACE_DEPTH 4
@@ -109,7 +113,7 @@
109113
110#endif114#endif
111115
112#ifdef DEBUG116#ifdef DEBUG_MEMORY
113#define xt_dup_string(t, s) xt_mm_dup_string(t, s, __LINE__, __FILE__)117#define xt_dup_string(t, s) xt_mm_dup_string(t, s, __LINE__, __FILE__)
114118
115char *xt_mm_dup_string(struct XTThread *self, const char *path, u_int line, const char *file);119char *xt_mm_dup_string(struct XTThread *self, const char *path, u_int line, const char *file);

Subscribers

People subscribed via source and target branches