mej / nhc

LBNL Node Health Check
Other
226 stars 79 forks source link

Fix unsafe string array conversion for passwd file parsing #117

Closed lebonez closed 1 year ago

lebonez commented 2 years ago

If a passwd file contains a line like below with a '*' in the second field you'll get an error like below if the CWD's first listed file has a '.' in it. In our case for slurm was /var/spool/slurm.

User:*:547:15815:User:/home/User:/bin/bash

common.nhc: line 381: cred_state.old: syntax error: invalid arithmetic operator (error token is ".old") NHC watchdog timer 26036 (60 secs) has expired. Signaling NHC: kill -s ALRM -- -26035 nhc: line 491: kill: (-26035) - No such process

The array conversion bash is parsing as list files in directory. is a valid field in passwd just not used often.

mej commented 1 year ago

I have to be honest...this report confuses me. What version of NHC is this? The most recent release, 1.4.3, has globbing disabled completely throughout the code. Glob expressions should expand only where required, and shell globbing is disabled immediately afterward (here, for example). So this problem should already be fixed.

Having said that, I actually prefer this method. I only recently ("recently" being "within the last 6ish years...") had the opportunity to test this technique in real production code, and I've found I actually like it better. So even though the specific issue cited here in this PR was corrected in another way, I'm taking this anyway! 😄

lebonez commented 1 year ago

Unsure why but it definitely was globbing the current directory. For whatever reason it would only glob the cwd if the first file is a dot file that it lists. Very strange the version we are using is 1.4.2.

mej commented 1 year ago

Unsure why but it definitely was globbing the current directory. For whatever reason it would only glob the cwd if the first file is a dot file that it lists. Very strange the version we are using is 1.4.2.

Yes, that was indeed a problem with 1.4.2; 1.4.3, the newest release, doesn't have that issue. 😀