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

[Marko Majic]

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.zeromq.org/pipermail/zeromq-dev/attachments/20201230/48bc11c5/attachment.htm>