core(tls): don't use tlsSlots without synchronization

alalek · alalek · commit 68ef903a7c60 · 2017-07-26T22:45:55.000+03:00
diff --git a/modules/core/src/system.cpp b/modules/core/src/system.cpp
@@ -1306,7 +1306,8 @@ struct ThreadData
 class TlsStorage
 {
 public:
-    TlsStorage()
+    TlsStorage() :
+        tlsSlotsSize(0)
     {
         tlsSlots.reserve(32);
         threads.reserve(32);
@@ -1351,9 +1352,10 @@ class TlsStorage
     size_t reserveSlot()
     {
         AutoLock guard(mtxGlobalAccess);
+        CV_Assert(tlsSlotsSize == tlsSlots.size());
 
         // Find unused slots
-        for(size_t slot = 0; slot < tlsSlots.size(); slot++)
+        for(size_t slot = 0; slot < tlsSlotsSize; slot++)
         {
             if(!tlsSlots[slot])
             {
@@ -1363,15 +1365,16 @@ class TlsStorage
         }
 
         // Create new slot
-        tlsSlots.push_back(1);
-        return (tlsSlots.size()-1);
+        tlsSlots.push_back(1); tlsSlotsSize++;
+        return tlsSlotsSize - 1;
     }
 
     // Release TLS storage index and pass associated data to caller
     void releaseSlot(size_t slotIdx, std::vector<void*> &dataVec, bool keepSlot = false)
     {
         AutoLock guard(mtxGlobalAccess);
-        CV_Assert(tlsSlots.size() > slotIdx);
+        CV_Assert(tlsSlotsSize == tlsSlots.size());
+        CV_Assert(tlsSlotsSize > slotIdx);
 
         for(size_t i = 0; i < threads.size(); i++)
         {
@@ -1393,7 +1396,7 @@ class TlsStorage
     // Get data by TLS storage index
     void* getData(size_t slotIdx) const
     {
-        CV_Assert(tlsSlots.size() > slotIdx);
+        CV_Assert(tlsSlotsSize > slotIdx);
 
         ThreadData* threadData = (ThreadData*)tls.GetData();
         if(threadData && threadData->slots.size() > slotIdx)
@@ -1406,7 +1409,8 @@ class TlsStorage
     void gather(size_t slotIdx, std::vector<void*> &dataVec)
     {
         AutoLock guard(mtxGlobalAccess);
-        CV_Assert(tlsSlots.size() > slotIdx);
+        CV_Assert(tlsSlotsSize == tlsSlots.size());
+        CV_Assert(tlsSlotsSize > slotIdx);
 
         for(size_t i = 0; i < threads.size(); i++)
         {
@@ -1422,7 +1426,7 @@ class TlsStorage
     // Set data to storage index
     void setData(size_t slotIdx, void* pData)
     {
-        CV_Assert(tlsSlots.size() > slotIdx && pData != NULL);
+        CV_Assert(tlsSlotsSize > slotIdx);
 
         ThreadData* threadData = (ThreadData*)tls.GetData();
         if(!threadData)
@@ -1438,9 +1442,8 @@ class TlsStorage
 
         if(slotIdx >= threadData->slots.size())
         {
-            AutoLock guard(mtxGlobalAccess);
-            while(slotIdx >= threadData->slots.size())
-                threadData->slots.push_back(NULL);
+            AutoLock guard(mtxGlobalAccess); // keep synchronization with gather() calls
+            threadData->slots.resize(slotIdx + 1, NULL);
         }
         threadData->slots[slotIdx] = pData;
     }
@@ -1449,6 +1452,8 @@ class TlsStorage
     TlsAbstraction tls; // TLS abstraction layer instance
 
     Mutex  mtxGlobalAccess;           // Shared objects operation guard
+    size_t tlsSlotsSize;              // equal to tlsSlots.size() in synchronized sections
+                                      // without synchronization this counter doesn't desrease - it is used for slotIdx sanity checks
     std::vector<int> tlsSlots;        // TLS keys state
     std::vector<ThreadData*> threads; // Array for all allocated data. Thread data pointers are placed here to allow data cleanup
 };

Original file line number	Diff line number	Diff line change
`@@ -1306,7 +1306,8 @@ struct ThreadData`
`1306`	`1306`	`class TlsStorage`
`1307`	`1307`	`{`
`1308`	`1308`	`public:`
`1309`		`- TlsStorage()`
	`1309`	`+ TlsStorage() :`
	`1310`	`+ tlsSlotsSize(0)`
`1310`	`1311`	`{`
`1311`	`1312`	`tlsSlots.reserve(32);`
`1312`	`1313`	`threads.reserve(32);`
`@@ -1351,9 +1352,10 @@ class TlsStorage`
`1351`	`1352`	`size_t reserveSlot()`
`1352`	`1353`	`{`
`1353`	`1354`	`AutoLock guard(mtxGlobalAccess);`
	`1355`	`+ CV_Assert(tlsSlotsSize == tlsSlots.size());`
`1354`	`1356`
`1355`	`1357`	`// Find unused slots`
`1356`		`- for(size_t slot = 0; slot < tlsSlots.size(); slot++)`
	`1358`	`+ for(size_t slot = 0; slot < tlsSlotsSize; slot++)`
`1357`	`1359`	`{`
`1358`	`1360`	`if(!tlsSlots[slot])`
`1359`	`1361`	`{`
`@@ -1363,15 +1365,16 @@ class TlsStorage`
`1363`	`1365`	`}`
`1364`	`1366`
`1365`	`1367`	`// Create new slot`
`1366`		`- tlsSlots.push_back(1);`
`1367`		`- return (tlsSlots.size()-1);`
	`1368`	`+ tlsSlots.push_back(1); tlsSlotsSize++;`
	`1369`	`+ return tlsSlotsSize - 1;`
`1368`	`1370`	`}`
`1369`	`1371`
`1370`	`1372`	`// Release TLS storage index and pass associated data to caller`
`1371`	`1373`	`void releaseSlot(size_t slotIdx, std::vector<void*> &dataVec, bool keepSlot = false)`
`1372`	`1374`	`{`
`1373`	`1375`	`AutoLock guard(mtxGlobalAccess);`
`1374`		`- CV_Assert(tlsSlots.size() > slotIdx);`
	`1376`	`+ CV_Assert(tlsSlotsSize == tlsSlots.size());`
	`1377`	`+ CV_Assert(tlsSlotsSize > slotIdx);`
`1375`	`1378`
`1376`	`1379`	`for(size_t i = 0; i < threads.size(); i++)`
`1377`	`1380`	`{`
`@@ -1393,7 +1396,7 @@ class TlsStorage`
`1393`	`1396`	`// Get data by TLS storage index`
`1394`	`1397`	`void* getData(size_t slotIdx) const`
`1395`	`1398`	`{`
`1396`		`- CV_Assert(tlsSlots.size() > slotIdx);`
	`1399`	`+ CV_Assert(tlsSlotsSize > slotIdx);`
`1397`	`1400`
`1398`	`1401`	`ThreadData* threadData = (ThreadData*)tls.GetData();`
`1399`	`1402`	`if(threadData && threadData->slots.size() > slotIdx)`
`@@ -1406,7 +1409,8 @@ class TlsStorage`
`1406`	`1409`	`void gather(size_t slotIdx, std::vector<void*> &dataVec)`
`1407`	`1410`	`{`
`1408`	`1411`	`AutoLock guard(mtxGlobalAccess);`
`1409`		`- CV_Assert(tlsSlots.size() > slotIdx);`
	`1412`	`+ CV_Assert(tlsSlotsSize == tlsSlots.size());`
	`1413`	`+ CV_Assert(tlsSlotsSize > slotIdx);`
`1410`	`1414`
`1411`	`1415`	`for(size_t i = 0; i < threads.size(); i++)`
`1412`	`1416`	`{`
`@@ -1422,7 +1426,7 @@ class TlsStorage`
`1422`	`1426`	`// Set data to storage index`
`1423`	`1427`	`void setData(size_t slotIdx, void* pData)`
`1424`	`1428`	`{`
`1425`		`- CV_Assert(tlsSlots.size() > slotIdx && pData != NULL);`
	`1429`	`+ CV_Assert(tlsSlotsSize > slotIdx);`
`1426`	`1430`
`1427`	`1431`	`ThreadData* threadData = (ThreadData*)tls.GetData();`
`1428`	`1432`	`if(!threadData)`
`@@ -1438,9 +1442,8 @@ class TlsStorage`
`1438`	`1442`
`1439`	`1443`	`if(slotIdx >= threadData->slots.size())`
`1440`	`1444`	`{`
`1441`		`- AutoLock guard(mtxGlobalAccess);`
`1442`		`- while(slotIdx >= threadData->slots.size())`
`1443`		`- threadData->slots.push_back(NULL);`
	`1445`	`+ AutoLock guard(mtxGlobalAccess); // keep synchronization with gather() calls`
	`1446`	`+ threadData->slots.resize(slotIdx + 1, NULL);`
`1444`	`1447`	`}`
`1445`	`1448`	`threadData->slots[slotIdx] = pData;`
`1446`	`1449`	`}`
`@@ -1449,6 +1452,8 @@ class TlsStorage`
`1449`	`1452`	`TlsAbstraction tls; // TLS abstraction layer instance`
`1450`	`1453`
`1451`	`1454`	`Mutex mtxGlobalAccess; // Shared objects operation guard`
	`1455`	`+ size_t tlsSlotsSize; // equal to tlsSlots.size() in synchronized sections`
	`1456`	`+ // without synchronization this counter doesn't desrease - it is used for slotIdx sanity checks`
`1452`	`1457`	`std::vector<int> tlsSlots; // TLS keys state`
`1453`	`1458`	`std::vector<ThreadData*> threads; // Array for all allocated data. Thread data pointers are placed here to allow data cleanup`
`1454`	`1459`	`};`