Dynmap-Forge/Fabric

Dynmap-Forge/Fabric

888k Downloads

findExecutableOnPath, for the new webp feature, can cause NPE in some environments

gbl opened this issue ยท 13 comments

commented

Issue Description:

Commit 460e6f9 of 2020-10-25 adds a new method findExecutableOnPath that calls System.getenv("PATH").split .... In a Multicraft environment, java seems to get called without PATH being defined, probably for security reasons. This results in System.getenv("PATH") returning null, and the method throwing a NPE, preventing dynmap initialization.

Solution: make the method return null if System.getenv("PATH") returns null.

[X] I have looked at all other issues and this is not a duplicate
[X] I have been able to replicate this

commented

For folks who don't want to install a new (untrusted) build and don't have access to their Multicraft installation, the other workaround I found was to put nonsense values into the config file for the paths to the webp binaries so that it just fails to find them and never checks PATH.

cwebpPath: not-installed
dwebpPath: not-installed
commented

Oh that's a nice solution @logicallysound , thank you for sharing!

commented

Don't be so hard on the author, he has been supporting dynmap for many years, even added support for Forge and Fabric recently, and dedicated a lot of time into the project - time he isn't getting paid for. None of us knows how the pandemic affects him or his family, and if he has other priorities right now, then noone has the right to critizise this.

Also, releasing a piece of software correctly is much more work than just uploading it; you need to run tests, upload to all distribution sites, update descriptions, update version info, and even if some of this can be automated, you still need to do a lot of work manually.

I'm convinced that, when once one development speeds up again, this fix will quickly make it into the official distribution. Until then, enjoy the advantage of open source which means people can fix stuff themselves if the original author won't, for whichever reason.

Yes, but he often only update one time each minecraft update (so if i follow this pantern, so a fix had come in summer).

But seams like he has fix it, now. Is not soo fun dynmap as some leaks (we use localhost, so is not
really bad, will fix own password too dynmap so i not have to go true all plugins next time).

commented

What I did was compile dynmap myself, adding an extra check to that method. But I don't want to distribute yet another inofficial build that people can't verify.

If you have control of the multicraft installation (basically, if you're the host, not the customer; in my case, this is true because we're running on a dedicated Linux server, but have Multicraft installed for the not-so-technically-inclined admins), you can change the start command in the .jar.conf file to start a script instead of java directly, like this:

[start]
command = /home/minecraft/multicraft/startserver.sh {MAX_MEMORY} {START_MEMORY} "{JAR}"

and have that script define a PATH before starting java:

#!/bin/bash
echo "starting with $1 $2 $3"
export PATH=/bin:/usr/bin
exec /opt/oracle/java/jdk1.8.0_202/jre/bin/java \
	-Xmx${1}M \
	-Xms${2}M \
	-XX:+UseG1GC \
# plus many more java options ....
	-Djline.terminal=jline.UnsupportedTerminal \
	-jar "$3" nogui

I used that script even before running into the dynmap bug, because the command line got too long with all my java options. So I just added the PATH definition. But as said, that's a workaround that won't work for most people, because when you're the customer of a typical MC hoster, you don't have access to those config files.

commented

I'm not as technically-inclined as you but I am in the lucky position where I have access to the .jar.conf file so I might be able to implement this. Thank you for the quick response!

commented

I just updated to beta-5 for 1.16.4 support but ran into this issue as well.

While the dynmapโ„ข devs investigate a proper fix for this, is there a workaround I could use to include PATH when my servers are started?

commented

I have access to the installation of my Multicraft (Yeah !!!) but I'm a poor Linux technician (bouh), I feel it will be difficult for me to apply this bypass but we will try it.

Thank you for this proposal

commented

Are there anything news associated with this error? I will not be able to use Dynmap until this is fixed.

commented

I didn't intend to publish my version, because, as I said, software from unknown origins isn't what people should run. But, it's been over two weeks without this bug being resolved, so, here's my release:

https://github.com/gbl/dynmap/releases/tag/fix_path_problem

Note that I wrote that before dynmap officially supported 1.16.4, so, it was forked from an older version. But it has been in production on two different servers for two weeks now, so it should work for you as well.

commented

I have reviewed the code from @gbl , and I can safely say that the latest changes are legit and safe :)
Thank you for sharing the code with us! ๐Ÿ‘

commented

Don't be so hard on the author, he has been supporting dynmap for many years, even added support for Forge and Fabric recently, and dedicated a lot of time into the project - time he isn't getting paid for. None of us knows how the pandemic affects him or his family, and if he has other priorities right now, then noone has the right to critizise this.

Also, releasing a piece of software correctly is much more work than just uploading it; you need to run tests, upload to all distribution sites, update descriptions, update version info, and even if some of this can be automated, you still need to do a lot of work manually.

I'm convinced that, when once one development speeds up again, this fix will quickly make it into the official distribution. Until then, enjoy the advantage of open source which means people can fix stuff themselves if the original author won't, for whichever reason.

commented

I didn't intend to publish my version, because, as I said, software from unknown origins isn't what people should run. But, it's been over two weeks without this bug being resolved, so, here's my release:

https://github.com/gbl/dynmap/releases/tag/fix_path_problem

Note that I wrote that before dynmap officially supported 1.16.4, so, it was forked from an older version. But it has been in production on two different servers for two weeks now, so it should work for you as well.

Your fix work :)

If he not change, so just release one update each Minecraft update and his solution is probably not to use multicraft.

I can say it is multicraft some causes this, work fine if not use multicraft. Is sad you have to use version some only are verify of users.

If you can fix it soo easy, is it comical that he has not fixed the error for so long.

commented

Thank you @gbl for this version, works like a dream for me since Dynmap was also broken for me on paper 1.16.4