A date validation bug in Chromium


#1

A blog post about a bug found by PVS-Studio in Chromium:

The C++ code that contains the bug:

static const int kDaysInMonth[13] = {
  0, 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31
};

bool ValidateDateTime(const DateTime& time) {
  if (time.year < 1 || time.year > 9999 ||
      time.month < 1 || time.month > 12 ||
      time.day < 1 || time.day > 31 ||
      time.hour < 0 || time.hour > 23 ||
      time.minute < 0 || time.minute > 59 ||
      time.second < 0 || time.second > 59) {
    return false;
  }
  if (time.month == 2 && IsLeapYear(time.year)) {
    return time.month <= kDaysInMonth[time.month] + 1;
  } else {
    return time.month <= kDaysInMonth[time.month];
  }
}

At the end of the blog post Andrey Karpov asks: “How can we improve our style to ward off errors like that?”

My answer is to write Rust-like code like this:

type Year =   u16[1 ... 9_999];
type Month =   u8[1 ... 12];
type Day =     u8[1 ... 31];
type Hour =    u8[0 ... 23];
type Minute =  u8[0 ... 59];
type Second =  u8[0 ... 59];

#[only_impl_new]
struct DateTime {
    year:   Year,
    month:  Month,
    day:    Day,
    hour:   Hour,
    minute: Minute,
    second: Second,
}

impl Year {
    fn is_leap_year(&self) -> bool {
        // implementation ...
        true
    }
}

impl DateTime {
    const DAYS_IN_MONTH: [Day; Month] =
        [31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31];

    fn new(year: Year,
           month: Month,
           day: Day,
           hour: Hour,
           minute: Minute,
           second: Second) -> Option<Self> {
        // This leap is inferred of type Day.
        let leap = if month == 2 && year.is_leap_year() { 1 } else { 0 };

        if time.day <= Self::DAYS_IN_MONTH[month] + leap {
            Some(DateTime(year, month, day, hour, minute, second))
        } else {
            None
        }
    }
}

It shows some ideas:

  • This code is untested and it’s meant just to show rough ideas, its syntaxes are far from final;
  • Newtypes like Year are ranged, they can’t be built with values outside their range (the compiler adds run-time or compile-time tests to verify their range, so you don’t need to verify their range manually);
  • #[only_impl_new] means that you can build a DateTime only through the new() function (this can be done today in Rust using privacy and modules, but it requires more boilerplate);
  • is_leap_year is a method;
  • No need to validate a DateTime, its correctness is verified when you build it;
  • All types are strong so you can’t mismatch them as in the blog post;
  • DAYS_IN_MONTH is an array (it’s not an associative array), it contains values of type Day and it’s indexed by indices of type Month. There’s little space for bugs here;
  • Year is still always > 0, so you need a different time data structure if you want to manage historical times…

#2

#3
if (time.month == 2 && IsLeapYear(time.year)) {
    return time.month <= kDaysInMonth[time.month] + 1;
  } else {
    return time.month <= kDaysInMonth[time.month];
  }

A simple improvement to this, which might make the bug more obvious in the first place, is to create a function int num_days(int month) instead of accessing the array directly. The array name has “month” in it and I think that can fool the eyes.

The function also has a lot of noise - if it was factored some then it can also improve the odds of catching this visually.

I agree that modeling the types with newtypes would improve things. C++ can do this too, albeit with more syntax. I can totally see this bug happening in today’s Rust code as well. Although newtypes are fairly ergonomic, it’s still some ceremony to write them - people will just use the primitives instead. Then there’s also the type complexity aspect - each new named type adds to the domain of types readers (and users) need to understand. I’ve seen people newtype way too much (not necessarily in Rust) and it’s its own kind of a mess. The “trick” is to know what concepts deserve such wrappers and which don’t.

I’m impressed that PVS Studio can find this type of bug - kudos to them.


#4

The shown Rust code contains three different ideas, the interval types is one of them. The other two are the strongly typed indexed array:

const DAYS_IN_MONTH: [Day; Month] =

And an annotation to disallow creating a struct without using its new function, that is, disallow struct literals, etc. (This can be done today with modules and privacy, but it’s less handy).