From 30151163f11abf4f6340023c90c0bb8547ff025d Mon Sep 17 00:00:00 2001 From: Gavin Halliday Date: Mon, 2 Sep 2024 12:02:46 +0100 Subject: [PATCH] HPCC-32553 Ensure errors are reported when closing files Signed-off-by: Gavin Halliday --- dali/base/dacoven.cpp | 2 + dali/base/dasds.cpp | 2 + dali/server/daserver.cpp | 1 + ecl/ecl-bundle/ecl-bundle.cpp | 1 + ecl/eclcc/eclcc.cpp | 10 ++++- ecl/eclcmd/queries/ecl-queries.cpp | 3 ++ ecl/hql/hqlcache.cpp | 1 + ecl/hqlcpp/hqlecl.cpp | 4 +- ecl/hqlcpp/hqlwcpp.cpp | 17 +++++--- ecl/hqlcpp/hqlwcpp.hpp | 4 +- ecl/hqlcpp/hqlwcpp.ipp | 14 +------ ecl/hthor/hthor.cpp | 1 + system/jlib/jcomp.cpp | 4 ++ system/jlib/jfile.cpp | 17 +++++++- system/jlib/jlzw.cpp | 64 ++++++++++++++++++------------ system/jlib/jptree.cpp | 2 + system/jlib/jutil.cpp | 2 + thorlcr/master/thgraphmanager.cpp | 1 + thorlcr/slave/slavmain.cpp | 1 + thorlcr/thorutil/thormisc.cpp | 1 + 20 files changed, 101 insertions(+), 51 deletions(-) diff --git a/dali/base/dacoven.cpp b/dali/base/dacoven.cpp index 081f5f2d73c..8aa60877944 100644 --- a/dali/base/dacoven.cpp +++ b/dali/base/dacoven.cpp @@ -563,12 +563,14 @@ class CCovenServer: public CCovenBase Owned f = createIFile(storename.get()); Owned io = f->open(IFOcreate); io->write(0, xml.length(), xml.str()); + io->close(); io.clear(); if (!backupname.isEmpty()) { try { f.setown(createIFile(backupname.get())); io.setown(f->open(IFOcreate)); io->write(0, xml.length(), xml.str()); + io->close(); io.clear(); } catch (IException *e) { diff --git a/dali/base/dasds.cpp b/dali/base/dasds.cpp index 89caaa9efc5..2f6410172be 100644 --- a/dali/base/dasds.cpp +++ b/dali/base/dasds.cpp @@ -1022,6 +1022,7 @@ void writeDelta(StringBuffer &xml, IFile &iFile, const char *msg="", unsigned re sprintf(strNum, "%016" I64F "X", fLen); memcpy(headerPtr + deltaHeaderSizeOff, strNum, 16); iFileIO->write(0, strlen(deltaHeader), headerPtr); + iFileIO->close(); } catch (IException *e) { @@ -1213,6 +1214,7 @@ class CDeltaWriter : implements IThreaded Owned iFile = createIFile(rL.str()); Owned fileIO = iFile->open(IFOcreate); fileIO->write(0, length, data); + fileIO->close(); } catch (IException *e) { diff --git a/dali/server/daserver.cpp b/dali/server/daserver.cpp index cdd2f21a1e9..f0004457432 100644 --- a/dali/server/daserver.cpp +++ b/dali/server/daserver.cpp @@ -612,6 +612,7 @@ int main(int argc, const char* argv[]) backupCheck.append("bakchk.").append((unsigned)GetCurrentProcessId()); OwnedIFile iFileDataDir = createIFile(backupCheck.str()); OwnedIFileIO iFileIO = iFileDataDir->open(IFOcreate); + iFileIO->close(); iFileIO.clear(); try { diff --git a/ecl/ecl-bundle/ecl-bundle.cpp b/ecl/ecl-bundle/ecl-bundle.cpp index 7a9074905e4..bebbc1e4d24 100644 --- a/ecl/ecl-bundle/ecl-bundle.cpp +++ b/ecl/ecl-bundle/ecl-bundle.cpp @@ -750,6 +750,7 @@ class CBundleInfoSet : public CInterfaceOf VStringBuffer redirect("IMPORT %s.%s.%s.%s as _%s; EXPORT %s := _%s;", VERSION_SUBDIR, name, version, name, name, name, name); Owned rfile = redirector->open(IFOcreate); rfile->write(0, redirect.length(), redirect.str()); + rfile->close(); bundle->setActive(true); } virtual void setActive(const char *version) diff --git a/ecl/eclcc/eclcc.cpp b/ecl/eclcc/eclcc.cpp index 14e7d101633..e898cabece3 100644 --- a/ecl/eclcc/eclcc.cpp +++ b/ecl/eclcc/eclcc.cpp @@ -1990,8 +1990,11 @@ void EclCC::outputXmlToOutputFile(EclCompileInstance & instance, IPropertyTree * { //Work around windows problem writing 64K to stdout if not redirected/piped Owned stream = createIOStream(ifileio.get()); - Owned buffered = createBufferedIOStream(stream,0x8000); - saveXML(*buffered, xml); + { + Owned buffered = createBufferedIOStream(stream,0x8000); + saveXML(*buffered, xml); + } + ifileio->close(); } } @@ -2044,7 +2047,10 @@ void EclCC::generateOutput(EclCompileInstance & instance) OwnedIFileIO ifileio = createArchiveOutputFile(instance); if (ifileio) + { ifileio->write(0, filenames.length(), filenames.str()); + ifileio->close(); + } } else { diff --git a/ecl/eclcmd/queries/ecl-queries.cpp b/ecl/eclcmd/queries/ecl-queries.cpp index 34e0cfa7030..5a59f94a20b 100644 --- a/ecl/eclcmd/queries/ecl-queries.cpp +++ b/ecl/eclcmd/queries/ecl-queries.cpp @@ -1382,7 +1382,10 @@ class EclCmdQueriesExport : public EclCmdCommon fprintf(stdout, "\nWriting to file %s\n", file->queryFilename()); if (io.get()) + { io->write(0, strlen(s), s); + io->close(); + } else fprintf(stderr, "\nFailed to create file %s\n", file->queryFilename()); } diff --git a/ecl/hql/hqlcache.cpp b/ecl/hql/hqlcache.cpp index 1e9cd46ebd0..56a6b24f32f 100644 --- a/ecl/hql/hqlcache.cpp +++ b/ecl/hql/hqlcache.cpp @@ -492,6 +492,7 @@ static void extractFile(const char * path, const char * moduleName, const char * Owned io = file->open(IFOcreate); if (text) io->write(0, strlen(text), text); + io->close(); io.clear(); if (ts) { diff --git a/ecl/hqlcpp/hqlecl.cpp b/ecl/hqlcpp/hqlecl.cpp index e52ac773ca9..ddd6c3e63c3 100644 --- a/ecl/hqlcpp/hqlecl.cpp +++ b/ecl/hqlcpp/hqlecl.cpp @@ -310,7 +310,7 @@ void HqlDllGenerator::expandCode(StringBuffer & filename, const char * codeTempl addDirectoryPrefix(fullname, targetDir).append(filename); Owned out = createIFile(fullname.str()); - Owned expander = createTemplateExpander(out, codeTemplate); + Owned expander = createTemplateExpander(codeTemplate); Owned writer = createCppWriter(*code, compiler); Owned props = createProperties(true); @@ -324,7 +324,7 @@ void HqlDllGenerator::expandCode(StringBuffer & filename, const char * codeTempl props->setProp("headerName", headerName.str()); props->setProp("outputName", fullname.str()); - expander->generate(*writer, pass, props); + expander->generate(*writer, out, pass, props); totalGeneratedSize += out->size(); diff --git a/ecl/hqlcpp/hqlwcpp.cpp b/ecl/hqlcpp/hqlwcpp.cpp index 0adcc71d34c..ef59a67735c 100644 --- a/ecl/hqlcpp/hqlwcpp.cpp +++ b/ecl/hqlcpp/hqlwcpp.cpp @@ -103,9 +103,14 @@ CppWriterTemplate::CppWriterTemplate(const char * codeTemplate) loadTemplate(codeTemplate); } -void CppWriterTemplate::generate(ISectionWriter & writer, unsigned pass, IProperties * properties) +void CppWriterTemplate::generate(ISectionWriter & writer, IFile * outputFile, unsigned pass, IProperties * properties) { - writer.setOutput(out, outStream); + Owned io = outputFile->open(IFOcreate); + if (!io) + throwError1(HQLERR_CouldNotCreateOutputX, outputFile->queryFilename()); + + outStream.setown(createIOStream(io)); + writer.setOutput(outputFile, outStream); const char * finger = text; bool output = true; @@ -149,6 +154,8 @@ void CppWriterTemplate::generate(ISectionWriter & writer, unsigned pass, IProper outputQuoted(writer, end-finger, finger); writer.setOutput(NULL, NULL); + outStream.clear(); + io->close(); } void CppWriterTemplate::loadTemplate(const char * codeTemplate) @@ -2271,11 +2278,9 @@ void HqlCppSectionWriter::generateSection(unsigned delta, IAtom * section, unsig //--------------------------------------------------------------------------- -ITemplateExpander * createTemplateExpander(IFile * output, const char * codeTemplate) +ITemplateExpander * createTemplateExpander(const char * codeTemplate) { - Owned expander = new CppWriterTemplate(codeTemplate); - expander->setOutput(output); - return expander.getClear(); + return new CppWriterTemplate(codeTemplate); } ISectionWriter * createCppWriter(IHqlCppInstance & _instance, CompilerType compiler) diff --git a/ecl/hqlcpp/hqlwcpp.hpp b/ecl/hqlcpp/hqlwcpp.hpp index 535c41374b8..e8001cc1de3 100644 --- a/ecl/hqlcpp/hqlwcpp.hpp +++ b/ecl/hqlcpp/hqlwcpp.hpp @@ -35,7 +35,7 @@ interface HQLCPP_API ISectionWriter : public IInterface interface HQLCPP_API ITemplateExpander : public IInterface { public: - virtual void generate(ISectionWriter & writer, unsigned pass, IProperties * properties = NULL) = 0; + virtual void generate(ISectionWriter & writer, IFile * _output, unsigned pass, IProperties * properties = NULL) = 0; }; extern HQLCPP_API StringBuffer & generateExprCpp(StringBuffer & out, IHqlExpression * expr, CompilerType compiler); @@ -43,7 +43,7 @@ extern HQLCPP_API StringBuffer & generateTypeCpp(StringBuffer & out, ITypeInfo * bool generateFunctionPrototype(StringBuffer & out, IHqlExpression * funcdef, CompilerType compiler); void generateFunctionReturnType(StringBuffer & prefix, StringBuffer & params, ITypeInfo * retType, IHqlExpression * attrs, CompilerType compiler); -extern HQLCPP_API ITemplateExpander * createTemplateExpander(IFile * output, const char * codeTemplate); +extern HQLCPP_API ITemplateExpander * createTemplateExpander(const char * codeTemplate); extern HQLCPP_API ISectionWriter * createCppWriter(IHqlCppInstance & _instance, CompilerType compiler); extern bool isTypePassedByAddress(ITypeInfo * type); diff --git a/ecl/hqlcpp/hqlwcpp.ipp b/ecl/hqlcpp/hqlwcpp.ipp index 8074016b25f..613c5da19fd 100644 --- a/ecl/hqlcpp/hqlwcpp.ipp +++ b/ecl/hqlcpp/hqlwcpp.ipp @@ -23,7 +23,7 @@ public: CppWriterTemplate(const char * codeTemplate); IMPLEMENT_IINTERFACE - virtual void generate(ISectionWriter & writer, unsigned pass, IProperties * properties = NULL); + virtual void generate(ISectionWriter & writer, IFile * _output, unsigned pass, IProperties * properties = NULL); void outputQuoted(ISectionWriter & writer, size32_t len, const char * str) { @@ -31,17 +31,6 @@ public: outStream->write(len, str); } - void setOutput(IFile * _output) - { - Owned io = _output->open(IFOcreate); - if (!io) - throwError1(HQLERR_CouldNotCreateOutputX, _output->queryFilename()); - - out.set(_output); - outStream.setown(createIOStream(io)); - } - - private: void loadTemplate(const char * codeTemplate); enum TplSectionType { TplEmbed, TplExpand, TplCondition, TplEndCondition }; @@ -58,7 +47,6 @@ protected: const char * text; unsigned len; CIArray sections; - Owned out; Owned outStream; }; diff --git a/ecl/hthor/hthor.cpp b/ecl/hthor/hthor.cpp index 166cea2c0a7..c4820125790 100644 --- a/ecl/hthor/hthor.cpp +++ b/ecl/hthor/hthor.cpp @@ -1295,6 +1295,7 @@ void CHThorIndexWriteActivity::execute() offsetBranches = builder->getOffsetBranches(); out->flush(); out.clear(); + io->close(); } if(clusterHandler) diff --git a/system/jlib/jcomp.cpp b/system/jlib/jcomp.cpp index 0e86580ccc8..9f6175c7edf 100644 --- a/system/jlib/jcomp.cpp +++ b/system/jlib/jcomp.cpp @@ -475,7 +475,10 @@ void CppCompiler::writeLogFile(const char* filepath, StringBuffer& log) Owned fio = f->open(IFOcreate); if(fio.get()) + { fio->write(0, log.length(), log.str()); + fio->close(); + } } bool CppCompiler::compile() @@ -560,6 +563,7 @@ bool CppCompiler::compile() //Don't leave lots of blank log files around if the compile was successful bool logIsEmpty = (dstIO->size() == 0); + dstIO->close(); dstIO.clear(); if (ret && logIsEmpty) dstfile->remove(); diff --git a/system/jlib/jfile.cpp b/system/jlib/jfile.cpp index b7308a06850..95cc17ce45a 100644 --- a/system/jlib/jfile.cpp +++ b/system/jlib/jfile.cpp @@ -73,6 +73,9 @@ // this should not be enabled in WindowRemoteDirectory used //#define CHECK_FILE_IO // If enabled, reads and writes are checked for sensible parameters +#ifdef _DEBUG +//#define CHECK_FILE_CLOSED_BEFORE_DELETE +#endif #ifdef _DEBUG #define ASSERTEX(e) assertex(e); @@ -2075,13 +2078,22 @@ CFileIO::CFileIO(HANDLE handle, IFOmode _openmode, IFSHmode _sharemode, IFEflags CFileIO::~CFileIO() { +#ifdef CHECK_FILE_CLOSED_BEFORE_DELETE + //Any file that is being written to, should be closed before the object is destroyed, otherwise errors from failing to commit will be lost + if ((file != NULLFILE) && (openmode!=IFOread)) + { + OERRLOG("CFileIO::~CFileIO - file object destroyed without being closed first"); // A programmer problem, but the operator should know about it. + PrintStackReport(); + } +#endif try { close(); } catch (IException * e) { - EXCLOG(e, "CFileIO::~CFileIO"); + //An error closing a file cannot throw an exception, but should be logged as a very severe error in the logs. + DISLOG(e, "CFileIO::~CFileIO"); PrintStackReport(); e->Release(); } @@ -3289,6 +3301,7 @@ void doCopyFile(IFile * target, IFile * source, size32_t buffersize, ICopyFilePr if (progress && progress->onProgress(offset, total) != CFPcontinue) break; } + targetIO->close(); // Ensure errors are reported. targetIO.clear(); if (usetmp) { StringAttr tail(pathTail(target->queryFilename())); @@ -4441,6 +4454,7 @@ void touchFile(IFile *iFile) Owned iFileIO = iFile->open(IFOcreate); if (!iFileIO) throw makeStringExceptionV(0, "touchFile: failed to create file %s", iFile->queryFilename()); + iFileIO->close(); } void touchFile(const char *filename) @@ -7323,6 +7337,7 @@ extern jlib_decl void writeSentinelFile(IFile * sentinelFile) { Owned sentinel = sentinelFile->open(IFOcreate); sentinel->write(0, 5, "rerun"); + sentinel->close(); } catch(IException *E) { diff --git a/system/jlib/jlzw.cpp b/system/jlib/jlzw.cpp index d0fb75762ff..5629865855e 100644 --- a/system/jlib/jlzw.cpp +++ b/system/jlib/jlzw.cpp @@ -2077,39 +2077,51 @@ class CCompressedFile : implements ICompressedFileIO, public CInterface //If the blocks are being expanded incrementally check if the position is within the current block //This test will never be true for row compressed data, or non-incremental decompression - if ((pos >= startBlockPos) && (pos < startBlockPos + fullBlockSize)) + try { - if (pos < nextExpansionPos) + if ((pos >= startBlockPos) && (pos < startBlockPos + fullBlockSize)) { - //Start decompressing again and avoid re-reading the data from disk - const void * rawData; - if (fileio) - rawData = compressedInputBlock.get(); - else - rawData = mmfile->base()+startBlockPos; - - assertex(rawData); - size32_t exp = expander->expandFirst(curblockbuf, rawData); - curblockpos = startBlockPos; - nextExpansionPos = startBlockPos + exp; if (pos < nextExpansionPos) - return; + { + //Start decompressing again and avoid re-reading the data from disk + const void * rawData; + if (fileio) + rawData = compressedInputBlock.get(); + else + rawData = mmfile->base()+startBlockPos; + + assertex(rawData); + nextExpansionPos = startBlockPos; // update in case an exception is thrown + size32_t exp = expander->expandFirst(curblockbuf, rawData); + curblockpos = startBlockPos; + nextExpansionPos = startBlockPos + exp; + if (pos < nextExpansionPos) + return; - curblockbuf.clear(); - } + curblockbuf.clear(); + } - for (;;) - { - size32_t nextSize = expander->expandNext(curblockbuf); - if (nextSize == 0) - throw makeStringExceptionV(-1, "Unexpected zero length compression block at position %llu", curblockpos); + for (;;) + { + size32_t nextSize = expander->expandNext(curblockbuf); + if (nextSize == 0) + throw makeStringException(-1, "Unexpected zero length compression block"); - curblockpos = nextExpansionPos; - nextExpansionPos = nextExpansionPos+nextSize; - if (pos < nextExpansionPos) - return; + curblockpos = nextExpansionPos; + nextExpansionPos = nextExpansionPos+nextSize; + if (pos < nextExpansionPos) + return; + } } } + catch (IException * e) + { + unsigned code = e->errorCode(); + StringBuffer msg; + e->errorMessage(msg).appendf(" at position %llu of %llu", nextExpansionPos, trailer.indexPos); + e->Release(); + throw makeStringException(code, msg.str()); + } size32_t expsize; curblocknum = lookupIndex(pos,curblockpos,expsize); @@ -2456,6 +2468,8 @@ class CCompressedFile : implements ICompressedFileIO, public CInterface } checkedwrite(trailer.indexPos,indexbuf.length(),indexbuf.toByteArray()); indexbuf.clear(); + if (fileio) + fileio->close(); } mode = ICFread; curblockpos = 0; diff --git a/system/jlib/jptree.cpp b/system/jlib/jptree.cpp index d7ba2871c78..70d483495ad 100644 --- a/system/jlib/jptree.cpp +++ b/system/jlib/jptree.cpp @@ -6332,6 +6332,7 @@ void saveXML(IFile &ifile, const IPropertyTree *tree, unsigned indent, unsigned if (!ifileio) throw MakeStringException(0, "saveXML: could not find %s to open", ifile.queryFilename()); saveXML(*ifileio, tree, indent, flags); + ifileio->close(); // Ensure errors are reported } void saveXML(IFileIO &ifileio, const IPropertyTree *tree, unsigned indent, unsigned flags) @@ -9692,6 +9693,7 @@ void saveYAML(IFile &ifile, const IPropertyTree *tree, unsigned indent, unsigned if (!ifileio) throw MakeStringException(0, "saveXML: could not find %s to open", ifile.queryFilename()); saveYAML(*ifileio, tree, indent, flags); + ifileio->close(); } void saveYAML(IFileIO &ifileio, const IPropertyTree *tree, unsigned indent, unsigned flags) diff --git a/system/jlib/jutil.cpp b/system/jlib/jutil.cpp index d5c8acb8917..109945c56c6 100644 --- a/system/jlib/jutil.cpp +++ b/system/jlib/jutil.cpp @@ -3422,6 +3422,7 @@ void jlib_decl atomicWriteFile(const char *fileName, const char *output) if (!ifileio) throw MakeStringException(0, "atomicWriteFile: could not create output file %s", newFileName.str()); ifileio->write(0, strlen(output), output); + ifileio->close(); } #else VStringBuffer newFileName("%s.XXXXXX", fileName); @@ -3435,6 +3436,7 @@ void jlib_decl atomicWriteFile(const char *fileName, const char *output) if (!ifileio) throw MakeStringException(0, "atomicWriteFile: could not create output file %s", newFileName.str()); ifileio->write(0, strlen(output), output); + ifileio->close(); } #endif if (file->exists()) diff --git a/thorlcr/master/thgraphmanager.cpp b/thorlcr/master/thgraphmanager.cpp index ce4f63bd4fa..dc78ae4c416 100644 --- a/thorlcr/master/thgraphmanager.cpp +++ b/thorlcr/master/thgraphmanager.cpp @@ -1083,6 +1083,7 @@ bool CJobManager::executeGraph(IConstWorkUnit &workunit, const char *graphName, out->setCreateFlags(S_IRWXU); OwnedIFileIO io = out->open(IFOcreate); io->write(0, file.length(), file.toByteArray()); + io->close(); io.clear(); } catch (IException *e) diff --git a/thorlcr/slave/slavmain.cpp b/thorlcr/slave/slavmain.cpp index d3eb593354b..325312fe10d 100644 --- a/thorlcr/slave/slavmain.cpp +++ b/thorlcr/slave/slavmain.cpp @@ -1831,6 +1831,7 @@ class CJobListener : public CSimpleInterface iFile->setCreateFlags(S_IRWXU); Owned iFileIO = iFile->open(IFOwrite); iFileIO->write(0, size, queryPtr); + iFileIO->close(); } catch (IException *e) { diff --git a/thorlcr/thorutil/thormisc.cpp b/thorlcr/thorutil/thormisc.cpp index 2aaf9e65914..afc06cee34a 100644 --- a/thorlcr/thorutil/thormisc.cpp +++ b/thorlcr/thorutil/thormisc.cpp @@ -1700,4 +1700,5 @@ void saveWuidToFile(const char *wuid) if (!wuidFileIO) throw makeStringException(0, "Failed to create file 'wuid' to store current workunit for post mortem script"); wuidFileIO->write(0, strlen(wuid), wuid); + wuidFileIO->close(); }