Print.printf() always runs vsnprintf twice

[knifter]

It seems to me that Print.printf(fmt, ...) is allocating a local buffer but never actually uses it, always running vsnprintf() twice. A buffer of size 64 is allocated but then vsnprintf is called with (NULL, 0, ...) parameters just to calculate the length. Then it always allocates a new buffer to vsnprintf into. I've modified the code in such a way that loc_buf[64] is used at first and if it proofs insufficient, only then allocate another. I think this was the original idea.

Also: vsnprintf() returns int, and negative int for an error. I guess this may wreck havoc in various ways when that gets assigned to size_t (uint) and becomes some (large) positive number.

Have a look at this diff for the problem and the fix:

diff --git a/cores/esp32/Print.cpp b/cores/esp32/Print.cpp
index 8c29534..57d725f 100644
--- a/cores/esp32/Print.cpp
+++ b/cores/esp32/Print.cpp
@@ -52,15 +52,19 @@ size_t Print::printf(const char *format, ...)
     va_list copy;
     va_start(arg, format);
     va_copy(copy, arg);
-    size_t len = vsnprintf(NULL, 0, format, copy);
+    int len = vsnprintf(temp, sizeof(loc_buf), format, copy);
     va_end(copy);
+    if(len < 0) {
+        va_end(arg);
+        return 0;
+    };
     if(len >= sizeof(loc_buf)){
         temp = new char[len+1];
         if(temp == NULL) {
             return 0;
         }
+        len = vsnprintf(temp, len+1, format, arg);
     }
-    len = vsnprintf(temp, len+1, format, arg);
     write((uint8_t*)temp, len);
     va_end(arg);
     if(len >= sizeof(loc_buf)){

[atanisoft]

@knifter Please send a PR with this change.

[stickbreaker]

@knifter I think you also need to call va_end(arg) in the allocation failure exit.

     if(len >= sizeof(loc_buf)){
         temp = new char[len+1];
         if(temp == NULL) {
+++         va_end(arg);
            return 0;
         }

Chuck.

[stickbreaker]

one more stupid question: I am questioning the use of delete[] in the original code. I think the array delete delete[] should not be used. Just the standard delete;

  temp = new char[len+1];
...
  delete[] temp;

I can't find the rule for when new char[] creates an array of pointers or a pointer to an array of characters? Does the array delete only exist if operator delete[] is defined in to object?

from cplusplus.com delete[]
delete[] first deletes each element then deletes the "global" construct.

class OBJECTx
{
    static void operator delete(void* ptr, std::size_t sz) 
    { 
        delete (ptr); // ::operator delete(ptr) can also be used 
    } 
    
    static void operator delete[](void* ptr, std::size_t sz) 
    { 
        delete (ptr); // ::operator delete(ptr) can also be used 
    } 
}

OBJECTx  x = new OBJECTx[10];

delete[] x;

Chuck.

[atanisoft]

@stickbreaker likely standard delete is fine here. But likely only because it is a basic char array. A more c++ approach might be do away with allocations entirely and use std::vector<uint8_t> tmp; tmp.resize(len); vsnprintf(tmp.data(),....

[stickbreaker]

@atanisoft I don't vote for the std::vector route because of the overhead. The "use small Stack buffer" else "heap" is a definite performance decision. Manipulating Heap, incurs a big performance hit. Single thread, MUTEX, possible core stall.

Chuck.

[atanisoft]

Yeah, there are definitely a few ways to "fix" this and likely the simplest will be actually to switch from new/delete to malloc/free like the rest of the arduino-esp32 code uses (mostly) as well as the proposed fixes above.

[knifter]

I've created a pull request with above mentioned changes, including:

• va_end when new/malloc fails
• malloc/free instead of new/delete

[stale]

[STALE_SET] This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

[stale]

[STALE_DEL] This stale issue has been automatically closed. Thank you for your contributions.