[Asterisk-code-review] astfd: Fix buffer overflow in DEBUG FD LEAKS. (asterisk[master])

Joshua Colp asteriskteam at digium.com
Thu Jul 2 07:51:55 CDT 2015


Joshua Colp has submitted this change and it was merged.

Change subject: astfd: Fix buffer overflow in DEBUG_FD_LEAKS.
......................................................................


astfd: Fix buffer overflow in DEBUG_FD_LEAKS.

If DEBUG_FD_LEAKS was used and more file descriptors than the default of
1024 were available, some DEBUG_FD_LEAKS-patched functions would
overwrite memory past the fixed-size (1024) fdleaks buffer.

This change:
- adds bounds checks to __ast_fdleak_fopen and __ast_fdleak_pipe
- consistently uses ARRAY_LEN() instead of sizeof() or 1023 or 1024
- stores pointers to constants instead of copying the contents
- reorders the fdleaks struct for possibly tighter packing
- adds a tiny bit of documentation

ASTERISK-25212 #close

Change-Id: Iacb69e7701c0f0a113786bd946cea5b6335a85e5
---
M main/astfd.c
1 file changed, 37 insertions(+), 22 deletions(-)

Approvals:
  Matt Jordan: Looks good to me, but someone else must approve
  Joshua Colp: Looks good to me, approved; Verified



diff --git a/main/astfd.c b/main/astfd.c
index 72c5761..7467376 100644
--- a/main/astfd.c
+++ b/main/astfd.c
@@ -48,19 +48,24 @@
 #include "asterisk/unaligned.h"
 
 static struct fdleaks {
-	char file[40];
+	const char *callname;
 	int line;
-	char function[25];
-	char callname[10];
-	char callargs[60];
 	unsigned int isopen:1;
+	char file[40];
+	char function[25];
+	char callargs[60];
 } fdleaks[1024] = { { "", }, };
 
+/* COPY does ast_copy_string(dst, src, sizeof(dst)), except:
+ * - if it doesn't fit, it copies the value after the slash
+ *   (possibly truncated)
+ * - if there is no slash, it copies the value with the head
+ *   truncated */
 #define	COPY(dst, src)                                             \
 	do {                                                           \
 		int dlen = sizeof(dst), slen = strlen(src);                \
 		if (slen + 1 > dlen) {                                     \
-			char *slash = strrchr(src, '/');                       \
+			const char *slash = strrchr(src, '/');                 \
 			if (slash) {                                           \
 				ast_copy_string(dst, slash + 1, dlen);             \
 			} else {                                               \
@@ -72,12 +77,15 @@
 	} while (0)
 
 #define STORE_COMMON(offset, name, ...)     \
-	COPY(fdleaks[offset].file, file);       \
-	fdleaks[offset].line = line;            \
-	COPY(fdleaks[offset].function, func);   \
-	strcpy(fdleaks[offset].callname, name); \
-	snprintf(fdleaks[offset].callargs, sizeof(fdleaks[offset].callargs), __VA_ARGS__); \
-	fdleaks[offset].isopen = 1;
+	do { \
+		struct fdleaks *tmp = &fdleaks[offset]; \
+		COPY(tmp->file, file);      \
+		tmp->line = line;           \
+		COPY(tmp->function, func);  \
+		tmp->callname = name;       \
+		snprintf(tmp->callargs, sizeof(tmp->callargs), __VA_ARGS__); \
+		tmp->isopen = 1;            \
+	} while (0)
 
 #undef open
 int __ast_fdleak_open(const char *file, int line, const char *func, const char *path, int flags, ...)
@@ -91,7 +99,7 @@
 		mode = va_arg(ap, int);
 		va_end(ap);
 		res = open(path, flags, mode);
-		if (res > -1 && res < (sizeof(fdleaks) / sizeof(fdleaks[0]))) {
+		if (res > -1 && res < ARRAY_LEN(fdleaks)) {
 			char sflags[80];
 			snprintf(sflags, sizeof(sflags), "O_CREAT%s%s%s%s%s%s%s%s",
 				flags & O_APPEND ? "|O_APPEND" : "",
@@ -115,7 +123,7 @@
 		}
 	} else {
 		res = open(path, flags);
-		if (res > -1 && res < (sizeof(fdleaks) / sizeof(fdleaks[0]))) {
+		if (res > -1 && res < ARRAY_LEN(fdleaks)) {
 			STORE_COMMON(res, "open", "\"%s\",%d", path, flags);
 		}
 	}
@@ -130,7 +138,9 @@
 		return res;
 	}
 	for (i = 0; i < 2; i++) {
-		STORE_COMMON(fds[i], "pipe", "{%d,%d}", fds[0], fds[1]);
+		if (fds[i] > -1 && fds[i] < ARRAY_LEN(fdleaks)) {
+			STORE_COMMON(fds[i], "pipe", "{%d,%d}", fds[0], fds[1]);
+		}
 	}
 	return 0;
 }
@@ -141,7 +151,7 @@
 	char sdomain[20], stype[20], *sproto = NULL;
 	struct protoent *pe;
 	int res = socket(domain, type, protocol);
-	if (res < 0 || res > 1023) {
+	if (res < 0 || res >= ARRAY_LEN(fdleaks)) {
 		return res;
 	}
 
@@ -183,7 +193,7 @@
 int __ast_fdleak_close(int fd)
 {
 	int res = close(fd);
-	if (!res && fd > -1 && fd < 1024) {
+	if (!res && fd > -1 && fd < ARRAY_LEN(fdleaks)) {
 		fdleaks[fd].isopen = 0;
 	}
 	return res;
@@ -198,7 +208,9 @@
 		return res;
 	}
 	fd = fileno(res);
-	STORE_COMMON(fd, "fopen", "\"%s\",\"%s\"", path, mode);
+	if (fd > -1 && fd < ARRAY_LEN(fdleaks)) {
+		STORE_COMMON(fd, "fopen", "\"%s\",\"%s\"", path, mode);
+	}
 	return res;
 }
 
@@ -211,7 +223,7 @@
 	}
 
 	fd = fileno(ptr);
-	if ((res = fclose(ptr)) || fd < 0 || fd > 1023) {
+	if ((res = fclose(ptr)) || fd < 0 || fd >= ARRAY_LEN(fdleaks)) {
 		return res;
 	}
 	fdleaks[fd].isopen = 0;
@@ -222,10 +234,13 @@
 int __ast_fdleak_dup2(int oldfd, int newfd, const char *file, int line, const char *func)
 {
 	int res = dup2(oldfd, newfd);
-	if (res < 0 || res > 1023) {
+	if (res < 0 || res >= ARRAY_LEN(fdleaks)) {
 		return res;
 	}
-	STORE_COMMON(res, "dup2", "%d,%d", oldfd, newfd);
+	/* On success, newfd will be closed automatically if it was already
+	 * open. We don't need to mention anything about that, we're updating
+	 * the value anway. */
+	STORE_COMMON(res, "dup2", "%d,%d", oldfd, newfd); /* res == newfd */
 	return res;
 }
 
@@ -233,7 +248,7 @@
 int __ast_fdleak_dup(int oldfd, const char *file, int line, const char *func)
 {
 	int res = dup(oldfd);
-	if (res < 0 || res > 1023) {
+	if (res < 0 || res >= ARRAY_LEN(fdleaks)) {
 		return res;
 	}
 	STORE_COMMON(res, "dup2", "%d", oldfd);
@@ -263,7 +278,7 @@
 		snprintf(line, sizeof(line), "%d/%d", (int) rl.rlim_cur, (int) rl.rlim_max);
 	}
 	ast_cli(a->fd, "Current maxfiles: %s\n", line);
-	for (i = 0; i < 1024; i++) {
+	for (i = 0; i < ARRAY_LEN(fdleaks); i++) {
 		if (fdleaks[i].isopen) {
 			snprintf(line, sizeof(line), "%d", fdleaks[i].line);
 			ast_cli(a->fd, "%5d %15s:%-7.7s (%-25s): %s(%s)\n", i, fdleaks[i].file, line, fdleaks[i].function, fdleaks[i].callname, fdleaks[i].callargs);

-- 
To view, visit https://gerrit.asterisk.org/764
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Iacb69e7701c0f0a113786bd946cea5b6335a85e5
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Walter Doekes <walter+asterisk at wjd.nu>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>



More information about the asterisk-code-review mailing list