[zeromq-dev] Fwd: Changes to zmq::blob_t::set()

Thu Dec 31 19:36:04 CET 2020

Нема на чему. :)))

Remember, the code is designed for malloc() to be always called and _data
to be always non NULL.
The safest change is to have the code functionally identical for platforms
on which malloc() returns valid pointer and changed behavior only for
platforms which return NULL when size_ i 0.
Yes, you're right. malloc() can be completely skipped. But then all the
potential usages of this memory need to be checked.

Greetings,

сре, 30. дец 2020. у 23:41 Marko Majic <mmajic16 at gmail.com> је написао/ла:

> Hvala na odgovoru Jovane...;-)
>
> I don't see what the problem would be with skipping those two lines...
> They are not skipped if malloc() returns NULL, rather, they are skipped
> whenever the requested size is 0 (in which case malloc() itself is also
> skipped).  clear() (which is called just before) already sets the data to
> NULL and size to 0 (which is what is being requested :-)).  And, since we
> have not made a call to malloc() in the first place, we don't technically
> own anything (so we shouldn't be setting _owned to true)?  Omitting a
> (relatively slow) kernel call (such as malloc()) when it is clearly
> unnecessary (client is asking for 0 bytes!) - seems like a sound practice
> (?)...
>
> The only small issue I see with this is that clear() itself doesn't set
> _owned to false after a call to free() (which, it seems to me, it
> definitely should(!) - as the blob object doesn't own anything after a call
> to free())...  I think I will make that change in my copy as well (but, of
> course, I won't be merging anything back as I am far too much of a noob
> with this library to dare to contribute)... :-)
>
> Regards,
>
> Marko
>
> On Wed, Dec 30, 2020 at 4:05 PM BJovke <bjovan at gmail.com> wrote:
>
>> Здраво Марко. :)
>>
>> This is the malloc() docs:
>>
>> *If size is zero, the behavior is implementation defined (null pointer
>> may be returned, or some non-null pointer may be returned that may not be
>> used to access storage, but has to be passed to std::free
>> <https://en.cppreference.com/w/cpp/memory/c/free>) *
>> So it might be non-NULL as well as NULL but obviously the code thinks
>> it's always not NULL if successful.
>> Your change is not good because it skips these lines in NULL case:
>>
>> _size = size_;
>>
>> _owned = true;
>>
>> I would change the assert to:
>> alloc_assert (_data != NULL || size_ == 0);
>> and skip only memcpy() call.
>>
>> There's also a malloc's matching free() but this will be OK because
>> calling free() on NULL is harmless.
>>
>> You need to check for potential other memcpy's, asserts or similar stuff
>> on this memory.
>>
>> Greetings!
>>
>> сре, 30. дец 2020. у 06:35 Marko Majic <mmajic16 at gmail.com> је
>> написао/ла:
>>
>>> Hi all,
>>>
>>>
>>> This is a repeat as I am not sure if my original message made it onto
>>> the mailing list (my apologies if it did!).
>>>
>>>
>>> I am trying to build a libzmq library (as a Windows DLL) with an
>>> unsupported compiler.  I had to make a few changes to various source files
>>> (mostly having to do with the mismatched class method signatures as  many
>>> declarations in header files seem to be dropping ‘const’ qualifiers from
>>> the actual definition in the source which, for some strange reason, works
>>> fine on my 64-bit compiler but results in missing linker symbols in my
>>> 32-bit compiler?).  However, in the end (after those changes), it all
>>> compiled (and seemed) fine.
>>>
>>>
>>>
>>> I then wrote a small script to auto-execute all the test_*.exe programs
>>> after building is complete and (initially) most of them worked but any that
>>> didn’t would invariably fail with an ‘OUT OF MEMORY’ error pointing to
>>> blob.hpp.  I found this odd – both because I have ample RAM on my dev box
>>> and because I didn’t think libzmq would be a particular memory hog – so I
>>> put a few diagnostic messages in blob.hpp set() method and it seems, that
>>> all the calls to set() that actually fail are requesting 0 bytes.  This, of
>>> course, made sense – since malloc() would be returning NULL when 0 bytes
>>> are requested which, in turn, would trip the subsequent alloc_assert();
>>> call (macro, that is).
>>>
>>>
>>>
>>> To get around this issue, I changed the set() code from:
>>>
>>> void set (const unsigned char *const data_, const size_t size_)
>>>
>>> {
>>>
>>> clear ();
>>>
>>>       _data = static_cast<unsigned char *> (malloc (size_));
>>>
>>> alloc_assert (_data);
>>>
>>> _size = size_;
>>>
>>> _owned = true;
>>>
>>> memcpy (_data, data_, size_);
>>>
>>> }
>>>
>>>
>>>
>>> To:
>>>
>>> void set (const unsigned char *const data_, const size_t size_)
>>>
>>> {
>>>
>>> clear ();
>>>
>>>       if(size_) {
>>>
>>>             <…same as above…>
>>>
>>>       }
>>>
>>> }
>>>
>>>
>>>
>>> And also made the same change to the (equivalent) call set_deep_copy().
>>>
>>>
>>>
>>> On the face of it, these changes make perfect logical sense (if making a
>>> copy of an empty blob – the result should be an empty blob, and when
>>> creating a blob of 0 bytes, the result should, again, be an empty blob). I
>>> re-compiled the library and test programs with the above changes to
>>> zmq::blob_t::set() and zmq::blob_t::set_deep_copy() and now all of the test
>>> programs execute fine and pass all the tests - 0 failures, 3 ignored tests
>>> – 2 having to do with ZMQ_MULTICAST_LOOP=false not working on Windows
>>> (marked as “TODO” so – evidently a known issue) and one having to do with
>>> TIPC not being available (which, again, makes sense since I am running this
>>> on Windows and {I think?} TIPC is a Linux thing)…
>>>
>>>
>>>
>>> So, it all seems great - however, since I am (admittedly) a complete
>>> noob on the internal workings of libzmq (and the above seemed a bit
>>> “elementary”) - I just wanted to check in with the “hive mind” of people
>>> that know far more about libzmq than me.  Are the changes I made OK? Or
>>> would they cause other problems (because empty blobs or requesting 0 bytes
>>> SHOULD cause assert failures)?  If latter, how do those test programs work
>>> on other platforms/compilers (I would assume that malloc(0) on any C
>>> compiler on any platform will return NULL just like it did for me – which,
>>> of course, would trip the alloc_assert macro). Or do I have a problem
>>> because some of the test programs are, in fact, requesting 0-byte blobs
>>> (when, in fact, none of them should)?
>>>
>>>
>>>
>>> Any enlightenment is (always) greatly appreciated!
>>>
>>>
>>>
>>> Cheers,
>>>
>>>
>>>
>>> Marko
>>>
>>>
>>> _______________________________________________
>>> zeromq-dev mailing list
>>> zeromq-dev at lists.zeromq.org
>>> https://lists.zeromq.org/mailman/listinfo/zeromq-dev
>>>
>>
>>
>> --
>>
>> Jovan Bunjevački.
>> _______________________________________________
>> zeromq-dev mailing list
>> zeromq-dev at lists.zeromq.org
>> https://lists.zeromq.org/mailman/listinfo/zeromq-dev
>>
> _______________________________________________
> zeromq-dev mailing list
> zeromq-dev at lists.zeromq.org
> https://lists.zeromq.org/mailman/listinfo/zeromq-dev
>

-- 

Jovan Bunjevački.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.zeromq.org/pipermail/zeromq-dev/attachments/20201231/5e7b93bb/attachment.htm>