From 8bea6bcc12928daf58d7f5f6370c3a548bd2801c Mon Sep 17 00:00:00 2001
From: Stefan Weil <sw@weilnetz.de>
Date: Mon, 4 Jun 2018 13:52:38 +0200
Subject: [PATCH] TFile: Improve handling of potential integer overflow

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>
---
 src/ccutil/serialis.cpp | 28 +++++++++++++++++++---------
 src/ccutil/serialis.h   |  6 +++---
 2 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/src/ccutil/serialis.cpp b/src/ccutil/serialis.cpp
index ddba5fb2..5121ac82 100644
--- a/src/ccutil/serialis.cpp
+++ b/src/ccutil/serialis.cpp
@@ -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);
@@ -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;
@@ -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;
 }
diff --git a/src/ccutil/serialis.h b/src/ccutil/serialis.h
index f7b63e36..de9e0eb8 100644
--- a/src/ccutil/serialis.h
+++ b/src/ccutil/serialis.h
@@ -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();
@@ -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.