Pr 426

[badboy]

Closes #426, now with test

[badboy]

@not-A-robot r+

[ghost]

📌 Commit 8655a6a has been approved by badboy

[ghost]

⚡ Test exempted - status

[vstakhov]

This change looks incorrect to my opinion. According to sdscatfmt, %T modifier means size_t variable. Indeed, len has this type in hiredis.c: size_t len;. In sds.h %T is handled in the following case:

            case 'u':
            case 'U':
            case 'T':
                if (next == 'u')
                    unum = va_arg(ap,unsigned int);
                else if(next == 'U')
                    unum = va_arg(ap,unsigned long long);
                else
                    unum = (unsigned long long)va_arg(ap,size_t);

Hence, before patch, there was a correct format string. Now, on the contrary, it is broken at least on big endian machines where size_t > unsigned int and when values does not fit in unsigned int. Could you please recheck this change?

[badboy]

@vstakhov Uff, not sure I'm following. What values would produce an incorrect result here? (If you bring this up in a new issue referencing this PR I can better track it)

[vstakhov]

You extract size_t argument using va_arg(ap,unsigned int);. Does it look sane? The previous version did the proper thing - it extracted size_t as size_t: (unsigned long long)va_arg(ap,size_t);. I'm using this PR because it changed the correct behaviour to the incorrect one (at least, I think so).

[badboy]

But I can't re-open a PR to track the potential bug ;)

I take a look at this problem next week. Thanks for bringing it up.