Ensure we never return 1 from sodium_init() onload

[starbelly]

sodium_init() will return 0 on success, -1 on failure, and 1 if sodium is already loaded and initialized (which is not an error). In the case where libsodium is already initialized and the system is restarted we may return 1 from onload nif function resulting in a crash.

• change the call to sodium_init() to check for an error return and return -1 explicitly in this case, otherwise always return zero at the end of our onload function.

[starbelly]

This case revealed itself via using elixir's new release functionality. By default when using runtime config the system is started once to configure itself, and then reboots to start up permanent with new configuration. With this in place, enacl would get loaded twice and the second time it would return 1 from sodim_init() call, resulting in a crash.

Here you is a repo that demonstrates the problem pulling enacl as a dep from hexpm : https://github.com/starbelly/nif_go_boom

To test :

git clone https://github.com/starbelly/nif_go_boom && \
cd nif_go_boom && \
mix deps.get  && \
mix deps.compile  && \
mix release && \
 _build/dev/rel/nif_go_boom/bin/nif_go_boom start_iex

There is a branch on this repo that pulls in my branch which contains the fix.

Assuming you're still in the nif_go_boom repo:

git checkout enacl-fixup-proof && \
rm -rf _build deps && \
mix deps.get  && \
mix deps.compile  && \
mix release && \
 _build/dev/rel/nif_go_boom/bin/nif_go_boom start_iex

[jlouis]

It looks correct to me.

However, I think we should also support a proper upgrade function, as we currently don't. This should fix code upgrades as well. I'm guessing we can just get away with doing nothing since most upgrades will be to a later version of libsodium and Frank tends to be backward compatible. So having a dummy return function there is probably enough since all of the old data will still work between upgrades, at least until we mess up that assumption.

On Sun, Jul 26, 2020 at 9:34 PM Bryan Paxton notifications@github.com wrote:

This case revealed itself via using elixir's new release functionality. By default when using runtime config the system is started once to configure itself, and then reboots to start up permanent with new configuration. With this in place, enacl would get loaded twice and the second time it would return 1 from sodim_init() call, resulting in a crash.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/jlouis/enacl/pull/51#issuecomment-664030732, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAABMH6RRCXGRQRVZTAKFBDR5SAKRANCNFSM4PID67SQ .

-- J.

[starbelly]

@jlouis Makes sense to me, so I'll just make some functions that return 0.

[starbelly]

@jlouis Added upgrade and unload handlers, the reload handle has been ignored since OTP 20 per docs. Do we think this is worth writing a test for? My spidey sense says no. To note, I did purge, delete, and load to make sure it "actually" works. 😁

[jlouis]

Merging this, do we also need a push for a new version?

[starbelly]

@jlouis Indeed we do!

[jlouis]

1.1.1 pushed