Skip to content

Commit

Permalink
Fix potential buffer overflows when calling sprintf
Browse files Browse the repository at this point in the history
1. Replace sprintf with snprintf, which is safer
2. An unsigned int or unsigned long int requires 11 bytes to print
   (including the null terminator)
3. Use PRIu16 macro to print uint16_t
4. Format string "#%2d %2d:%02d:%02d.%02d\n" in
   StopWatch::stopLapBtnEventHandler() requires at least 17 bytes.
   The 16-byte buffer would clearly be overrun if sprintf were used.
  • Loading branch information
szsam committed Apr 21, 2023
1 parent 40f7e1c commit fdf24db
Show file tree
Hide file tree
Showing 5 changed files with 12 additions and 12 deletions.
4 changes: 2 additions & 2 deletions src/components/datetime/DateTimeController.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -140,9 +140,9 @@ std::string DateTime::FormattedTime() {
hour12 = (hour == 12) ? 12 : hour - 12;
amPmStr = "PM";
}
sprintf(buff, "%i:%02i %s", hour12, minute, amPmStr);
snprintf(buff, sizeof(buff), "%i:%02i %s", hour12, minute, amPmStr);
} else {
sprintf(buff, "%02i:%02i", hour, minute);
snprintf(buff, sizeof(buff), "%02i:%02i", hour, minute);
}
return std::string(buff);
}
6 changes: 3 additions & 3 deletions src/displayapp/screens/StopWatch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -198,11 +198,11 @@ void StopWatch::stopLapBtnEventHandler() {
continue;
}
TimeSeparated_t times = convertTicksToTimeSegments(laps[i]);
char buffer[16];
char buffer[17];
if (times.hours == 0) {
sprintf(buffer, "#%2d %2d:%02d.%02d\n", i + 1, times.mins, times.secs, times.hundredths);
snprintf(buffer, sizeof(buffer), "#%2d %2d:%02d.%02d\n", i + 1, times.mins, times.secs, times.hundredths);
} else {
sprintf(buffer, "#%2d %2d:%02d:%02d.%02d\n", i + 1, times.hours, times.mins, times.secs, times.hundredths);
snprintf(buffer, sizeof(buffer), "#%2d %2d:%02d:%02d.%02d\n", i + 1, times.hours, times.mins, times.secs, times.hundredths);
}
lv_label_ins_text(lapText, LV_LABEL_POS_LAST, buffer);
}
Expand Down
8 changes: 4 additions & 4 deletions src/displayapp/screens/SystemInfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -234,9 +234,9 @@ std::unique_ptr<Screen> SystemInfo::CreateScreen4() {
auto nb = uxTaskGetSystemState(tasksStatus, maxTaskCount, nullptr);
std::sort(tasksStatus, tasksStatus + nb, sortById);
for (uint8_t i = 0; i < nb && i < maxTaskCount; i++) {
char buffer[7] = {0};
char buffer[11] = {0};

sprintf(buffer, "%lu", tasksStatus[i].xTaskNumber);
snprintf(buffer, sizeof(buffer), "%lu", tasksStatus[i].xTaskNumber);
lv_table_set_cell_value(infoTask, i + 1, 0, buffer);
switch (tasksStatus[i].eCurrentState) {
case eReady:
Expand All @@ -260,9 +260,9 @@ std::unique_ptr<Screen> SystemInfo::CreateScreen4() {
lv_table_set_cell_value(infoTask, i + 1, 1, buffer);
lv_table_set_cell_value(infoTask, i + 1, 2, tasksStatus[i].pcTaskName);
if (tasksStatus[i].usStackHighWaterMark < 20) {
sprintf(buffer, "%d low", tasksStatus[i].usStackHighWaterMark);
snprintf(buffer, sizeof(buffer), "%" PRIu16 " low", tasksStatus[i].usStackHighWaterMark);
} else {
sprintf(buffer, "%d", tasksStatus[i].usStackHighWaterMark);
snprintf(buffer, sizeof(buffer), "%" PRIu16, tasksStatus[i].usStackHighWaterMark);
}
lv_table_set_cell_value(infoTask, i + 1, 3, buffer);
}
Expand Down
4 changes: 2 additions & 2 deletions src/displayapp/screens/Twos.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -241,8 +241,8 @@ void Twos::updateGridDisplay() {
const unsigned int row = i / nCols;
const unsigned int col = i % nCols;
if (grid[row][col].value > 0) {
char buffer[7];
sprintf(buffer, "%d", grid[row][col].value);
char buffer[11];
snprintf(buffer, sizeof(buffer), "%u", grid[row][col].value);
lv_table_set_cell_value(gridDisplay, row, col, buffer);
} else {
lv_table_set_cell_value(gridDisplay, row, col, "");
Expand Down
2 changes: 1 addition & 1 deletion src/displayapp/screens/settings/SettingDisplay.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ SettingDisplay::SettingDisplay(Pinetime::Applications::DisplayApp* app, Pinetime
char buffer[12];
for (unsigned int i = 0; i < options.size(); i++) {
cbOption[i] = lv_checkbox_create(container1, nullptr);
sprintf(buffer, "%2ds", options[i] / 1000);
snprintf(buffer, sizeof(buffer), "%2" PRIu16 "s", options[i] / 1000);
lv_checkbox_set_text(cbOption[i], buffer);
cbOption[i]->user_data = this;
lv_obj_set_event_cb(cbOption[i], event_handler);
Expand Down

0 comments on commit fdf24db

Please sign in to comment.