curl_printf.h macros can conflict with external headers #743

Closed
davidben opened this Issue Mar 30, 2016 · 3 comments

Projects

None yet

2 participants

@davidben
Contributor

curl_printf.h defines printf to curl_mprintf, etc. This can cause problems with external headers which may use __attribute__((format(printf, ...))) markers. This can be worked around by spelling it __format__ and __printf__ (which I have done for BoringSSL), but this still seems something curl should avoid. Defining macros in libc's namespace right before including external headers is a little screwy.

The simplest fix would be to require curl_printf.h be one of the last includes, just before curl_memory.h. Though this too is a little screwy since curl_memory.h includes curl.h which includes stdio.h. I think the only reason that one doesn't break is because curl_printf.h pulls in mprintf.h which, in turn, pulls in stdio.h first.

I don't know if you'd rather that get resolved first. (One possibility is to separate the headers which define the symbols of interest from the headers which define the convenience macros. Another is maybe not to use the macros at all and call curl_mprintf and friends directly in cURL code) If not, I'm happy to send a patch which just moves all the curl_printf.h includes to the end.

@bagder bagder self-assigned this Mar 30, 2016
@bagder
Member
bagder commented Mar 31, 2016

Thanks @davidben, it all makes sense.

I like keeping the defines simply because it makes a lot of code use the traditional and commonly known function names. If we'd use the full curl_printf() names only, I fear we will get use of the original functions to sneak in over time - and cause portability issues. Also, there are over 200 calls to *printf() in libcurl so a lot of search/replace and reindenting code would have to be done.

Moving the include seems like the least annoying change. I think we can improve the curl_memory.h situation somewhat by stop it from including <curl/curl.h> since the only reason it does so is to get the memory replacement callback typedefs so I think we can live with having them copied there. (An alternative approach would be to move the typedefs over to a separate header files that could be included individually for this purpose but that's a slightly larger take since the include files are public.) The curl_memory.h setup is only for debug builds anyway. Like this:

diff --git a/lib/curl_memory.h b/lib/curl_memory.h
index fcd177c..637b24d 100644
--- a/lib/curl_memory.h
+++ b/lib/curl_memory.h
@@ -5,11 +5,11 @@
  *  Project                     ___| | | |  _ \| |
  *                             / __| | | | |_) | |
  *                            | (__| |_| |  _ <| |___
  *                             \___|\___/|_| \_\_____|
  *
- * Copyright (C) 1998 - 2013, Daniel Stenberg, <daniel@haxx.se>, et al.
+ * Copyright (C) 1998 - 2016, Daniel Stenberg, <daniel@haxx.se>, et al.
  *
  * This software is licensed as described in the file COPYING, which
  * you should have received as part of this distribution. The terms
  * are also available at https://curl.haxx.se/docs/copyright.html.
  *
@@ -81,11 +81,22 @@
 #error "Header memdebug.h shall not be included before curl_memory.h"
 #endif

 #ifndef CURLX_NO_MEMORY_CALLBACKS

-#include <curl/curl.h> /* for the callback typedefs */
+/*
+ * The following memory funciton replacement typedef's are COPIED from
+ * curl/curl.h and MUST match the originals. We copy them to avoid having to
+ * include curl/curl.h here. We avoid that include since it includes stdio.h
+ * and other headers that may get messed up with defines done here.
+ */
+typedef void *(*curl_malloc_callback)(size_t size);
+typedef void (*curl_free_callback)(void *ptr);
+typedef void *(*curl_realloc_callback)(void *ptr, size_t size);
+typedef char *(*curl_strdup_callback)(const char *str);
+typedef void *(*curl_calloc_callback)(size_t nmemb, size_t size);
+

 extern curl_malloc_callback Curl_cmalloc;
 extern curl_free_callback Curl_cfree;
 extern curl_realloc_callback Curl_crealloc;
 extern curl_strdup_callback Curl_cstrdup;
@bagder bagder added a commit that referenced this issue Apr 1, 2016
@bagder bagder curl_memory.h: avoid the curl/curl.h include
Discussed in #743
7218b52
@bagder
Member
bagder commented Apr 1, 2016

I merged that patch just now: 7218b52 to get it out of the way =)

@bagder bagder added a commit that closed this issue Apr 29, 2016
@bagder bagder lib: include curl_printf.h as one of the last headers
curl_printf.h defines printf to curl_mprintf, etc. This can cause
problems with external headers which may use
__attribute__((format(printf, ...))) markers etc.

To avoid that they cause problems with system includes, we include
curl_printf.h after any system headers. That makes the three last
headers to always be, and we keep them in this order:

 curl_printf.h
 curl_memory.h
 memdebug.h

None of them include system headers, they all do funny #defines.

Reported-by: David Benjamin

Fixes #743
4f45240
@bagder bagder closed this in 4f45240 Apr 29, 2016
@bagder
Member
bagder commented Apr 29, 2016

I bit the bullet and made it happen. Thanks for the report!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment