RTOS/TMS320F28375D: Critical bug in xdc runtime System_doPrint using System_snprintf

Dirk,

Thanks for the detailed information.  I will look into reproducing the problem and determining which is the best way to fix the issue.

Judah

Dirk,
thanks for the report.

The fix I am testing right now is the following:

--- a/src/packages/xdc/runtime/System.c
+++ b/src/packages/xdc/runtime/System.c
@@ -310,6 +310,7 @@ Int System_doPrint(Char *buf, SizeT n, CString fmt, VaList *pva, Bool aFlag)
     Int     base;
     Char    c;
     Int     res;
+    Int     temp_res;
     Char    outbuf[OUTMAX];
     ptrdiff_t diff;

@@ -506,7 +507,9 @@ Int System_doPrint(Char *buf, SizeT n, CString fmt, VaList *pva, Bool aFlag)
                          /* Have enough space, increment to account for '\0' */
                          parse.precis++;
                     }
-                    res += System_extendFxn(&buf, &fmt, pva, &parse);
+                    temp_res = System_extendFxn(&buf, &fmt, pva, &parse);
+                    res += temp_res;
+                    n = n - temp_res;
                 }

This still has to go through more testing, obviously, but it seems to fix your test case.

Dear Judah, dear Sasha,

thank you for your quick response!

@Sasha: Your patch makes it better but leaves one problem. At the end System_doPrint writes a termination with '\0' outside the buffer.

1. The problem is, that the System_extendFxn (xdc_runtimeSystem_printfExtend_I) sets the buf pointer outside the buffer without respecting n - the end of the buffer (see first figure).
2. Than, back in Sytem_doPrinf (xdc_runtime_System_doPrint_I), the function works with this pointer outside the buffer (see second figure).
3. At the end it writes the termination (see figure three). I catched this error in my code.

In step 2 the parse structure has the information that the buffer is full because parse->precis is -1. When you come back to the outer System_doPrint, the information is lost.

To solve the problem you have to prevent the last termination.

Thanks a lot for your help. I hope you can find a clean solution.

Dirk

There should be a new XDCtools 3.55 release in a couple of days, and below is the fix that will be in there. We also added a comment in the documentation that states that if extended formats are used, the return value from snprintf() and vsnprintf() is not guaranteed to be the number of characters that would be printed if there were enough space. System_extendFxn is calling some APIs that do not return the number of characters that would be printed if the space were available, which means that using extended formats was always an exception regarding the return value from snprintf() and vsnprintf(). We could for some extended formats do the both - fixed the bug you have found and also preserve the return value functionality for snprintf() and vsnprintf(), but we have decided that we didn't want to add more code to satisfy both.

--- a/src/packages/xdc/runtime/System.c
+++ b/src/packages/xdc/runtime/System.c
@@ -24,6 +24,7 @@
#include <stdlib.h>
#include <stdarg.h>
#include <string.h>
+#include <limits.h>
/*
 *  ======== OUTMAX ========
@@ -310,6 +311,7 @@ Int System_doPrint(Char *buf, SizeT n, CString fmt, VaList *pva, Bool aFlag)
    Int     base;
    Char    c;
    Int     res;
+    Int     temp_res;
    Char    outbuf[OUTMAX];
@@ -498,15 +500,32 @@ Int System_doPrint(Char *buf, SizeT n, CString fmt, VaList *pva, Bool aFlag)
                /* check if enough buffer space available */
                if (n > 1U) {
-                    /* parse.precis should account for the buffer size */
+                    /* parse.precis should account for the buffer size. We are
+                     * assuming that SIZE_MAX>=INT_MAX because it's true for
+                     * the compilers we care about. A longer discussion is here:
+                     * stackoverflow.com/.../is-the-max-value-of-size-t-size-max-defined-relative-to-the-other-integer-type
+                     */
                    if ((parse.precis == -1) || ((SizeT)parse.precis >= n)) {
-                         parse.precis = (Int)n;
+                         if (n < INT_MAX) {
+                             parse.precis = (Int)n;
+                         }
+                         else {
+                             parse.precis = INT_MAX;
+                         }
                    }
                    else {
                         /* Have enough space, increment to account for '\0' */
                         parse.precis++;
                    }
-                    res += System_extendFxn(&buf, &fmt, pva, &parse);
+                    temp_res = System_extendFxn(&buf, &fmt, pva, &parse);
+                    /* temp_res is how many were printed. We are losing here the
+                     * info about how many characters would be printed, but that
+                     * info is lost by calling various Text functions that don't
+                     * report how many characters would be printed if there were
+                     * enough space.
+                     */
+                    res += temp_res;
+                    n = n - temp_res;
                }
            }

--- a/src/packages/xdc/runtime/System.xdt
+++ b/src/packages/xdc/runtime/System.xdt
@@ -181,6 +181,12 @@ xdc_Int xdc_runtime_System_printfExtend__I(xdc_Char **pbuf, xdc_CString *PFMT,
                                                parse->ptr, pva, parse->aFlag);
            if (*pbuf != NULL) {
+                if (res >= parse->precis) {
+                    /* Not enough space for all characters, only
+                     * (parse->precis - 1) and '\0' were printed.
+                     */
+                    res = parse->precis - 1;
+                }
                *pbuf += res;
            }

--- a/src/packages/xdc/runtime/System.xdc                                      
+++ b/src/packages/xdc/runtime/System.xdc
@@ -629,10 +629,12 @@ module System {
     *  @a(returns)
     *  `snprintf` returns the number of characters that would have been
     *  written had `n` been sufficiently large, not counting the terminating
-     *  '\0' character.
+     *  '\0' character. However, if extended conversion specifiers are used
+     *  (@see #printf), the return value may be lower than the number of
+     *  characters that would be written had `n` been sufficiently large.
     */
    Int snprintf(Char buf[], SizeT n, CString fmt, ...);
@@ -650,7 +652,9 @@ module System {                                            
     *  @a(returns)                                                          
     *  `vsnprintf` returns the number of characters that would have been    
     *  written had `n` been sufficiently large, not counting the terminating
-     *  '\0' character.                                                      
+     *  '\0' character. However, if extended conversion specifiers are used  
+     *  (@see #printf), the return value may be lower than the number of      
+     *  characters that would be written had `n` been sufficiently large.    
     */                                                                      
    Int vsnprintf(Char buf[], SizeT n, CString fmt, VaList va);

                  

Dear Sasha,

thank you for the patch and excuse my late answer.

I made a test with my testcase and a more complex test in my application. Now everything is fine and works for me.

By the way: Now that this works so fine there should be no reason anymore not to realize asnprintf.

I made a test by just modifying vsnprintf setting the last argument from FALSE to TRUE. It works.

Int System_vsnprintf(Char buf[], SizeT n, CString fmt, VaList va)
{
return (System_doPrint(buf, n, fmt, vaRef(va), TRUE));
}

The only thing that is missing is the wrapper.
Maybe you can think about adding a wrapper :-).

One more time. Thanks alot.
Bye
Dirk