Nonstandard usage of strcpy in str_util.cpp and url.cpp can result in mangled strings

[orion@…]

From forum discussions, seems to be happening primarily on 64-bit linux hosts.

Found connecting to Leiden Classical:

The master file lists a scheduler with whitespace around the URL, also the file upload handler has whitespace around the URL. They both become mangled in the client_state.xml file, and as a result, BOINC cannot connect to the project properly.

In master_boinc.gorlaeus.net.xml:

<scheduler> http://boinc.gorlaeus.net/Classical_c'''gi'''/cgi </scheduler>

becomes in client_state.xml:

<scheduler_url>http://boinc.gorlaeus.net/Classical_c'''i/'''/cgi </scheduler_url>

In sched_reply_boinc.gorlaeus.net.xml

<url> http://boinc.gorlaeus.net/Classical_cgi/file_upload_h'''an'''dler </url>

becomes in client_state.xml:

<url>http://boinc.gorlaeus.net/Classical_cgi/file_upload_h'''nd'''dler </url>

I focused on the whitespace, as none of the other projects I'm using have a whitespace, and it stuck out to me. No idea how to work the code, however.

[orion@…]

I'm also seeing this in client_state.xml, for the same Leiden tasks in the original report:

<command_line>
classical.in classical.out classical.ttdout
</command_line>

"ttdout" instead of "stdout," but I don't know where it gets the <command_line> from, I do not see it in any other XML files. It appears to be the same distance from the end of the string as the other malformities.

[orion@…]

(sorry, that should read "tddout" not "ttdout" in the most recent followup. I mistyped it.)

[dandelot]

The use of strcpy in strip_whitespace: strcpy(str, str+1);

violates the C standard. One must not have the source and destination strings be (even part of) the same string. Whether the strcpy will work, do nothing, or just behave badly depends on the endianness and implementation of strcpy on the target machine.

remove_str() has the same problematic usage. If the source string length were calculated memmove() could be used and is guaranteed to work properly even with overlapping areas. Or one could code a trivial local_strcpy that could be guaranteed to work.

The following should work (making an attempt to follow local style):

+
+// strcpy() cannot be safely used on overlapping strings
+// according the the C standard,
+// so here we provide a simple alternate implementation.
+static void
+local_strcpy(char *targ, char *src)
+{
+      unsigned n = 0;
+      while (1) {
+         targ[n] = src[n];
+         if ( !src[n]) {
+            break;
+         }
+         n++;
+      }
+}
 // remove whitespace from start and end of a string
 //
 void strip_whitespace(char *str) {
@@ -306,7 +322,7 @@
         if (!str[0]) break;
         if (!isascii(str[0])) break;
         if (!isspace(str[0])) break;
-        strcpy(str, str+1);
+        local_strcpy(str, str+1);
     }
     while (1) {
         n = (int)strlen(str);
@@ -709,7 +725,7 @@
     while (1) {
         p = strstr(p, str);
         if (!p) break;
-        strcpy(p, p+n);
+        local_strcpy(p, p+n);
     }
 }

[dandelot]

lib/url.cpp has the same mistake with strcpy.

url.cpp:260:        strcpy(p, p+1);

The various other strcpy instances in lib would need a closer look ( most strcpy uses clearly have different strings as arguments, so those are fine).

(I'm not the David Anderson you know...)

[dandelot]

It would be wiser to define local_strcpy as

static void local_strcpy(volatile char *targ, volatile char *src)

(the rest as before) to ensure against a too-smart compiler.

[dandelot]

I found no evidence of other obviously-incorrect strcpy calls anywhere in the boinc trunk source (using a simple find-and-grep). So one can hope the two lib/ source files are the only ones with the problem.

[orion@…]

I can confirm that this patch solves the bug as reported.

[dandelot]

A patch which addresses the same bug in url.cpp as well as the one reported in this bug report is:

Index: str_util.h
===================================================================
--- str_util.h	(revision 22840)
+++ str_util.h	(working copy)
@@ -36,6 +36,7 @@
 extern char* time_to_string(double);
 extern char* precision_time_to_string(double);
 extern std::string timediff_format(double);
+extern void local_strcpy(volatile char *targ, volatile char *src);
 
 inline bool ends_with(std::string const& s, std::string const& suffix) {
     return
Index: url.cpp
===================================================================
--- url.cpp	(revision 22840)
+++ url.cpp	(working copy)
@@ -257,7 +257,7 @@
     while (1) {
         p = strstr(buf, "//");
         if (!p) break;
-        strcpy(p, p+1);
+        local_strcpy(p, p+1);
     }
     n = strlen(buf);
     if (buf[n-1] != '/') {
Index: str_util.cpp
===================================================================
--- str_util.cpp	(revision 22840)
+++ str_util.cpp	(working copy)
@@ -298,6 +298,21 @@
     return argc;
 }
 
+
+// strcpy() cannot be safely used on overlapping strings
+// according the the C standard,
+// so here we provide a simple alternate implementation.
+void local_strcpy(volatile char *targ, volatile char *src)
+{
+      unsigned n = 0;
+      while (1) {
+         targ[n] = src[n];
+         if ( !src[n]) {
+            break;
+         }
+         n++;
+      }
+}
 // remove whitespace from start and end of a string
 //
 void strip_whitespace(char *str) {
@@ -306,7 +321,7 @@
         if (!str[0]) break;
         if (!isascii(str[0])) break;
         if (!isspace(str[0])) break;
-        strcpy(str, str+1);
+        local_strcpy(str, str+1);
     }
     while (1) {
         n = (int)strlen(str);
@@ -709,7 +724,7 @@
     while (1) {
         p = strstr(p, str);
         if (!p) break;
-        strcpy(p, p+n);
+        local_strcpy(p, p+n);
     }
 }

A separate bug report on lib/url.cpp is needed (IMO) because it assumes variously that a url will not ever exceed 255 or 1023 bytes in length. Some day one will...

[orion@…]

Looks like part of this might be fixed in 23007

Still needs to patch url.cpp.