Page MenuHomeWildfire Games

Fix gcc 7 ModIo.cpp compile warning - aligned-new / extended alignment 64
ClosedPublic

Authored by elexis on Jul 20 2019, 6:36 PM.

Details

Summary

Ever since rP21759 there is a gcc 7 compiler warning:

../../../source/ps/ModIo.cpp: In member function ‘void ModIo::StartDownloadMod(size_t)’:
../../../source/ps/ModIo.cpp:320:82: warning: ‘new’ of type ‘DownloadCallbackData’ with extended alignment 64 [-Waligned-new=]
  m_CallbackData = new DownloadCallbackData(sys_OpenFile(m_DownloadFilePath, "wb"));
                                                                                  ^
../../../source/ps/ModIo.cpp:320:82: note: uses ‘void* operator new(std::size_t)’, which does not have an alignment parameter
../../../source/ps/ModIo.cpp:320:82: note: use ‘-faligned-new’ to enable C++17 over-aligned new support
Test Plan

Make sure the warning is fixed in the most appropriate way.

First I looked into #pragma pack, but this uses extended alginment, 64 bit so it seems that can't be used here.
Reading a bit the documentation:

libsodium/include/sodium/crypto_generichash.h

/*
 * Important when writing bindings for other programming languages:
 * the state address should be 64-bytes aligned.
 */
typedef crypto_generichash_blake2b_state crypto_generichash_state;

libsodium/include/sodium/utils.h
 *
 * The crypto_generichash_state structure is packed and its length is
 * either 357 or 361 bytes. For this reason, when using sodium_malloc() to
 * allocate a crypto_generichash_state structure, padding must be added in
 * order to ensure proper alignment. crypto_generichash_statebytes()
 * returns the rounded up structure size, and should be prefered to sizeof():
 * state = sodium_malloc(crypto_generichash_statebytes());
 */

So it seems there is a malloc missing as well here, other ModIO code already does that in this file.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

elexis created this revision.Jul 20 2019, 6:36 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/149/display/redirect

elexis added inline comments.Wed, Jul 24, 5:02 PM
source/ps/ModIo.cpp
174 ↗(On Diff #9016)

(*& pointless)

historic_bruno added inline comments.
source/ps/ModIo.cpp
59 ↗(On Diff #9016)

Do we catch this somewhere?

61 ↗(On Diff #9016)

static_cast seems better here, and also below

171 ↗(On Diff #9016)

static_cast

174 ↗(On Diff #9016)

static_cast

554 ↗(On Diff #9016)

I'm not clear on how m_CallbackData->hash_state is ever nullptr here, doesn't that imply DownloadCallbackData failed to construct (which would surely be handled before this point)?

555 ↗(On Diff #9016)

remove *&

Something like this is what it would look like with PSError_ModIO_MallocFailed

, D2124 would also be done when messing with PSERROR.
But the rest of the ModIO class uses ENSURE, so it should be used here as well for consistency I suppose.

source/ps/ModIo.cpp
59 ↗(On Diff #9016)

ModIo::StartDownloadMod constructs this, from JSI_ModIo::StartDownloadMod called from modio.js.

So it would seem right to JS_ReportError, rather than std::terminate and rather than LOGERROR as is in ModIo.cpp.

61 ↗(On Diff #9016)

Digested:
https://en.cppreference.com/w/cpp/language/static_cast
https://stackoverflow.com/questions/310451/should-i-use-static-cast-or-reinterpret-cast-when-casting-a-void-to-whatever
https://stackoverflow.com/questions/28002/regular-cast-vs-static-cast-vs-dynamic-cast
and agree, the static_cast is more narrow and does support that conversion (5), thus preferable.

crypto_generichash_state is a typedef for:

typedef struct CRYPTO_ALIGN(64) crypto_generichash_blake2b_state {
    unsigned char opaque[384];
} crypto_generichash_blake2b_state;

(I guess sodium_malloc returns a void* and not a *crypto_generichash_state so it can be used for other functions.)
(I suppose its still cleaner to cast immediately rather than catching a void*.)

554 ↗(On Diff #9016)

Correct, but it's good practice to not deref pointers without a null check I heard, so that if it ever would come to this (for example due to undocumented implementation or library bugs), it wouldn't segfault here but report the error.

elexis updated this revision to Diff 9112.Wed, Jul 24, 11:31 PM

Use ENSURE for consistency, remove pointless &*, prefer static_cast over reinterpret_cast

elexis added inline comments.Wed, Jul 24, 11:35 PM
source/ps/ModIo.cpp
554 ↗(On Diff #9016)

Consistent ENSURE could work too as a placeholder until throw / JS error handling or anything else was implemented.

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/228/display/redirect

historic_bruno added inline comments.Thu, Jul 25, 6:29 AM
source/ps/ModIo.cpp
554 ↗(On Diff #9016)

That's basically the argument an ENSURE. There's no actual error we anticipate handling (it's not really the final hash that failed to compute), but we want an obvious debug message if a dev messes up the logic somehow.

elexis updated this revision to Diff 9123.Fri, Jul 26, 12:45 AM

One more ENSURE

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/234/display/redirect

source/ps/ModIo.cpp
171 ↗(On Diff #9123)

Feel like one more ENSURE? :P
For some reason the other case jumped out at me, but this one slipped under the radar. Looks like a silent failure if data->hash_state somehow managed to be nullptr here.

elexis updated this revision to Diff 9124.Fri, Jul 26, 1:12 AM

play the same song again

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/235/display/redirect

Itms requested changes to this revision.Mon, Aug 5, 12:24 PM
Itms added a subscriber: Itms.

Thanks for looking into this! The concept looks good to me and works, but it has a memory leak. The fix is easy 🙂

source/ps/ModIo.cpp
52 ↗(On Diff #9124)

This now needs a dtor calling sodium_free(hash_state).

This revision now requires changes to proceed.Mon, Aug 5, 12:24 PM
elexis updated this revision to Diff 9234.Tue, Aug 6, 2:06 AM

Add sillily missing destructor.

Vulcan added a comment.Tue, Aug 6, 2:11 AM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/314/display/redirect

Stan added a subscriber: Stan.Tue, Aug 6, 8:51 AM
Stan added inline comments.
source/ps/ModIo.cpp
72 ↗(On Diff #9234)

Since we take ownership shouldn't this be a smart ptr ?

Itms accepted this revision.Tue, Aug 6, 2:24 PM

This is good now!

source/ps/ModIo.cpp
72 ↗(On Diff #9234)

No need to overcomplicate things here, we are not going to do strange things with this pointer, it's just a regular pointer. Nothing outside of the ModIo class will or can meddle with it.

This revision is now accepted and ready to land.Tue, Aug 6, 2:24 PM
elexis added a comment.Tue, Aug 6, 2:55 PM

Thanks for the reviews and comments!

source/ps/ModIo.cpp
72 ↗(On Diff #9234)

The member stores what sodium_malloc creates, what crypto_generichash_init and sodium_free expect, thus avoiding any implicit and explicit conversion (coincidentally removing the compiler warning).

A shared_ptr has the advantage that it is automatically deleted, so one can't leak memory as easily and can share it amongst multiple classes / pieces of code without having to care about whose obligation deletion is.
But what would be the advantage of a smart pointer here? The pointer is not shared with other code, and avoiding deletion would mean assuming that delete would be the same as sodium_free. But then why would that function exist?

The specs say:

The sodium_free() function unlocks and deallocates memory allocated using sodium_malloc() or sodium_allocarray().
sodium_free() also fills the memory region with zeros before the deallocation.

This is written in the chapter Secure memory https://libsodium.gitbook.io/doc/memory_management:

After use, sensitive data should be overwritten, but memset() and hand-written code can be silently stripped out by an optimizing compiler or by the linker

elexis retitled this revision from Fix gcc 7 ModIO.cpp compile warning to Fix gcc 7 ModIo.cpp compile warning - aligned-new / extended alignment 64.Tue, Aug 6, 2:57 PM
This revision was automatically updated to reflect the committed changes.