mirrors/pv
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Introduce strlcat() instead of strcat(), and replace all sizeof() of buffers inside structures with constants where possible, to avoid bugs caused by reading the size of a pointer instead of the size of the buffer it points to.

Browse Source
This commit is contained in:
Andrew Wood
2023-07-23 01:57:00 +01:00
parent 80e3f791c4
commit c7e0bb727e
12 changed files with 171 additions and 80 deletions
Download Patch File Download Diff File Expand all files Collapse all files
autoconf/configure.in
+1 -1
View File
@@ -39,7 +39,7 @@ AC_SYS_LARGEFILE
AC_HEADER_TIOCGWINSZ
AC_CHECK_FUNCS(getopt_long getopt)
AC_CHECK_HEADERS(getopt.h)
AC_CHECK_FUNCS(memcpy basename vsnprintf)
AC_CHECK_FUNCS(memcpy basename vsnprintf strlcat)
AC_CHECK_HEADERS(limits.h)
AC_ARG_ENABLE(debugging,
autoconf/header.in
+3
View File
@@ -81,6 +81,9 @@
/* Define to 1 if you have the <string.h> header file. */
#undef HAVE_STRING_H
/* Define to 1 if you have the `strlcat' function. */
#undef HAVE_STRLCAT
/* Define to 1 if you have the <sys/ipc.h> header file. */
#undef HAVE_SYS_IPC_H
doc/NEWS.md
+1
View File
@@ -8,6 +8,7 @@
* cleanup: improved the output formatting of "`make test`"
* cleanup: skip test 10 (for *SIGPIPE*) if GNU "`head`" is not available, so "`make test`" on stock OpenBSD 7.3 works
* cleanup: replace all calls to `sprintf()` and `snprintf()` with a new wrapper function `pv_snprintf()` to centralise compatibility changes
* cleanup: replace all calls to `strcat()` with a wrapper `pv_strlcat()` to improve security and compatibility
* cleanup: tidy up and fix compilation warning in "`--watchfd`" code
* cleanup: replace all `write()` calls to the terminal with a wrapper `pv_write_retry()` for consistency
src/include/pv-internal.h
+40 -19
View File
@@ -48,6 +48,27 @@ typedef struct pvhistory {
long double elapsed_sec;
} pvhistory_t;
#define PV_SIZEOF_DEFAULT_FORMAT 512
#define PV_SIZEOF_CWD 4096
#define PV_SIZEOF_LASTOUTPUT_BUFFER 256
#define PV_SIZEOF_STR_NAME 512
#define PV_SIZEOF_STR_TRANSFERRED 128
#define PV_SIZEOF_STR_BUFPERCENT 128
#define PV_SIZEOF_STR_TIMER 128
#define PV_SIZEOF_STR_RATE 128
#define PV_SIZEOF_STR_AVERAGE_RATE 128
#define PV_SIZEOF_STR_PROGRESS 1024
#define PV_SIZEOF_STR_LASTOUTPUT 512
#define PV_SIZEOF_STR_ETA 128
#define PV_SIZEOF_STR_FINETA 128
#define PV_FORMAT_ARRAY_MAX 100
#define PV_SIZEOF_CRS_LOCK_FILE 1024
#define PV_SIZEOF_FILE_FDINFO 4096
#define PV_SIZEOF_FILE_FD 4096
#define PV_SIZEOF_FILE_FDPATH 4096
#define PV_SIZEOF_DISPLAY_NAME 512
/*
* Structure for holding PV internal state. Opaque outside the PV library.
@@ -83,14 +104,14 @@ struct pvstate_s {
unsigned int width; /* screen width */
unsigned int height; /* screen height */
const char *name; /* display name */
char default_format[512]; /* default format string */
char default_format[PV_SIZEOF_DEFAULT_FORMAT]; /* default format string */
const char *format_string; /* output format string */
/******************
* Program status *
******************/
const char *program_name; /* program name for error reporting */
char cwd[4096]; /* current working directory for relative path */
char cwd[PV_SIZEOF_CWD]; /* current working directory for relative path */
const char *current_file; /* current file being read */
int exit_status; /* exit status to give (0=OK) */
@@ -132,24 +153,24 @@ struct pvstate_s {
char *display_buffer;
long display_buffer_size;
int lastoutput_length; /* number of last-output bytes to show */
unsigned char lastoutput_buffer[256];
unsigned char lastoutput_buffer[PV_SIZEOF_LASTOUTPUT_BUFFER];
int prev_width; /* screen width last time we were called */
int prev_length; /* length of last string we output */
char str_name[512];
char str_transferred[128];
char str_bufpercent[128];
char str_timer[128];
char str_rate[128];
char str_average_rate[128];
char str_progress[1024];
char str_lastoutput[512];
char str_eta[128];
char str_fineta[128];
char str_name[PV_SIZEOF_STR_NAME];
char str_transferred[PV_SIZEOF_STR_TRANSFERRED];
char str_bufpercent[PV_SIZEOF_STR_BUFPERCENT];
char str_timer[PV_SIZEOF_STR_TIMER];
char str_rate[PV_SIZEOF_STR_RATE];
char str_average_rate[PV_SIZEOF_STR_AVERAGE_RATE];
char str_progress[PV_SIZEOF_STR_PROGRESS];
char str_lastoutput[PV_SIZEOF_STR_LASTOUTPUT];
char str_eta[PV_SIZEOF_STR_ETA];
char str_fineta[PV_SIZEOF_STR_FINETA];
unsigned long components_used; /* bitmask of components used */
struct {
const char *string;
int length;
} format[100];
} format[PV_FORMAT_ARRAY_MAX];
bool display_visible; /* set once anything written to terminal */
/********************
@@ -166,7 +187,7 @@ struct pvstate_s {
bool crs_noipc; /* set if we can't use IPC */
#endif /* HAVE_IPC */
int crs_lock_fd; /* fd of lockfile, -1 if none open */
char crs_lock_file[1024];
char crs_lock_file[PV_SIZEOF_CRS_LOCK_FILE];
int crs_y_start; /* our initial Y coordinate */
/*******************
@@ -229,11 +250,11 @@ struct pvwatchfd_s {
int watch_fd; /* fd to watch, -1 = not displayed */
#ifdef __APPLE__
#else
char file_fdinfo[4096]; /* path to /proc fdinfo file */
char file_fd[4096]; /* path to /proc fd symlink */
char file_fdinfo[PV_SIZEOF_FILE_FDINFO]; /* path to /proc fdinfo file */
char file_fd[PV_SIZEOF_FILE_FD]; /* path to /proc fd symlink */
#endif
char file_fdpath[4096]; /* path to file that was opened */
char display_name[512]; /* name to show on progress bar */
char file_fdpath[PV_SIZEOF_FILE_FDPATH]; /* path to file that was opened */
char display_name[PV_SIZEOF_DISPLAY_NAME]; /* name to show on progress bar */
struct stat sb_fd; /* stat of fd symlink */
struct stat sb_fd_link; /* lstat of fd symlink */
unsigned long long size; /* size of whole file, 0 if unknown */
src/include/pv.h
+12 -1
View File
@@ -56,11 +56,22 @@ extern unsigned long long pv_getnum_ull(const char *);
extern int pv_getnum_check(const char *, pv_numtype_t);
/*
* Wrapper for sprintf(), with less safe fallbacks for systems without that
* String handling wrappers.
*/
/*
* Wrapper for sprintf(), falling back to sprintf() on systems without that
* function.
*/
extern int pv_snprintf(char *, size_t, const char *, ...);
/*
* Implementation of strlcat() where it is unavailable: append a string to a
* buffer, constraining the buffer to a particular size and ensuring
* termination with '\0'.
*/
extern size_t pv_strlcat(char *, const char *, size_t);
/*
* Main PV functions.
*/
src/main/options.c
+1
View File
@@ -148,6 +148,7 @@ opts_t opts_parse(int argc, char **argv)
/* "-s @" is valid, so allow it. */
if ('@' == *optarg)
break;
/* falls through */
case 'A':
case 'w':
case 'H':
src/pv/cursor.c
+2 -2
View File
@@ -102,9 +102,9 @@ static void pv_crs_open_lockfile(pvstate_t state, int fd)
if (!tmpdir)
tmpdir = "/tmp";
memset(state->crs_lock_file, 0, sizeof(state->crs_lock_file));
memset(state->crs_lock_file, 0, PV_SIZEOF_CRS_LOCK_FILE);
(void) pv_snprintf(state->crs_lock_file,
sizeof(state->crs_lock_file),
PV_SIZEOF_CRS_LOCK_FILE,
"%s/pv-%s-%i.lock", tmpdir, basename(ttydev),
(int) geteuid());
src/pv/display.c
+51 -42
View File
@@ -286,11 +286,12 @@ static void pv__format_init(pvstate_t state)
state->str_average_rate[0] = 0;
state->str_progress[0] = 0;
state->str_eta[0] = 0;
memset(state->format, 0, sizeof(state->format));
memset(state->format, 0,
PV_FORMAT_ARRAY_MAX * sizeof(state->format[0]));
if (state->name) {
(void) pv_snprintf(state->str_name,
sizeof(state->str_name), "%9.500s:",
PV_SIZEOF_STR_NAME, "%9.500s:",
state->name);
}
@@ -364,10 +365,8 @@ static void pv__format_init(pvstate_t state)
state->format[segment].string =
state->str_lastoutput;
state->format[segment].length = 0;
if (num > sizeof(state->lastoutput_buffer))
num =
sizeof
(state->lastoutput_buffer);
if (num > PV_SIZEOF_LASTOUTPUT_BUFFER)
num = PV_SIZEOF_LASTOUTPUT_BUFFER;
if (num < 1)
num = 1;
state->lastoutput_length = num;
@@ -658,15 +657,13 @@ static const char *pv__format(pvstate_t state,
if ((state->components_used & PV_DISPLAY_BYTES) != 0) {
if (state->bits) {
(void) pv_snprintf(state->display_buffer,
sizeof
(state->display_buffer),
state->display_buffer_size,
"%.99s%lld\n",
numericprefix,
8 * total_bytes);
} else {
(void) pv_snprintf(state->display_buffer,
sizeof
(state->display_buffer),
state->display_buffer_size,
"%.99s%lld\n",
numericprefix,
total_bytes);
@@ -702,12 +699,12 @@ static const char *pv__format(pvstate_t state,
if ((state->components_used & PV_DISPLAY_BYTES) != 0) {
if (state->bits && !state->linemode) {
pv__sizestr(state->str_transferred,
sizeof(state->str_transferred), "%s",
PV_SIZEOF_STR_TRANSFERRED, "%s",
(long double) total_bytes * 8, "",
_("b"), 1);
} else {
pv__sizestr(state->str_transferred,
sizeof(state->str_transferred), "%s",
PV_SIZEOF_STR_TRANSFERRED, "%s",
(long double) total_bytes, "", _("B"),
state->linemode ? 0 : 1);
}
@@ -717,7 +714,7 @@ static const char *pv__format(pvstate_t state,
if ((state->components_used & PV_DISPLAY_BUFPERCENT) != 0) {
if (state->buffer_size > 0)
(void) pv_snprintf(state->str_bufpercent,
sizeof(state->str_bufpercent),
PV_SIZEOF_STR_BUFPERCENT,
"{%3ld%%}",
pv__calc_percentage
(state->read_position -
@@ -726,7 +723,7 @@ static const char *pv__format(pvstate_t state,
#ifdef HAVE_SPLICE
if (state->splice_used)
(void) pv_snprintf(state->str_bufpercent,
sizeof(state->str_bufpercent),
PV_SIZEOF_STR_BUFPERCENT,
"{%s}", "----");
#endif
}
@@ -747,7 +744,7 @@ static const char *pv__format(pvstate_t state,
*/
if (elapsed_sec > (long double) 86400.0L) {
(void) pv_snprintf(state->str_timer,
sizeof(state->str_timer),
PV_SIZEOF_STR_TIMER,
"%ld:%02ld:%02ld:%02ld",
((long) elapsed_sec) / 86400,
(((long) elapsed_sec) / 3600) %
@@ -756,7 +753,7 @@ static const char *pv__format(pvstate_t state,
60, ((long) elapsed_sec) % 60);
} else {
(void) pv_snprintf(state->str_timer,
sizeof(state->str_timer),
PV_SIZEOF_STR_TIMER,
"%ld:%02ld:%02ld",
((long) elapsed_sec) / 3600,
(((long) elapsed_sec) / 60) %
@@ -768,11 +765,11 @@ static const char *pv__format(pvstate_t state,
if ((state->components_used & PV_DISPLAY_RATE) != 0) {
if (state->bits && !state->linemode) {
pv__sizestr(state->str_rate,
sizeof(state->str_rate), "[%s]",
PV_SIZEOF_STR_RATE, "[%s]",
8 * rate, "", _("b/s"), 1);
} else {
pv__sizestr(state->str_rate,
sizeof(state->str_rate), "[%s]", rate,
PV_SIZEOF_STR_RATE, "[%s]", rate,
_("/s"), _("B/s"),
state->linemode ? 0 : 1);
}
@@ -782,12 +779,12 @@ static const char *pv__format(pvstate_t state,
if ((state->components_used & PV_DISPLAY_AVERAGERATE) != 0) {
if (state->bits && !state->linemode) {
pv__sizestr(state->str_average_rate,
sizeof(state->str_average_rate),
PV_SIZEOF_STR_AVERAGE_RATE,
"[%s]", 8 * average_rate, "", _("b/s"),
1);
} else {
pv__sizestr(state->str_average_rate,
sizeof(state->str_average_rate),
PV_SIZEOF_STR_AVERAGE_RATE,
"[%s]", average_rate, _("/s"),
_("B/s"), state->linemode ? 0 : 1);
}
@@ -824,14 +821,14 @@ static const char *pv__format(pvstate_t state,
*/
if (eta > 86400L) {
(void) pv_snprintf(state->str_eta,
sizeof(state->str_eta),
PV_SIZEOF_STR_ETA,
"%.16s %ld:%02ld:%02ld:%02ld",
_("ETA"), eta / 86400,
(eta / 3600) % 24,
(eta / 60) % 60, eta % 60);
} else {
(void) pv_snprintf(state->str_eta,
sizeof(state->str_eta),
PV_SIZEOF_STR_ETA,
"%.16s %ld:%02ld:%02ld",
_("ETA"), eta / 3600,
(eta / 60) % 60, eta % 60);
@@ -843,8 +840,8 @@ static const char *pv__format(pvstate_t state,
*/
if (bytes_since_last < 0) {
unsigned int i;
for (i = 0; i < sizeof(state->str_eta)
&& state->str_eta[i] != 0; i++) {
for (i = 0; i < PV_SIZEOF_STR_ETA
&& state->str_eta[i] != '\0'; i++) {
state->str_eta[i] = ' ';
}
}
@@ -896,19 +893,19 @@ static const char *pv__format(pvstate_t state,
struct tm time = *time_ptr;
(void) pv_snprintf(state->str_fineta,
sizeof(state->str_fineta),
PV_SIZEOF_STR_FINETA,
"%.16s ", _("ETA"));
strftime(state->str_fineta +
strlen(state->str_fineta),
sizeof(state->str_fineta) - 1 -
PV_SIZEOF_STR_FINETA - 1 -
strlen(state->str_fineta), time_format,
&time);
}
if (!show_eta) {
unsigned int i;
for (i = 0; i < sizeof(state->str_fineta)
&& state->str_fineta[i] != 0; i++) {
for (i = 0; i < PV_SIZEOF_STR_FINETA
&& state->str_fineta[i] != '\0'; i++) {
state->str_fineta[i] = ' ';
}
}
@@ -959,26 +956,32 @@ static const char *pv__format(pvstate_t state,
available_width = 0;
if (available_width >
(int) (sizeof(state->str_progress)) - 16)
(int) (PV_SIZEOF_STR_PROGRESS) - 16)
available_width =
sizeof(state->str_progress) - 16;
PV_SIZEOF_STR_PROGRESS - 16;
for (i = 0;
i <
(available_width * state->percentage) / 100 -
1; i++) {
if (i < available_width)
strcat(state->str_progress, "=");
pv_strlcat(state->str_progress,
"=",
PV_SIZEOF_STR_PROGRESS);
}
if (i < available_width) {
strcat(state->str_progress, ">");
pv_strlcat(state->str_progress, ">",
PV_SIZEOF_STR_PROGRESS);
i++;
}
for (; i < available_width; i++) {
strcat(state->str_progress, " ");
pv_strlcat(state->str_progress, " ",
PV_SIZEOF_STR_PROGRESS);
}
strcat(state->str_progress, "] ");
strcat(state->str_progress, pct);
pv_strlcat(state->str_progress, "] ",
PV_SIZEOF_STR_PROGRESS);
pv_strlcat(state->str_progress, pct,
PV_SIZEOF_STR_PROGRESS);
} else {
int p = state->percentage;
@@ -989,9 +992,9 @@ static const char *pv__format(pvstate_t state,
available_width = 0;
if (available_width >
(int) (sizeof(state->str_progress)) - 16)
(int) (PV_SIZEOF_STR_PROGRESS) - 16)
available_width =
sizeof(state->str_progress) - 16;
PV_SIZEOF_STR_PROGRESS - 16;
debug("available_width: %d", available_width);
@@ -999,13 +1002,18 @@ static const char *pv__format(pvstate_t state,
p = 200 - p;
for (i = 0; i < (available_width * p) / 100; i++) {
if (i < available_width)
strcat(state->str_progress, " ");
pv_strlcat(state->str_progress,
" ",
PV_SIZEOF_STR_PROGRESS);
}
strcat(state->str_progress, "<=>");
pv_strlcat(state->str_progress, "<=>",
PV_SIZEOF_STR_PROGRESS);
for (; i < available_width; i++) {
strcat(state->str_progress, " ");
pv_strlcat(state->str_progress, " ",
PV_SIZEOF_STR_PROGRESS);
}
strcat(state->str_progress, "]");
pv_strlcat(state->str_progress, "]",
PV_SIZEOF_STR_PROGRESS);
}
/*
@@ -1072,7 +1080,8 @@ static const char *pv__format(pvstate_t state,
while (--spaces_to_add >= 0) {
spaces[spaces_to_add] = ' ';
}
strcat(state->display_buffer, spaces);
pv_strlcat(state->display_buffer, spaces,
state->display_buffer_size);
}
state->prev_width = state->width;
state->prev_length = output_length;
src/pv/loop.c
+2 -3
View File
@@ -560,10 +560,9 @@ int pv_watchpid_loop(pvstate_t state)
new_format_string[sizeof(new_format_string) - 1] = '\0';
state_copy.format_string = NULL;
(void) pv_snprintf(state_copy.default_format,
sizeof(state_copy.default_format),
PV_SIZEOF_DEFAULT_FORMAT,
"%.510s", new_format_string);
state_copy.default_format[sizeof(state_copy.default_format) - 1] =
'\0';
state_copy.default_format[PV_SIZEOF_DEFAULT_FORMAT - 1] = '\0';
/*
* Get things ready for the main loop.
src/pv/state.c
+4 -3
View File
@@ -68,7 +68,7 @@ pvstate_t pv_state_alloc(const char *program_name)
* Get the current working directory, if possible, as a base for
* showing relative filenames with --watchfd.
*/
if (NULL == getcwd(state->cwd, sizeof(state->cwd))) {
if (NULL == getcwd(state->cwd, PV_SIZEOF_CWD - 1)) {
/* failed - will always show full path */
state->cwd[0] = '\0';
}
@@ -76,6 +76,7 @@ pvstate_t pv_state_alloc(const char *program_name)
/* CWD is root directory - always show full path */
state->cwd[0] = '\0';
}
state->cwd[PV_SIZEOF_CWD - 1] = '\0';
return state;
}
@@ -119,8 +120,8 @@ void pv_state_set_format(pvstate_t state, bool progress,
{
#define PV_ADDFORMAT(x,y) if (x) { \
if (state->default_format[0] != '\0') \
strcat(state->default_format, " "); \
strcat(state->default_format, y); \
pv_strlcat(state->default_format, " ", sizeof(state->default_format)); \
pv_strlcat(state->default_format, y, sizeof(state->default_format)); \
}
state->default_format[0] = '\0';
src/pv/string.c
+46 -1
View File
@@ -11,9 +11,11 @@
#include <stdio.h>
#include <stdarg.h>
#include <string.h>
/*
* Wrapper for sprintf(), with less safe fallbacks for systems without that
* Wrapper for sprintf(), falling back to sprintf() on systems without that
* function.
*
* Returns -1 if "str" or "format" are NULL or if "size" is 0.
@@ -49,4 +51,47 @@ int pv_snprintf(char *str, size_t size, const char *format, ...)
return ret;
}
/*
* Implementation of strlcat() where it is unavailable: append a string to a
* buffer, constraining the buffer to a particular size and ensuring
* termination with '\0'.
*
* Appends the string "src" to the buffer "dst", assuming "dst" is "dstsize"
* bytes long, and ensuring that "dst" is always terminated with a '\0'
* byte.
*
* Returns the intended length of the string, not including the terminating
* '\0', i.e. strlen(src)+strlen(dst), regardless of whether truncation
* occurred.
*
* Note that this implementation has the side effect that "dst" will always
* be terminated with a '\0' even if "src" was zero bytes long.
*/
size_t pv_strlcat(char *dst, const char *src, size_t dstsize)
{
#ifdef HAVE_STRLCAT
return strlcat(dst, src, dstsize);
#else
size_t dstlen, srclen, available;
if (NULL == dst)
return 0;
if (NULL == src)
return 0;
if (0 == dstsize)
return 0;
dst[dstsize - 1] = '\0';
dstlen = strlen(dst);
srclen = strlen(src);
available = dstsize - dstlen;
if (available > 1)
(void) pv_snprintf(dst + dstlen, available, "%.*s",
available - 1, src);
return dstlen + srclen;
#endif
}
/* EOF */
src/pv/watchpid.c
+8 -8
View File
@@ -89,7 +89,7 @@ int pv_watchfd_info(pvstate_t state, pvwatchfd_t info, int automatic)
}
strlcpy(info->file_fdpath, vnodeInfo.pvip.vip_path,
sizeof(info->file_fdpath));
PV_SIZEOF_FILE_FDPATH);
info->size = 0;
@@ -152,17 +152,17 @@ int pv_watchfd_info(pvstate_t state, pvwatchfd_t info, int automatic)
info->watch_pid, strerror(errno));
return 1;
}
(void) pv_snprintf(info->file_fdinfo, sizeof(info->file_fdinfo),
(void) pv_snprintf(info->file_fdinfo, PV_SIZEOF_FILE_FDINFO,
"/proc/%u/fdinfo/%d", info->watch_pid,
info->watch_fd);
(void) pv_snprintf(info->file_fd, sizeof(info->file_fd),
(void) pv_snprintf(info->file_fd, PV_SIZEOF_FILE_FD,
"/proc/%u/fd/%d", info->watch_pid,
info->watch_fd);
memset(info->file_fdpath, 0, sizeof(info->file_fdpath));
memset(info->file_fdpath, 0, PV_SIZEOF_FILE_FDPATH);
if (readlink
(info->file_fd, info->file_fdpath,
sizeof(info->file_fdpath) - 1) < 0) {
PV_SIZEOF_FILE_FDPATH - 1) < 0) {
if (!automatic)
pv_error(state, "%s %u: %s %d: %s",
_("pid"),
@@ -527,7 +527,7 @@ void pv_watchpid_setname(pvstate_t state, pvwatchfd_t info)
int path_length, cwd_length, max_display_length;
char *file_fdpath = info->file_fdpath;
memset(info->display_name, 0, sizeof(info->display_name));
memset(info->display_name, 0, PV_SIZEOF_DISPLAY_NAME);
path_length = strlen(info->file_fdpath);
cwd_length = strlen(state->cwd);
@@ -542,7 +542,7 @@ void pv_watchpid_setname(pvstate_t state, pvwatchfd_t info)
max_display_length = (state->width / 2) - 6;
if (max_display_length >= path_length) {
(void) pv_snprintf(info->display_name,
sizeof(info->display_name),
PV_SIZEOF_DISPLAY_NAME,
"%4d:%.498s", info->watch_fd,
file_fdpath);
} else {
@@ -552,7 +552,7 @@ void pv_watchpid_setname(pvstate_t state, pvwatchfd_t info)
suffix_length = max_display_length - prefix_length - 3;
(void) pv_snprintf(info->display_name,
sizeof(info->display_name),
PV_SIZEOF_DISPLAY_NAME,
"%4d:%.*s...%.*s",
info->watch_fd, prefix_length,
file_fdpath, suffix_length,