search

gearman / gearmand

http://gearman.org/
Other
736 stars 138 forks source link

CodeQL alerts #406

Open esabol opened 1 month ago

esabol commented 1 month ago

PR #404 and PR #405 fix a couple of CodeQL alerts, I think. After those, only a few CodeQL alerts remain....

The five "Potential use after free" alerts (#42, #43, #44, #45, #46) for libgearman-server/queue.cc lines 172-176 all appear to be wrong. Those lines come after the server.queue.functions= new queue_st(); line and inside the if (server.queue.functions) { ... } block, so I don't see any problem here. I think these should be dismissed as erroneous alerts. Do you agree, @SpamapS ?

The Uncontrolled format string alert around line 626 of libgearman-server/gearmand.cc appears to be legitimate. The alert suggests that, if a user sets the GEARMAND_PORT environment variable to a string with percent characters in it, for example, it could result in a buffer overflow and/or crash the program. I think this could be fixed by removing any percent characters from the buffer, right? I'm not sure that would actually satisfy CodeQL though, but it would satisfy me enough that the alert could be dismissed as handled. What do you think?

SpamapS commented 1 month ago

Agreed, the use after free ones seem like bugs in CodeQL. Maybe we should report them.

SpamapS commented 1 month ago

For the other alert, I think it doesn't matter, because the environment is essentially an administrative function, not a user function. So if they want to put % chars in there and break the daemon who are we to stop them? Anyway, it can probably be refactored to make it more robust to that and remove the footgun.

esabol commented 1 month ago

Agreed, the use after free ones seem like bugs in CodeQL. Maybe we should report them.

Yeah, I'll do that. OK, I have dismissed them as false positives and added comments explaining why.

I think I'll come up with a PR to truncate the GEARMAN_PORT environment variable value at the first % character.

esabol commented 1 month ago

I think I'll come up with a PR to truncate the GEARMAN_PORT environment variable value at the first % character.

I decided to truncate it at the first non-digit character instead since that would also address issue #320. See PR #411. Please review, @SpamapS.