v1.2 ssl_options:on_error string init not recommended

[Pascal-Fremaux]

Hi, testing integrating the new Paho 1.2, thx for the update!

I checked the new method for the SSL error to replace ours, and I saw something that could give warning as uselessly doing pointer arithmetic (not recommended in modern C++)
Such warning could be enforced by gcc warning option (sorry, not in my mind tonight) or got via some static checker tool.

In ssl_options:on_error()
string errMsg { str, str+len };

Also I am not sure what ctor it uses.

This would be safer to use the simple ctor (ptr, len)
basic_string( const CharT* s,
size_type count,
const Allocator& alloc = Allocator() );

so here:
string errMsg { str, len };

This one do not generate any warning as no pointer arithmetic.

[fpagliughi]

With the STL, raw pointers can be used as iterators. So it calls:

template< class InputIt >
basic_string( InputIt first, InputIt last,
              const Allocator& alloc = Allocator() );

with str+len being the last iterator as it points to one place past the end of the data you want to copy into the string.

This is fairly portable across STL containers as most have a constructor using first and last iterators, like:

template< class InputIt >
vector( InputIt first, InputIt last,
        const Allocator& alloc = Allocator() );

but I'm not sure of any other containers that take a pointer and a length the way that basic_string<> does. So I probably used that constructor out of portability/habit. I'm sure I use it all over the place, like:

paho.mqtt.cpp/src/async_client.cpp

Lines 212 to 214 in 33921c8

	size_t len = (topicLen == 0) ? strlen(topicName) : size_t(topicLen);
	string topic(topicName, topicName+len);

I would have no problem changing them as the constructor you mention makes as much (or more) sense for the specific usage.

But I know of no warnings that claim this as a problem. Could you try to look that up for me? (Thanks)

I'm not sure this would come up too often in a pure C++ application, but when interfacing to a C lib, I don't know any way around this type of thing.

[Pascal-Fremaux]

I had suspected the Iterator ctor but was unsure that pointer are taken as iterators, so good info for me.

Pointer arithmetic is unsafe, and so not recommended by the safety code guidelines in general, with exception of the pointers on real array as long as managing the out of bounds.

You can find recommendations in the ISO C++ Code guidelines by searching "pointer arithmetic" in it. (https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines)
In particular: ES.42: Keep use of pointers simple and straightforward
"Pointer arithmetic is fragile and easy to get wrong, the source of many, many bad bugs and security violations."

This is why it is enforced in the code guidelines that deals with code safety, as in the major MISRAC++08 (obsolete but the current reference), or AUTOSAR C++14 code guidelines that modernize the MISRA-C++08 (or JSF)
Example:
See MISRA C++ 2008 [7]
Rule M5-0-15 (required, implementation, automated) Array
indexing shall be the only form of pointer arithmetic.
Rule M5-0-16 (required, implementation, automated)
A pointer operand and any pointer resulting from pointer arithmetic
using that operand shall both address elements of the same array.
(AUTOSAR C++14 code guidelines 19-03)
Rule A5-0-4 (required, implementation, automated)
Pointer arithmetic shall not be used with pointers to non-final classes.

(in your case the string is handled by a pointer, not as an array, so not compliant)

Those rules can be tested with static code checkers (Codesonar, Klockwork...)

GCC warnings allows cheking some wrong/implicit conversions but I do nto know if they work for that case:
(I did not test them, I got such info in our code via Codesonar and rule M5-0-15, but never test on paho),
could be:
https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html
-Wconversion or -Warith-conversion (not focus on pointer arithmetic but arithmetic implicit conversion in general)
-Wpointer-arith (in -Wpedantic)

But you'd better use clang-tidy that I never used but perfect for that
It seems it check such with rule cppcoreguidelines-pro-bounds-pointer-arithmetic (TBC)
https://stackoverflow.com/questions/59707939/avoid-pointer-arithmetic-fix-clang-tidy-error

Some examples from the Codesonar doc (on M5-0-15 like):
void parith(int *p, int i)
{
int *p1;

p1 = p + i;    /* 'Pointer Arithmetic' warning issued here */
p1 = i + p;    /* 'Pointer Arithmetic' warning issued here */
p += i;        /* 'Pointer Arithmetic' warning issued here */
p++;                       /* operator is not -, +, +=, or -= */
i += 5;                    /* '+=' operands are not pointers */
p1 = *(p - 5); /* 'Pointer Arithmetic' warning issued here */
i = *p - 5;                 /* '-' operands are not pointers */                 
i = p1 - p;                 /* exception case: subtraction between two pointers */
p -= 4;        /* 'Pointer Arithmetic' warning issued here */

}

[fpagliughi]

Cool. Thanks for the info. I actually guessed you were mainly talking about MISRA or an industrial guideline.

Like I said, I have no problem changing this specific case, and I really should remember it when working with string between C and C++, as the ptr/len ctor is actually more obvious and direct. But I do think the idiom of ptr+len as an iterator for "last" is common, especially for all the other containers that lack any better way to do it.

Also, this library is just one giant wrapper around a C lib, and in this interface area, the general C++ rules need to be broken because your interfacing with a different, lower-level API. Other languages like Rust let you (or require you) to mark these sections with an unsafe keyword, or equivalent, but C++ lacks this because all the unsafe stuff is part of the language.

So there's always going to be exceptions.