Skip to content

Commit 2c0388b

Browse files
caolanmcaolanm
committed
Drop Admin::Document FILE* when document is expired
Admin::Documents are moved to _expiredDocuments when the matching real document is closed and are persistent until coolwsd exits, keeping the FILE* associated buffers around for coolwsds lifetime. fclose always uses ::close on its fd, but wasn't typically called before now, and ChildProcess always closed the fd. If we starting calling fclose then there is a duplicate ::close. By moving the FILE* to the ChildProcess, managed by shared_ptr, we can avoid having a permanently open FILE* in _expiredDocuments and avoid dup() for extra fds and guessing games as to who should close the smaps file. Signed-off-by: Caolán McNamara <caolan.mcnamara@collabora.com> Change-Id: Ied919f6e0fe614be7ad42cc15ddca60f0a4adab2
1 parent 758c0f6 commit 2c0388b

File tree

6 files changed

+38
-24
lines changed

6 files changed

+38
-24
lines changed

‎wsd/Admin.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -789,10 +789,10 @@ void Admin::uploadedAlert(const std::string& docKey, pid_t pid, bool value)
789789

790790
void Admin::addDoc(const std::string& docKey, pid_t pid, const std::string& filename,
791791
const std::string& sessionId, const std::string& userName, const std::string& userId,
792-
const int smapsFD, const std::string& wopiSrc, bool readOnly)
792+
const std::weak_ptr<FILE>& smapsFp, const std::string& wopiSrc, bool readOnly)
793793
{
794-
addCallback([this, docKey, pid, filename, sessionId, userName, userId, smapsFD, wopiSrc, readOnly] {
795-
_model.addDocument(docKey, pid, filename, sessionId, userName, userId, smapsFD, Poco::URI(wopiSrc), readOnly);
794+
addCallback([this, docKey, pid, filename, sessionId, userName, userId, smapsFp, wopiSrc, readOnly] {
795+
_model.addDocument(docKey, pid, filename, sessionId, userName, userId, smapsFp, Poco::URI(wopiSrc), readOnly);
796796
});
797797
}
798798

‎wsd/Admin.hpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,8 @@ class Admin final : public SocketPoll
115115
/// Calls with same pid will increment view count, if pid already exists
116116
void addDoc(const std::string& docKey, pid_t pid, const std::string& filename,
117117
const std::string& sessionId, const std::string& userName,
118-
const std::string& userId, int smapsFD, const std::string& wopiSrc, bool readOnly);
118+
const std::string& userId, const std::weak_ptr<FILE>& smapsFD,
119+
const std::string& wopiSrc, bool readOnly);
119120

120121
/// Decrement view count till becomes zero after which doc is removed
121122
void rmDoc(const std::string& docKey, const std::string& sessionId);

‎wsd/AdminModel.cpp

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,8 @@ void Document::updateMemoryDirty()
147147
if (now - _lastTimeSMapsRead >= 5)
148148
{
149149
size_t lastMemDirty = _memoryDirty;
150-
_memoryDirty = _procSMaps ? Util::getPssAndDirtyFromSMaps(_procSMaps).second : 0;
150+
auto procSMaps = _procSMaps.lock();
151+
_memoryDirty = procSMaps ? Util::getPssAndDirtyFromSMaps(procSMaps.get()).second : 0;
151152
_lastTimeSMapsRead = now;
152153
if (lastMemDirty != _memoryDirty)
153154
_hasMemDirtyChanged = true;
@@ -533,12 +534,12 @@ void AdminModel::uploadedAlert(const std::string& docKey, pid_t pid, bool value)
533534
void AdminModel::addDocument(const std::string& docKey, pid_t pid,
534535
const std::string& filename, const std::string& sessionId,
535536
const std::string& userName, const std::string& userId,
536-
const int smapsFD, const Poco::URI& wopiSrc, bool isViewReadOnly)
537+
const std::weak_ptr<FILE>& smapsFp, const Poco::URI& wopiSrc, bool isViewReadOnly)
537538
{
538539
ASSERT_CORRECT_THREAD_OWNER(_owner);
539540
const auto ret =
540541
_documents.emplace(docKey, std::make_unique<Document>(docKey, pid, filename, wopiSrc));
541-
ret.first->second->setProcSMapsFD(smapsFD);
542+
ret.first->second->setProcSMapsFp(smapsFp);
542543
ret.first->second->takeSnapshot();
543544
ret.first->second->addView(sessionId, userName, userId, isViewReadOnly);
544545
LOG_DBG("Added admin document [" << docKey << "].");
@@ -614,6 +615,7 @@ void AdminModel::doRemove(std::map<std::string, std::unique_ptr<Document>>::iter
614615
std::unique_ptr<Document> doc;
615616
std::swap(doc, docIt->second);
616617
_documents.erase(docIt);
618+
doc->setExpired();
617619
_expiredDocuments.emplace(docItKey + std::to_string(std::chrono::duration_cast<std::chrono::nanoseconds>(
618620
std::chrono::steady_clock::now().time_since_epoch()).count()),
619621
std::move(doc));

‎wsd/AdminModel.hpp

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,6 @@ class Document
174174
, _recvBytes(0)
175175
, _wopiDownloadDuration(0)
176176
, _wopiUploadDuration(0)
177-
, _procSMaps(nullptr)
178177
, _lastTimeSMapsRead(0)
179178
, _badBehaviorDetectionTime(0)
180179
, _abortTime(0)
@@ -189,8 +188,6 @@ class Document
189188

190189
~Document()
191190
{
192-
if (_procSMaps)
193-
fclose(_procSMaps);
194191
}
195192

196193
std::string getDocKey() const { return _docKey; }
@@ -249,14 +246,21 @@ class Document
249246
std::chrono::milliseconds getWopiDownloadDuration() const { return _wopiDownloadDuration; }
250247
void setWopiUploadDuration(const std::chrono::milliseconds wopiUploadDuration) { _wopiUploadDuration = wopiUploadDuration; }
251248
std::chrono::milliseconds getWopiUploadDuration() const { return _wopiUploadDuration; }
252-
void setProcSMapsFD(const int smapsFD) { _procSMaps = fdopen(smapsFD, "r"); }
249+
void setProcSMapsFp(std::weak_ptr<FILE> procSMaps) { _procSMaps = procSMaps; }
253250
bool hasMemDirtyChanged() const { return _hasMemDirtyChanged; }
254251
void setMemDirtyChanged(bool changeStatus) { _hasMemDirtyChanged = changeStatus; }
255252
time_t getBadBehaviorDetectionTime() const { return _badBehaviorDetectionTime; }
256253
void setBadBehaviorDetectionTime(time_t badBehaviorDetectionTime){ _badBehaviorDetectionTime = badBehaviorDetectionTime;}
257254
time_t getAbortTime() const { return _abortTime; }
258255
void setAbortTime(time_t abortTime) { _abortTime = abortTime; }
259256

257+
// If the Document is expired then _memoryDirty is now static and we can
258+
// drop the _procSMaps reference as we have no further interest in it.
259+
void setExpired()
260+
{
261+
_procSMaps.reset();
262+
}
263+
260264
std::string to_string() const;
261265

262266
private:
@@ -283,7 +287,7 @@ class Document
283287
std::chrono::milliseconds _wopiDownloadDuration;
284288
std::chrono::milliseconds _wopiUploadDuration;
285289

286-
FILE* _procSMaps;
290+
std::weak_ptr<FILE> _procSMaps;
287291
std::time_t _lastTimeSMapsRead;
288292

289293
std::time_t _badBehaviorDetectionTime;
@@ -393,8 +397,8 @@ class AdminModel
393397

394398
void addDocument(const std::string& docKey, pid_t pid, const std::string& filename,
395399
const std::string& sessionId, const std::string& userName,
396-
const std::string& userId, int smapsFD, const Poco::URI& wopiSrc,
397-
bool readOnly);
400+
const std::string& userId, const std::weak_ptr<FILE>& smapsFp,
401+
const Poco::URI& wopiSrc, bool readOnly);
398402

399403
void removeDocument(const std::string& docKey, const std::string& sessionId);
400404
void removeDocument(const std::string& docKey);

‎wsd/DocumentBroker.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3721,7 +3721,7 @@ DocumentBroker::addSessionInternal(const std::shared_ptr<ClientSession>& session
37213721
// Create uri without query parameters
37223722
const std::string wopiSrc(uri.getScheme() + "://" + uri.getAuthority() + uri.getPath());
37233723
_admin.addDoc(_docKey, getPid(), getFilename(), id, session->getUserName(),
3724-
session->getUserId(), _childProcess->getSMapsFD(), wopiSrc, session->isReadOnly());
3724+
session->getUserId(), _childProcess->getSMapsFp(), wopiSrc, session->isReadOnly());
37253725
_admin.setDocWopiDownloadDuration(_docKey, _wopiDownloadDuration);
37263726
#endif
37273727

‎wsd/Process.hpp

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -202,7 +202,6 @@ class ChildProcess final : public WSProcess
202202
std::make_shared<WebSocketHandler>(socket, request))
203203
, _jailId(jailId)
204204
, _configId(configId)
205-
, _smapsFD(-1)
206205
, _urpFromKitFD(socket->getIncomingFD(SharedFDType::URPFromKit))
207206
, _urpToKitFD(socket->getIncomingFD(SharedFDType::URPToKit))
208207
{
@@ -232,11 +231,7 @@ class ChildProcess final : public WSProcess
232231
std::shared_ptr<StreamSocket> urpToKit(_urpToKit.lock());
233232
if (urpToKit)
234233
urpToKit->shutdown();
235-
if (_smapsFD != -1)
236-
{
237-
::close(_smapsFD);
238-
_smapsFD = -1;
239-
}
234+
_smapsFp.reset();
240235
}
241236

242237
const ChildProcess& operator=(ChildProcess&& other) = delete;
@@ -245,8 +240,20 @@ class ChildProcess final : public WSProcess
245240
std::shared_ptr<DocumentBroker> getDocumentBroker() const { return _docBroker.lock(); }
246241
const std::string& getJailId() const { return _jailId; }
247242
const std::string& getConfigId() const { return _configId; }
248-
void setSMapsFD(int smapsFD) { _smapsFD = smapsFD; }
249-
int getSMapsFD() { return _smapsFD; }
243+
void setSMapsFD(int smapsFD)
244+
{
245+
_smapsFp = std::shared_ptr<FILE>(fdopen(smapsFD, "r"), [](FILE* p) {
246+
if (!p)
247+
return;
248+
fclose(p);
249+
});
250+
if (!_smapsFp)
251+
{
252+
LOG_ERR("Error while fdopen smaps fd");
253+
::close(smapsFD);
254+
}
255+
}
256+
std::weak_ptr<FILE> getSMapsFp() const { return _smapsFp; }
250257

251258
void moveSocketFromTo(const std::shared_ptr<SocketPoll>& from, SocketPoll& to)
252259
{
@@ -259,7 +266,7 @@ class ChildProcess final : public WSProcess
259266
std::weak_ptr<DocumentBroker> _docBroker;
260267
std::weak_ptr<StreamSocket> _urpFromKit;
261268
std::weak_ptr<StreamSocket> _urpToKit;
262-
int _smapsFD;
269+
std::shared_ptr<FILE> _smapsFp;
263270
int _urpFromKitFD;
264271
int _urpToKitFD;
265272
};

0 commit comments

Comments
 (0)