Skip to content
This repository was archived by the owner on Oct 5, 2023. It is now read-only.

Commit de776d4

Browse files
committed
Check for integer overflow in sfntly::FontData::Bound().
Also delete dead code and cleanup some nits. This is cl/96914065.
1 parent 1ed8c82 commit de776d4

File tree

3 files changed

+60
-30
lines changed

3 files changed

+60
-30
lines changed

Diff for: cpp/src/sfntly/data/font_data.cc

+27-17
Original file line numberDiff line numberDiff line change
@@ -14,33 +14,43 @@
1414
* limitations under the License.
1515
*/
1616

17-
#include <limits.h>
17+
#include "sfntly/data/font_data.h"
18+
1819
#include <algorithm>
1920
#include <functional>
21+
#include <limits>
2022

21-
#include "sfntly/data/font_data.h"
23+
#include "sfntly/port/logging.h"
2224

2325
namespace sfntly {
2426

2527
int32_t FontData::Size() const {
2628
return std::min<int32_t>(array_->Size() - bound_offset_, bound_length_);
2729
}
2830

29-
bool FontData::Bound(int32_t offset, int32_t length) {
30-
if (offset + length > Size() || offset < 0 || length < 0)
31-
return false;
32-
33-
bound_offset_ += offset;
31+
void FontData::Bound(int32_t offset, int32_t length) {
32+
// Inputs should not be negative.
33+
CHECK(offset >= 0);
34+
CHECK(length >= 0);
35+
36+
// Check to make sure |bound_offset_| will not overflow.
37+
CHECK(bound_offset_ <= std::numeric_limits<int32_t>::max() - offset);
38+
const int32_t new_offset = bound_offset_ + offset;
39+
40+
if (length == GROWABLE_SIZE) {
41+
// When |length| has the special value of GROWABLE_SIZE, it means the size
42+
// should not have any artificial limits, thus it is just the underlying
43+
// |array_|'s size. Just make sure |new_offset| is still within bounds.
44+
CHECK(new_offset <= array_->Size());
45+
} else {
46+
// When |length| has any other value, |new_offset| + |length| points to the
47+
// end of the array. Make sure that is within bounds, but use subtraction to
48+
// avoid an integer overflow.
49+
CHECK(new_offset <= array_->Size() - length);
50+
}
51+
52+
bound_offset_ = new_offset;
3453
bound_length_ = length;
35-
return true;
36-
}
37-
38-
bool FontData::Bound(int32_t offset) {
39-
if (offset > Size() || offset < 0)
40-
return false;
41-
42-
bound_offset_ += offset;
43-
return true;
4454
}
4555

4656
int32_t FontData::Length() const {
@@ -60,7 +70,7 @@ FontData::FontData(FontData* data, int32_t offset) {
6070
Init(data->array_);
6171
Bound(data->bound_offset_ + offset,
6272
(data->bound_length_ == GROWABLE_SIZE)
63-
? GROWABLE_SIZE : data->bound_length_ - offset);
73+
? GROWABLE_SIZE : data->bound_length_ - offset);
6474
}
6575

6676
FontData::~FontData() {}

Diff for: cpp/src/sfntly/data/font_data.h

+2-13
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,9 @@
1919

2020
#include <limits.h>
2121

22-
#include <vector>
23-
24-
#include "sfntly/port/type.h"
2522
#include "sfntly/data/byte_array.h"
2623
#include "sfntly/port/refcount.h"
24+
#include "sfntly/port/type.h"
2725

2826
namespace sfntly {
2927

@@ -60,16 +58,7 @@ class FontData : virtual public RefCount {
6058
// visible within the bounds set.
6159
// @param offset the start of the new bounds
6260
// @param length the number of bytes in the bounded array
63-
// @return true if the bounding range was successful; false otherwise
64-
virtual bool Bound(int32_t offset, int32_t length);
65-
66-
// Sets limits on the size of the FontData. This is a offset bound only so if
67-
// the FontData is writable and growable then there is no limit to that growth
68-
// from the bounding operation.
69-
// @param offset the start of the new bounds which must be within the current
70-
// size of the FontData
71-
// @return true if the bounding range was successful; false otherwise
72-
virtual bool Bound(int32_t offset);
61+
virtual void Bound(int32_t offset, int32_t length);
7362

7463
// Makes a slice of this FontData. The returned slice will share the data with
7564
// the original <code>FontData</code>.

Diff for: cpp/src/sfntly/port/logging.h

+31
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
/*
2+
* Copyright 2015 Google Inc. All Rights Reserved.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
#ifndef SFNTLY_CPP_SRC_SFNTLY_PORT_LOGGING_H_
18+
#define SFNTLY_CPP_SRC_SFNTLY_PORT_LOGGING_H_
19+
20+
#include <stdio.h>
21+
#include <stdlib.h>
22+
23+
// Cheap base/logging.h knock off.
24+
25+
#define CHECK(expr) \
26+
if (!(expr)) { \
27+
printf("CHECK failed\n"); \
28+
abort(); \
29+
}
30+
31+
#endif // SFNTLY_CPP_SRC_SFNTLY_PORT_LOGGING_H_

0 commit comments

Comments
 (0)