P-p-H-d / mlib

Library of generic and type safe containers in pure C language (C99 or C11) for a wide collection of container (comparable to the C++ STL).
BSD 2-Clause "Simplified" License
898 stars 77 forks source link

Wrong implementation for set_cstr for bounded strings #86

Closed vladipus closed 3 years ago

vladipus commented 3 years ago

Indirectly they would use strncpy_s, but that method DOES NOT allow truncating, which seems needed by the semantic of your method.

vladipus commented 3 years ago

image this implementation also seems off and risky, since the last argument is actually count that could be used as s1[count]

vladipus commented 3 years ago

Maybe something like that would do? image

vladipus commented 3 years ago

Another problem is that strcpy_s guarantees null-termination, but your bound strings may be non-null terminated.

vladipus commented 3 years ago

Hmm, you are using strlen in you bounded string size method. That is strange and contradicts with my statement above. Now I see you have max_size + 1 as an array. But you still have to refactor the usage of strncpy

vladipus commented 3 years ago

You can even end up with an invalid state since currently: image

vladipus commented 3 years ago

You could just image or maybe rewrite the m_core_strncpy to M_core_strncpy_s and use it everywhere afterwards.

vladipus commented 3 years ago

image this method is also implemented incorrectly. the size of the destination buffer can't be removed that simply and the m_core_strncat is also wrong in case of using strncat_s, semantically wrong.

vladipus commented 3 years ago

An interesting thing. At least in debug build. The strncpy_s actually may write data past zero terminator, so it mast be saved manually and restored (for OOR compatibility.

P-p-H-d commented 3 years ago

The issue with strncpy_s and strncat_s should be fixed in master. This is strange however. All the tests already passed with Visual, and they did trigger this behavior of truncation... So I don't know why they succeed...

this method is also implemented incorrectly. the size of the destination buffer can't be removed that simply

Which method?

P-p-H-d commented 3 years ago

This is strange however. All the tests already passed with Visual, and they did trigger this behavior of truncation... So I don't know why they succeed...

It is because there was a constraint violation (as you notice), the default runtime handler is called which terminates the program and returns a negative error code to the caller! This is not handled by the script which assumes a successful termination. (It is fixed in master).

vladipus commented 3 years ago

Which method? bounded string's equal str p

vladipus commented 3 years ago

Are you sure about your re-implementation? I've coped with the issue with _s variants. Seems like it is not that bad of an idea to use the secure methods on the bounded strings, since their size is statically defined.

P-p-H-d commented 3 years ago

Which method? bounded string's equal str p

I see no issue with _equal_str_p of bounded string.

Are you sure about your re-implementation?

As sure as the tests passed

vladipus commented 3 years ago

I see no issue with _equal_str_p of bounded string.

In case you've also reimplemented the cat method yourself, there might be no issue. But there WAS one.

As sure as the tests passed

I've implemented _s variants through non-_s like so and all tests have also passed ;) image

P-p-H-d commented 3 years ago

In case you've also reimplemented the cat method yourself, there might be no issue. But there WAS one.

Yes, the service that used the cat method was wrong. But _equal_str_p doesn't use this cat method :)

vladipus commented 3 years ago

Maybe I've misrememeber =) Ok, closing the issue.