Skip to content

Commit

Permalink
TFile: Improve handling of potential integer overflow
Browse files Browse the repository at this point in the history
Raise an assertion for unexpected arguments and use size_t instead of int
for the size argument which is typically sizeof(some_datatype).

Signed-off-by: Stefan Weil <sw@weilnetz.de>
  • Loading branch information
stweil committed Jun 4, 2018
1 parent 45b11cd commit 8bea6bc
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 12 deletions.
28 changes: 19 additions & 9 deletions src/ccutil/serialis.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ char* TFile::FGets(char* buffer, int buffer_size) {
return size > 0 ? buffer : nullptr;
}

int TFile::FReadEndian(void* buffer, int size, int count) {
int TFile::FReadEndian(void* buffer, size_t size, int count) {
int num_read = FRead(buffer, size, count);
if (swap_) {
char* char_buffer = static_cast<char*>(buffer);
Expand All @@ -109,12 +109,20 @@ int TFile::FReadEndian(void* buffer, int size, int count) {
return num_read;
}

int TFile::FRead(void* buffer, int size, int count) {
int TFile::FRead(void* buffer, size_t size, int count) {
ASSERT_HOST(!is_writing_);
int required_size = size * count;
if (required_size <= 0) return 0;
if (data_->size() - offset_ < required_size)
ASSERT_HOST(size > 0);
ASSERT_HOST(count > 0);
size_t required_size;
if (SIZE_MAX / size <= count) {
// Avoid integer overflow.
required_size = data_->size() - offset_;
} else {
required_size = size * count;
if (data_->size() - offset_ < required_size) {
required_size = data_->size() - offset_;
}
}
if (required_size > 0 && buffer != nullptr)
memcpy(buffer, &(*data_)[offset_], required_size);
offset_ += required_size;
Expand Down Expand Up @@ -149,14 +157,16 @@ bool TFile::CloseWrite(const STRING& filename, FileWriter writer) {
return (*writer)(*data_, filename);
}

int TFile::FWrite(const void* buffer, int size, int count) {
int TFile::FWrite(const void* buffer, size_t size, int count) {
ASSERT_HOST(is_writing_);
int total = size * count;
if (total <= 0) return 0;
ASSERT_HOST(size > 0);
ASSERT_HOST(count > 0);
ASSERT_HOST(SIZE_MAX / size > count);
size_t total = size * count;
const char* buf = static_cast<const char*>(buffer);
// This isn't very efficient, but memory is so fast compared to disk
// that it is relatively unimportant, and very simple.
for (int i = 0; i < total; ++i)
for (size_t i = 0; i < total; ++i)
data_->push_back(buf[i]);
return count;
}
Expand Down
6 changes: 3 additions & 3 deletions src/ccutil/serialis.h
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,9 @@ class TFile {
// Replicates fread, followed by a swap of the bytes if needed, returning the
// number of items read. If swap_ is true then the count items will each have
// size bytes reversed.
int FReadEndian(void* buffer, int size, int count);
int FReadEndian(void* buffer, size_t size, int count);
// Replicates fread, returning the number of items read.
int FRead(void* buffer, int size, int count);
int FRead(void* buffer, size_t size, int count);
// Resets the TFile as if it has been Opened, but nothing read.
// Only allowed while reading!
void Rewind();
Expand All @@ -87,7 +87,7 @@ class TFile {

// Replicates fwrite, returning the number of items written.
// To use fprintf, use snprintf and FWrite.
int FWrite(const void* buffer, int size, int count);
int FWrite(const void* buffer, size_t size, int count);

private:
// The number of bytes used so far.
Expand Down

3 comments on commit 8bea6bc

@Shreeshrii
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FAIL: osd_test

Running main() from gtest_main.cc
[==========] Running 14 tests from 8 test cases.
[----------] Global test environment set-up.
[----------] 3 tests from TessdataEngEuroHebrew/OSDTest
[ RUN ] TessdataEngEuroHebrew/OSDTest.MatchOrientationDegrees/0
count > 0:Error:Assert failed:in file serialis.cpp, line 115
FAIL osd_test (exit status: 139)

@Shreeshrii
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also failing

tesseract data/train/cop.sophia-001.exp0.tif data/train/cop.sophia-001.exp0 --psm 13 lstm.train
Tesseract Open Source OCR Engine v4.0.0-beta.1-311-g83ae with Leptonica
Page 1
Warning. Invalid resolution 0 dpi. Using 70 instead.
count > 0:Error:Assert failed:in file serialis.cpp, line 163
Makefile:91: recipe for target 'data/train/cop.sophia-001.exp0.lstmf' failed
make: *** [data/train/cop.sophia-001.exp0.lstmf] Segmentation fault (core dumped)

@stweil
Copy link
Contributor Author

@stweil stweil commented on 8bea6bc Jun 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed with PR #1633 which allows count == 0.

Please sign in to comment.