[dev.icinga.com #6821] [Patch] Fix build issue and crash found on Solaris, potentially other Unix OSes

[icinga-migration]

This issue has been migrated from Redmine: https://dev.icinga.com/issues/6821

Created by jan.andres@berenberg.de on 2014-07-31 08:35:18 +00:00

Assignee: jan.andres@berenberg.de
Status: Resolved (closed on 2014-08-04 06:50:05 +00:00)
Target Version: 2.0.2
Last Update: 2014-08-04 07:03:09 +00:00 (in Redmine)

Icinga Version: 2.0.1

---

Hi,

This patch grew out of a heap corruption issue encountered in Utility::GlobRecursive() due to the way readdir_r() is used there. The buffer passed as the second argument of readdir_r() may need to be larger than just a struct dirent on some OSes because that struct's d_name member isn't guaranteed to be large enough for the file name to fit in. For instance, on Solaris, it's only 1 character in size. For reference see e.g. the POSIX readdir_r man page at: http://www.unix.com/man-page/POSIX/3posix/readdir\_r/

Anyway, while a quick fix would have been possible I chose to take the clean approach and replace the Win32 and Unix specific portions of GlobRecursive by portable code using the interfaces of boost::filesystem, cleaning things up a little along the way. (It could have been simplified more by using boost::recursive_directory_iterator instead of boost::directory_iterator but I didn't do that on purpose as it wouldn't have preserved the traversal order of the old implementation.)

The patch also contains a one-line fix for an unrelated issue: The argument to Utility::GetSymbolName() is a const void * but dladdr() requires a void *, so my compiler required me to add an explicit cast there.

Attachments

• icinga2-bego-20140730-01.diff jan.andres@berenberg.de - 2014-07-31 08:35:00 +00:00
• icinga2-readdir-quickfix.diff jan.andres@berenberg.de - 2014-08-02 06:55:50 +00:00
• icinga2-readdir-quickfix2.diff jan.andres@berenberg.de - 2014-08-02 07:20:30 +00:00

Changesets

2014-08-04 06:49:04 +00:00 by jan.andres@berenberg.de 20fc877

Build fix for Solaris

refs #6821

Signed-off-by: Gunnar Beutner <gunnar.beutner@netways.de>

2014-08-04 06:49:04 +00:00 by jan.andres@berenberg.de 46dbe5a

Fix incorrect usage of readdir_r

refs #6821

Signed-off-by: Gunnar Beutner <gunnar.beutner@netways.de>

2014-08-04 06:49:04 +00:00 by gbeutner 9c7be0e

Update AUTHORS

refs #6821

2014-08-04 06:49:11 +00:00 by gbeutner d8619cc

Merge branch 'fix/solaris-6821'

fixes #6821

[icinga-migration]

Updated by gbeutner on 2014-08-01 06:52:23 +00:00

• Assigned to set to jan.andres@berenberg.de

There are a number of issues with your patch:

• It fails to build on RHEL 5: http://build.icinga.org/jenkins/job/icinga2-rhel-5-rpm-package-x86/140/console
• Ideally there should be one patch for each separate issue (e.g. one for fixing the build issues and another one for the buffer overflow)
• Can you export patches using "git format-patch" (the difference being that format-patch provides additional information like the 'Author:' attribute while diff does not)

Also, it seems odd to use Boost.Filesystem for GlobRecursive() while leaving Glob() (and possibly other code that deals with paths) the way it is. I agree that we should use Boost.Filesystem however IMO we should either fix the buffer overflow (while sticking to readdir_r) or update all existing code to use Boost.Filesystem (which is definitely a bit more work).

I think we should quickfix the issue for 2.0.2 and plan the update for Boost.Filesystem for 2.1 if you're interested in working on that.

[icinga-migration]

Updated by gbeutner on 2014-08-01 07:33:27 +00:00

Also, apparently it might be safe to just use readdir instead of readdir_r because readdir's static dirent struct is allocated on a per-DIR basis (rather than on a global basis which would make using readdir non-safe for use with threads).

[icinga-migration]

Updated by gbeutner on 2014-08-01 08:36:42 +00:00

• Priority changed from Normal to High

[icinga-migration]

Updated by jan.andres@berenberg.de on 2014-08-02 06:56:25 +00:00

• File added icinga2-readdir-quickfix.diff

Okay, so here's an approach for a quick fix that should be fairly portable. I don't think it's a good idea to use readdir() instead of readdir_r() because it's not thread-safe. I don't know enough about the architecture of icinga2 to say if it would be okay to introduce non-thread-safe code here, but I generally wouldn't do that in a universal utility function such as GlobRecursive(); we don't know where it might get used in the future.

I'll get back to you on the other issues when I'm back at work next week.

[icinga-migration]

Updated by jan.andres@berenberg.de on 2014-08-02 07:21:01 +00:00

• File added icinga2-readdir-quickfix2.diff

Ah, at second glance, it's indeed the case that POSIX guarantees the buffer returned by readdir() can only get overwritten by another readdir() call when it's on the same directory stream. I wasn't aware of that. So we might as well just do this.

[icinga-migration]

Updated by gbeutner on 2014-08-04 06:50:05 +00:00

• Status changed from New to Resolved
• Done % changed from 0 to 100

Applied in changeset d8619cc.

[icinga-migration]

Updated by gbeutner on 2014-08-04 07:03:09 +00:00

• Target Version set to 2.0.2

I've merged the patch with some minor changes:

• Split the patch into two separate commits
• Added author information
• Replaced readdir_r with readdir in the exception message