View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0003995unrealircdpublic2010-12-31 12:252011-01-02 19:25
Reporterwarg Assigned Tosyzop  
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionno change required 
Product Version3.2.9-RC1 
Summary0003995: softlinks to modules are copied/hardlinked to tmp, not the files which the softlinks point to
Descriptioncurrently, if you have (for example, as I do):

./modules/cloak.so
./modules/commands.so
./modules/etc..
./modules/autoload/cloak.so # softlink to `../cloak.so'
./modules/autoload/commands.so # softlink to `../commands.so'

and in your unrealircd.conf, you do:
loadmodule "modules/autoload/cloak.so;
loadmodule "modules/autoload/commands.so;

the ircd will copy/hardlink the softlinks to tmp dir, not the modules they point to:

tmp/AB40C33.cloak.so # still a (hard/soft)link to ../cloak.so
tmp/6495DE24.commands.so # still a (hard/soft)link to ../commands.so

this is now a problem because ./cloak.so and ./commands.so doesn't exist, and so unreal fails to start.

I'm submitting this patch to follow the link, and thus enabling unreal to copy/hardlink the actual module.
Steps To Reproduceput your modules in ./modules/
create ./modules/autoload
put relative path (not absolute) softlinks in ./modules/autoload that point to ../commands.so.
in the unrealircd.conf put loadmodule "modules/autoload/commands.so";

now try to start unreal. it should fail, because it copied/hardlinked the softlink instead of the files they point to.
Additional InformationThis patch uses realpath in the unreal_copyfileex function to get the real path of the src object. This is a simple fix, and should work on POSIX and Windows.
Attached Files
follow_symlinks.diff (646 bytes)
follow_symlinks2.patch (829 bytes)
unreal-3995-nohardlink-symlinks.patch (805 bytes)
unreal-3995-nohardlink-symlinks-r2.patch (833 bytes)
3rd party modules

Activities

syzop

2010-12-31 16:47

administrator   ~0016556

Last edited: 2010-12-31 16:50

 Could you add some error checking, like if realpath() returns NULL?

Oh, and:
char* buf[2048];
?? ;)
You might want to drop the asterisk.

warg

2010-12-31 19:04

reporter   ~0016558

Yeah, new patch attached.

syzop

2011-01-01 18:27

administrator   ~0016567

Much better ;)

One thing:
+ return 0;
+ } else {

if you return (which is correct), then get rid of the else clause and the unneeded indentation after that.

ohnobinki: since you seem to be watching the issue... realpath() man page says 'Avoid using this function. It is broken by design' and has various warnings regarding buffer size. Do you see a better function? I see canonicalize_file_name() but it's GNU only. readlink() would fit, but it's not the same as realpath() and symlink-to-symlink would still not work. What do you think, should we just go with this patch anyway? Maybe up the buffer to 4K? It seems like picking some arbitrary (big) number... Or any other suggestions?

syzop

2011-01-01 18:45

administrator   ~0016568

See also (other than the man page):

http://insanecoding.blogspot.com/2007/11/pathmax-simply-isnt.html

https://www.securecoding.cert.org/confluence/download/attachments/3524/realpath.pdf

I only quickly glanced over it, but.. this definitely needs some more attention prior to inclusion.

syzop

2011-01-01 19:04

administrator   ~0016569

What OS are you on warg?

We don't hardlink modules anymore since... well at least in the version you are using.

        /* For OpenBSD, do not do a hardlinkink attempt first because it checks inode
         * numbers to see if a certain module is already loaded. -- Syzop
         * EDIT (2009): Looks like Linux got smart too, from now on we always copy....
         */
        ret = unreal_copyfileex(path, tmppath, 0);

Odd that you are having problems then?

syzop

2011-01-01 19:30

administrator   ~0016570

Cannot reproduce this issue.

ohnobinki

2011-01-02 04:47

reporter   ~0016573

Whether or not it matters now, I saw this in the RATIONALE section of the POSIX readlink manpage:

``The behavior specified for readlink() when bufsiz is zero represents historical practice. For this case, the standard developers considered a change whereby readlink() would return the number of non-null bytes contained in the symbolic link with the buffer buf remaining unchanged; however, since the stat structure member st_size value can be used to determine the size of buffer necessary to contain the contents of the symbolic link as returned by readlink(), this proposal was rejected, and the historical practice retained.''

Thus, picking arbitrary buffer sizes is unnecessary with the help of stat(). If it matters, we could write our own canonicalize_file_name() and choose whether or not to compile that function in a manner similar to strlcpy() and the like.

However, we could probably just detect whether or not a file is a symlink in unreal_copyfileex() and then not try to hardlink it in that case... which would be the simplest way to walk around the issue.

ohnobinki

2011-01-02 04:57

reporter   ~0016576

unreal-3995-nohardlink-symlinks.patch: Untested attempt at automatically disabling hardlinking of symlinks.

May you check if this fixes your problem warg? Thanks :-).

ohnobinki

2011-01-02 05:25

reporter   ~0016577

unreal-3995-nohardlink-symlinks-r2.patch: be more efficient.

As syzop said, we no longer hardlink modules (0003557). And I can't find any use of unreal_copyfileex() where the source file might possibly be a symlink other than modules which aren't hardlinked anymore. So we could commit this to make unreal_copyfileex() hardier without actually fixing anything ;-).

warg

2011-01-02 19:15

reporter   ~0016578

I was apparently in my 3.2.8 tree, not rc tree. I apologize for the absurd waste of time.

syzop

2011-01-02 19:25

administrator   ~0016579

Closing.

ohnobinki: indeed, while your patch is technically OK, there's no scenario when it will be needed, so no need to apply.

Issue History

Date Modified Username Field Change
2010-12-31 12:25 warg New Issue
2010-12-31 12:25 warg File Added: follow_symlinks.diff
2010-12-31 16:47 syzop Note Added: 0016556
2010-12-31 16:50 syzop Note Edited: 0016556
2010-12-31 19:04 warg File Added: follow_symlinks2.patch
2010-12-31 19:04 warg Note Added: 0016558
2011-01-01 18:27 syzop Note Added: 0016567
2011-01-01 18:45 syzop Note Added: 0016568
2011-01-01 18:45 syzop Relationship added child of 0003915
2011-01-01 19:04 syzop Note Added: 0016569
2011-01-01 19:30 syzop Note Added: 0016570
2011-01-02 04:47 ohnobinki Note Added: 0016573
2011-01-02 04:55 ohnobinki File Added: unreal-3995-nohardlink-symlinks.patch
2011-01-02 04:57 ohnobinki Note Added: 0016576
2011-01-02 05:19 ohnobinki File Added: unreal-3995-nohardlink-symlinks-r2.patch
2011-01-02 05:25 ohnobinki Note Added: 0016577
2011-01-02 19:15 warg Note Added: 0016578
2011-01-02 19:25 syzop Note Added: 0016579
2011-01-02 19:25 syzop Status new => closed
2011-01-02 19:25 syzop Assigned To => syzop
2011-01-02 19:25 syzop Resolution open => no change required
2011-01-02 19:25 syzop Relationship deleted child of 0003915