stdcxx-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Martin Sebor (JIRA)" <j...@apache.org>
Subject [jira] Commented: (STDCXX-1019) __rw_mkstemp in file.cpp should honor TMPDIR environment variable
Date Fri, 31 Oct 2008 20:43:44 GMT

    [ https://issues.apache.org/jira/browse/STDCXX-1019?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12644427#action_12644427
] 

Martin Sebor commented on STDCXX-1019:
--------------------------------------

I'm afraid the attached patch isn't quite safe.

{noformat}
Index: file.cpp
===================================================================
--- file.cpp	(revision 702657)
+++ file.cpp	(working copy)
@@ -42,6 +42,7 @@
 #include <stdio.h>    // for P_tmpdir, std{err,in,out}, tmpnam()
 #include <stdlib.h>   // for mkstemp(), strtoul(), size_t
 #include <ctype.h>    // for isalpha(), isspace(), toupper()
+#include <string.h>   // for memcpy()
 
 
 #if defined (_WIN32) && !defined (__CYGWIN__)
@@ -58,6 +59,9 @@
 #  define _BINARY 0
 #endif
 
+#ifndef PATH_MAX
+#  define PATH_MAX   1024
+#endif
 
 #include <rw/_file.h>
 #include <rw/_defs.h>
@@ -257,8 +261,18 @@
 #    define P_tmpdir "/tmp"
 #  endif   // P_tmpdir
 
-    char fnamebuf[] = P_tmpdir "/.rwtmpXXXXXX";
+    const char *tmpdir = getenv ("TMPDIR");
+    if (tmpdir == NULL) { 
+        tmpdir = P_tmpdir;
+    }
 
+    char fnamebuf [PATH_MAX];
+
+    size_t len = strlen (tmpdir) - 1;
+ 
+    memcpy (fnamebuf, tmpdir, len);
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
{noformat}

There's no guarantee that the string fits in the provided buffer, i.e., that {{(len + sizeof
"/.rwtmpXXXXXX" < sizeof fnamebuf)}}. A buffer overflow here would open up a security hole.
The best way to handle this case is to fail.

{noformat}
+    memcpy (fnamebuf+len, "/.rwtmpXXXXXX", sizeof ("/.rwtmpXXXXXX"));
                           ^^^^^^^^^^^^^^^          ^^^^^^^^^^^^^^^
+
{noformat}

We should avoid the string duplication here to eliminate possible mismatches in future changes.
Defining a local (static) constant for the string would be one way to avoid the duplication.

{noformat}
     fd = mkstemp (fnamebuf);
 
     if (fd >= 0)
@@ -294,7 +308,7 @@
     // names that have no extension. tempnam uses malloc to allocate
     // space for the filename; the program is responsible for freeing
     // this space when it is no longer needed. 
-    char* const fname = tempnam (P_tmpdir, ".rwtmp");
+    char* const fname = tempnam (tmpdir, ".rwtmp");
 
     if (!fname)
         return -1;
{noformat}


> __rw_mkstemp in file.cpp should honor TMPDIR environment variable
> -----------------------------------------------------------------
>
>                 Key: STDCXX-1019
>                 URL: https://issues.apache.org/jira/browse/STDCXX-1019
>             Project: C++ Standard Library
>          Issue Type: Sub-task
>          Components: 27. Input/Output
>    Affects Versions: 4.2.1
>         Environment: SunOS clue 5.10 Generic_118833-33 sun4u sparc SUNW,Sun-Fire-V215

> CC: Sun C++ 5.9 SunOS_sparc 2007/05/03
>            Reporter: Scott (Yu) Zhong
>            Assignee: Martin Sebor
>             Fix For: 4.2.2
>
>         Attachments: STDCXX-1019.patch
>
>   Original Estimate: 1h
>  Remaining Estimate: 1h
>
> TMPDIR should be honored in the internal function __rw_mkstemp

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


Mime
View raw message