Skip to content

Commit

Permalink
Remove more silently failing file writes. See mkazhdan#280.
Browse files Browse the repository at this point in the history
In general, the code's IO design has a problem:
It uses RAII for file resource handling.
This is not robust, because only `fclose()` (or `fflush()`) is
guaranteed to actually pass data to the OS, and thus surface potential errors.

"RAII-for-files" requires that `fclose()` be put in C++ destructors.
But destructors are not allowed to `throw`.
Thus "RAII-for-files" cannot do proper error handling of failed writes.

**C++ RAII cannot be used for file management.**

Or anything where the cleanup action can fail.
(Unless you're happy to swallow errors silently, which nobody should be.)

This commit replaces all calls to `fwrite()` and `fclose()` by

    throwing_fwrite()
    throwing_fclose()
    fwrite_from_destructor()
    fclose_from_destructor()

All variants try to report good error reasons, such as
"no space left on device"
(currently only imlemented for POSIX, not for Windows, where
no error details are produced).

The `*_from_destructor()` versions use non-allocating writes to
`std::cerr` and terminate the program.
  • Loading branch information
nh2 committed Nov 3, 2023
1 parent 8d6a77d commit e155f45
Show file tree
Hide file tree
Showing 23 changed files with 161 additions and 90 deletions.
10 changes: 5 additions & 5 deletions Src/AdaptiveTreeVisualization.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -183,11 +183,11 @@ void WriteGrid( const char *fileName , ConstPointer( Real ) values , unsigned in
FILE *fp = fopen( fileName , "wb" );
if( !fp ) ERROR_OUT( "Failed to open file for writing: " , fileName );
int r = (int)res;
fwrite( &r , sizeof(int) , 1 , fp );
throwing_fwrite( &r , sizeof(int) , 1 , fp );
size_t count = 1;
for( unsigned int d=0 ; d<Dim ; d++ ) count *= res;
fwrite( values , sizeof(Real) , count , fp );
fclose( fp );
throwing_fwrite( values , sizeof(Real) , count , fp );
throwing_fclose( fp );
}
else
{
Expand Down Expand Up @@ -392,7 +392,7 @@ void _Execute( const FEMTree< Dim , Real > *tree , XForm< Real , Dim+1 > modelTo
DenseNodeData< Real , IsotropicUIntPack< Dim-1 , FEMSig > >::WriteSignatures( fs );
sliceTree->write( fs , sliceModelToUnitCube , false );
sliceCoefficients.write( fs );
fclose( fp );
throwing_fclose( fp );

delete sliceTree;
}
Expand Down Expand Up @@ -522,6 +522,6 @@ int main( int argc , char* argv[] )
default: ERROR_OUT( "Only dimensions 1-4 supported" );
}

fclose( fp );
throwing_fclose( fp );
return EXIT_SUCCESS;
}
4 changes: 2 additions & 2 deletions Src/Array.inl
Original file line number Diff line number Diff line change
Expand Up @@ -430,14 +430,14 @@ template< class C >
size_t fwrite( Array< C > source , size_t eSize , size_t count , FILE* fp )
{
if( count*eSize>source.maximum()*sizeof( C ) ) ERROR_OUT( "Size of write exceeds source maximum: " , count*eSize , " > " , source.maximum()*sizeof( C ) );
if( count ) return fwrite( &source[0] , eSize , count , fp );
if( count ) return throwing_fwrite( &source[0] , eSize , count , fp );
else return 0;
}
template< class C >
size_t fwrite( ConstArray< C > source , size_t eSize , size_t count , FILE* fp )
{
if( count*eSize>source.maximum()*sizeof( C ) ) ERROR_OUT( "Size of write exceeds source maximum: " , count*eSize , " > " , source.maximum()*sizeof( C ) );
if( count ) return fwrite( &source[0] , eSize , count , fp );
if( count ) return throwing_fwrite( &source[0] , eSize , count , fp );
else return 0;
}
template< class C >
Expand Down
19 changes: 10 additions & 9 deletions Src/BMPStream.inl
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ DAMAGE.

#include <stdio.h>
#include "Array.h"
#include "MyMiscellany.h"

/* constants for the biCompression field */
#define BI_RGB 0L
Expand Down Expand Up @@ -91,16 +92,16 @@ inline void BMPGetImageInfo( char* fileName , int& width , int& height , int& ch
fread( &bmfh , sizeof( BITMAPFILEHEADER ) , 1 , fp );
fread( &bmih , sizeof( BITMAPINFOHEADER ) , 1 , fp );

if( bmfh.bfType!=BMP_BF_TYPE || bmfh.bfOffBits!=BMP_BF_OFF_BITS ){ fclose(fp) ; ERROR_OUT( "Bad bitmap file header" ); };
if( bmih.biSize!=BMP_BI_SIZE || bmih.biWidth<=0 || bmih.biHeight<=0 || bmih.biPlanes!=1 || bmih.biBitCount!=24 || bmih.biCompression!=BI_RGB ) { fclose(fp) ; ERROR_OUT( "Bad bitmap file info" ); }
if( bmfh.bfType!=BMP_BF_TYPE || bmfh.bfOffBits!=BMP_BF_OFF_BITS ){ throwing_fclose(fp) ; ERROR_OUT( "Bad bitmap file header" ); };
if( bmih.biSize!=BMP_BI_SIZE || bmih.biWidth<=0 || bmih.biHeight<=0 || bmih.biPlanes!=1 || bmih.biBitCount!=24 || bmih.biCompression!=BI_RGB ) { throwing_fclose(fp) ; ERROR_OUT( "Bad bitmap file info" ); }
width = bmih.biWidth;
height = bmih.biHeight;
channels = 3;
bytesPerChannel = 1;
int lineLength = width * channels;
if( lineLength % 4 ) lineLength = (lineLength / 4 + 1) * 4;
if( bmih.biSizeImage!=lineLength*height ){ fclose(fp) ; ERROR_OUT( "Bad bitmap image size" ); };
fclose( fp );
if( bmih.biSizeImage!=lineLength*height ){ throwing_fclose(fp) ; ERROR_OUT( "Bad bitmap image size" ); };
throwing_fclose( fp );
}

inline void* BMPInitRead( char* fileName , int& width , int& height )
Expand Down Expand Up @@ -154,7 +155,7 @@ inline void* BMPInitWrite( char* fileName , int width , int height , int quality
bmfh.bfReserved2 = 0;
bmfh.bfOffBits = BMP_BF_OFF_BITS;

fwrite( &bmfh , sizeof(BITMAPFILEHEADER) , 1 , info->fp );
throwing_fwrite( &bmfh , sizeof(BITMAPFILEHEADER) , 1 , info->fp );

bmih.biSize = BMP_BI_SIZE;
bmih.biWidth = width;
Expand All @@ -168,7 +169,7 @@ inline void* BMPInitWrite( char* fileName , int width , int height , int quality
bmih.biClrUsed = 0;
bmih.biClrImportant = 0;

fwrite( &bmih , sizeof(BITMAPINFOHEADER) , 1 , info->fp );
throwing_fwrite( &bmih , sizeof(BITMAPINFOHEADER) , 1 , info->fp );

return info;
}
Expand All @@ -178,7 +179,7 @@ inline void BMPWriteRow( Pointer( ChannelType ) pixels , void* v , int j )
BMPInfo* info = (BMPInfo*)v;
ConvertRow< ChannelType , unsigned char >( pixels , info->data , info->width , Channels , 3 );
for( int i=0 ; i<info->width ; i++ ) { unsigned char temp = info->data[i*3] ; info->data[i*3] = info->data[i*3+2] ; info->data[i*3+2] = temp; }
fwrite( info->data , sizeof(unsigned char) , info->width*3 , info->fp );
throwing_fwrite( info->data , sizeof(unsigned char) , info->width*3 , info->fp );
int nbytes = info->width*3;
while( nbytes % 4 ) putc( 0 , info->fp ) , nbytes++;
}
Expand All @@ -197,11 +198,11 @@ void BMPReadRow( Pointer( ChannelType ) pixels , void* v , int j )
inline void BMPFinalize( void* v )
{
BMPInfo* info = (BMPInfo*)v;
fclose( info->fp );
throwing_fclose( info->fp );
FreePointer( info->data );
free( info );
}

inline void BMPFinalizeWrite( void* v ){ BMPFinalize( v ); }
inline void BMPFinalizeRead ( void* v ){ BMPFinalize( v ); }
#endif // BMP_STREAM_INCLUDED
#endif // BMP_STREAM_INCLUDED
8 changes: 4 additions & 4 deletions Src/CmdLineParser.inl
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,7 @@ inline char** ReadWords(const char* fileName,int& cnt)
if(!fp){return NULL;}
cnt=0;
while(fscanf(fp," %s ",temp)==1){cnt++;}
fclose(fp);
throwing_fclose(fp);

names=new char*[cnt];
if(!names){return NULL;}
Expand All @@ -297,12 +297,12 @@ inline char** ReadWords(const char* fileName,int& cnt)
for(int j=0;j<cnt;j++){delete[] names[j];}
delete[] names;
cnt=0;
fclose(fp);
throwing_fclose(fp);
return NULL;
}
strcpy(names[cnt],temp);
cnt++;
}
fclose(fp);
throwing_fclose(fp);
return names;
}
}
13 changes: 7 additions & 6 deletions Src/DataStream.imp.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ DAMAGE.

#include <vector>
#include "DataStream.h"
#include "MyMiscellany.h"
#include "VertexFactory.h"
#include "Ply.h"

Expand Down Expand Up @@ -86,7 +87,7 @@ struct FileBackedOutputDataStream : public OutputDataStream< Data >
protected:
FILE *_fp;

void base_write( const Data &d ){ fwrite( &d , sizeof(Data) , 1 , _fp ); }
void base_write( const Data &d ){ throwing_fwrite( &d , sizeof(Data) , 1 , _fp ); }
};

/////////////////////////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -129,8 +130,8 @@ struct FileBackedOutputDataStream< std::vector< Data > > : public OutputDataStre
void base_write( const std::vector< Data > &d )
{
unsigned int pSize = (unsigned int)d.size();
fwrite( &pSize , sizeof(unsigned int) , 1 , _fp );
fwrite( &d[0] , sizeof(Data) , pSize , _fp );
throwing_fwrite( &pSize , sizeof(unsigned int) , 1 , _fp );
throwing_fwrite( &d[0] , sizeof(Data) , pSize , _fp );
}
};

Expand Down Expand Up @@ -170,7 +171,7 @@ struct FileBackedOutputFactoryTypeStream : public OutputDataStream< typename Fac
Pointer( char ) _buffer;
const size_t _bufferSize;

void base_write( const Data &d ){ _factory.toBuffer( d , _buffer ) ; fwrite( _buffer , sizeof(unsigned char) , _bufferSize , _fp ); }
void base_write( const Data &d ){ _factory.toBuffer( d , _buffer ) ; throwing_fwrite( _buffer , sizeof(unsigned char) , _bufferSize , _fp ); }
};


Expand Down Expand Up @@ -215,7 +216,7 @@ struct BinaryInputDataStream : public InputDataStream< typename Factory::VertexT
typedef typename Factory::VertexType Data;

BinaryInputDataStream( const char* filename , const Factory &factory );
~BinaryInputDataStream( void ){ fclose( _fp ) , _fp=NULL; }
~BinaryInputDataStream( void ){ fclose_from_destructor( _fp ) , _fp=NULL; }
void reset( void );

protected:
Expand All @@ -231,7 +232,7 @@ struct BinaryOutputDataStream : public OutputDataStream< typename Factory::Verte
typedef typename Factory::VertexType Data;

BinaryOutputDataStream( const char* filename , const Factory &factory );
~BinaryOutputDataStream( void ){ fclose( _fp ) , _fp=NULL; }
~BinaryOutputDataStream( void ){ fclose_from_destructor( _fp ) , _fp=NULL; }
void reset( void ){ fseek( _fp , 0 , SEEK_SET ); }

protected:
Expand Down
4 changes: 2 additions & 2 deletions Src/DataStream.imp.inl
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ ASCIIInputDataStream< Factory >::ASCIIInputDataStream( const char* fileName , co
template< typename Factory >
ASCIIInputDataStream< Factory >::~ASCIIInputDataStream( void )
{
fclose( _fp );
fclose_from_destructor( _fp );
_fp = NULL;
}

Expand All @@ -62,7 +62,7 @@ ASCIIOutputDataStream< Factory >::ASCIIOutputDataStream( const char* fileName ,
template< typename Factory >
ASCIIOutputDataStream< Factory >::~ASCIIOutputDataStream( void )
{
fclose( _fp );
fclose_from_destructor( _fp );
_fp = NULL;
}

Expand Down
6 changes: 3 additions & 3 deletions Src/EDTInHeat.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ void _Execute( int argc , char* argv[] )
if( fscanf( fp , " %f " , &f )!=1 ) ERROR_OUT( "Failed to read xform" );
modelToUnitCube(i,j) = (Real)f;
}
fclose( fp );
throwing_fclose( fp );
}
}
else modelToUnitCube = XForm< Real , Dim+1 >::Identity();
Expand Down Expand Up @@ -281,7 +281,7 @@ void _Execute( int argc , char* argv[] )
for( int j=0 ; j<Dim+1 ; j++ ) fprintf( fp , " %f" , (float)unitCubeToModel(i,j) );
fprintf( fp , "\n" );
}
fclose( fp );
throwing_fclose( fp );
}
}

Expand Down Expand Up @@ -537,7 +537,7 @@ void _Execute( int argc , char* argv[] )
DenseNodeData< Real , IsotropicUIntPack< Dim , FEMSig > >::WriteSignatures( fs );
tree.write( fs , modelToUnitCube , false );
edtSolution.write( fs );
fclose( fp );
throwing_fclose( fp );
}
}
if( valueInfo ) delete valueInfo , valueInfo = NULL;
Expand Down
6 changes: 3 additions & 3 deletions Src/Image.h
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,7 @@ bool TiledImageReader::GetInfo( const char* fileName , unsigned int& width , uns
else if( _tileHeights[r+1]!=_h ) ERROR_OUT( "Images in the same row must have the same heights: " , _tileHeights[r+1] ," != " , _h );
}
}
fclose( fp );
throwing_fclose( fp );
if( fileDir ) delete[] fileDir;
_tileWidths[0] = _tileHeights[0] = 0;
for( unsigned int c=0 ; c<_tileColumns ; c++ ) _tileWidths[c+1] += _tileWidths[c];
Expand Down Expand Up @@ -370,7 +370,7 @@ TiledImageReader::TiledImageReader( const char* fileName , unsigned int& width ,
strcpy( _tileNames[r*_tileColumns+c] , tileName );
}
}
fclose( fp );
throwing_fclose( fp );
if( fileDir ) delete[] fileDir;
for( unsigned int r=0 ; r<_tileRows ; r++ ) for( unsigned int c=0 ; c<_tileColumns ; c++ )
{
Expand Down Expand Up @@ -442,7 +442,7 @@ TiledImageWriter::TiledImageWriter( const char* fileName , unsigned int width ,
fprintf( fp , "%s\n" , localTileName );
delete[] localTileName;
}
fclose( fp );
throwing_fclose( fp );
_currentPixelRow = 0;
}
TiledImageWriter::~TiledImageWriter( void )
Expand Down
6 changes: 3 additions & 3 deletions Src/JPEG.inl
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ inline bool JPEGReader::GetInfo( const char* fileName , unsigned int& width , un
height = cInfo.image_height;
jpeg_destroy_decompress( &cInfo );

fclose( fp );
throwing_fclose( fp );
return true;
}

Expand Down Expand Up @@ -102,7 +102,7 @@ inline JPEGReader::~JPEGReader( void )
{
(void) jpeg_finish_decompress( &_cInfo );
jpeg_destroy_decompress( &_cInfo );
fclose( _fp );
fclose_from_destructor( _fp );
}
inline unsigned int JPEGReader::nextRow( unsigned char* row )
{
Expand Down Expand Up @@ -137,7 +137,7 @@ inline JPEGWriter::~JPEGWriter( void )
{
jpeg_finish_compress( &_cInfo );
jpeg_destroy_compress( &_cInfo );
fclose( _fp );
fclose_from_destructor( _fp );
}
inline unsigned int JPEGWriter::nextRow( const unsigned char* row )
{
Expand Down
6 changes: 3 additions & 3 deletions Src/MergePlyClientServer.inl
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ inline void _Copy( FILE *target , FILE *source , size_t sz , size_t bufferSize=1
if( sz==-1 )
{
size_t ioBytes;
while( ioBytes=fread( buffer , sizeof(unsigned char) , bufferSize , source ) ) fwrite( buffer , sizeof(unsigned char) , ioBytes , target );
while( ioBytes=fread( buffer , sizeof(unsigned char) , bufferSize , source ) ) throwing_fwrite( buffer , sizeof(unsigned char) , ioBytes , target );
}
else
{
Expand Down Expand Up @@ -110,8 +110,8 @@ void _OffsetPolygons( const Factory &factory , std::string in , std::string out

auto WritePolygon = [&]( FILE *fp , int n )
{
fwrite( &n , sizeof(int) , 1 , fp );
fwrite( faceIndices , sizeof(Index) , n , fp );
throwing_fwrite( &n , sizeof(int) , 1 , fp );
throwing_fwrite( faceIndices , sizeof(Index) , n , fp );
};

for( unsigned int j=0 ; j<std::get<2>( _elems[1] ).size() ; j++ ) inPly->get_property( std::get<0>( _elems[1] ) , &std::get<2>( _elems[1] )[j] );
Expand Down
Loading

0 comments on commit e155f45

Please sign in to comment.