Opened 13 years ago

Closed 12 years ago

Last modified 12 years ago

#349 closed defect (fixed)

Erroneous handling of broken links to the config file

Reported by: Jari Häkkinen Owned by: Peter Johansson
Priority: major Milestone: svndigest 0.7
Component: configuration Version: trunk
Keywords: Cc:

Description (last modified by Jari Häkkinen)

related to ticket:351

There should be a message telling the user about the problematic config file, i.e., a link that is broken. Havind done

cd .svndigest
ln -s ../.svndigest/config

This is a link to itself! Doing a cat on this file returns

# cat .svndigest/config
cat: .svndigest/config: Too many levels of symbolic links

whereas svndigest happily ignores the problem. Even if the link is to a non-existing file svndigest doesn't mind

# cat .svndigest/config 
cat: .svndigest/config: No such file or directory
# svndigest --no-report --copyright --verbose
Done parsing parameters
Initializing SVN singleton.
Checking target directory
Acquiring repository information
Building directory tree
Parsing directory tree

This is how we want svndigest to handle config

  1. If config file is given with --config-file complain if node does not exist (see ticket:351)
  2. If node exists, complain if node is not a file, or a symlink pointing to a file, or a symlink pointing to a symlink pointing to a file, etc. This also includes complaining if node is a dir or any other funky type of node (remember to catch the case where a link is a link to itself).
  3. If node exists but is not readable, complain (I think this already taken care of in main).
  4. If node exists but is not a svndigest config file, complain. Well this is not a clear-cut case, but incorrectly formatted config files should be taken care of by Configuration class. If the load function finds something funky, throw an exception.
  5. If no --config-file switch is given and there exists no /path/to/root/.svndigest/config, then silently use a default configuration.

Change History (19)

comment:1 Changed 13 years ago by Peter Johansson

Reading a config file from a symbolic link works for me. Can you please add more details.

comment:2 Changed 13 years ago by Jari Häkkinen

In my root WC directory I have a directory .svndigest with the config file

/path/to/WC/.svndigest/config

I want to run svndigest only on a branch of my repo but still use the same config like this

cd /path/to/WC/bin
mkdir .svndigest
cd .svndigest
ln -s ../../.svndigest/config .
cd ..
svndigest --copyright

The program runs but complains about missing aliases. If I change the link command above to copy instead then svndigest does not complain.

comment:3 Changed 13 years ago by Peter Johansson

What if you issue svndigest --generate-config? Is the output similar to the link or is it the default config file?

And you are using trunk version?

comment:4 in reply to:  3 Changed 13 years ago by Jari Häkkinen

Replying to peter:

What if you issue svndigest --generate-config? Is the output similar to the link or is it the default config file?

The same configuration file I link to works if I instead use a copy of it. My config file contains only two sections

### Section for setting aliases used in copyright update
[copyright-alias]

### Section for setting trac environment
[trac]
# If trac-root is set, svndigest will create anchors to the Trac page.

And you are using trunk version?

Yes.

The problem may be that the configuration reader opens the symbolic link entry in the operating system file table as an ordinary file. The file content of an symbolic link is a pointer to the file to be used instead of the symbolic link entry. I think the way forward is to print whatever the configuration reads.

comment:5 Changed 13 years ago by Jari Häkkinen

Milestone: svndigest 0.x+
Resolution: invalid
Status: newclosed

So, I found the problem. It was a user error ... the link was not pointing on the configuration file.

comment:6 Changed 13 years ago by Jari Häkkinen

Component: coreconfiguration
Description: modified (diff)
Milestone: svndigest 0.7
Resolution: invalid
Status: closedreopened
Summary: Reading of config file fails if it is a symbolic linkErroneous handling of broken links to the config file

I closed the ticket prematurely. There should be a message telling the user about the problematic config file, i.e., a link that is broken. In my case I had

cd .svndigest
ln -s ../.svndigest/config

This is a link to itself! Doing a cat on this file returns

# cat .svndigest/config
cat: .svndigest/config: Too many levels of symbolic links

Even if the link is to a non-existing file svndigest doesn't mind

# cat .svndigest/config 
cat: .svndigest/config: No such file or directory
# svndigest --no-report --copyright --verbose
Done parsing parameters
Initializing SVN singleton.
Checking target directory
Acquiring repository information
Building directory tree
Parsing directory tree

So, I simply change the description of this issue.

comment:7 Changed 13 years ago by Peter Johansson

Description: modified (diff)
Type: defectenhancement

ticket:351 was marked as related

comment:8 Changed 13 years ago by Peter Johansson

On line 71 in svndigest.cc there is a call

if (node_exist(option->config_file())) {

which probably returns false, and loading is skipped.

I think we should add a test for your case in Parameter::analyse.

comment:9 Changed 13 years ago by Peter Johansson

The question is: how to differentiate between node does not exists and node exist in form of a broken link.

comment:10 in reply to:  9 Changed 13 years ago by Jari Häkkinen

Replying to peter:

The question is: how to differentiate between node does not exists and node exist in form of a broken link.

The things it that the node exists, there is a link to something. The problem is that a link can point to it self, to a file that is not a config file, or to nothing (a broken link). I am excluding the case where the link actually points to a proper config file (this works in the current trunk version). The 'cat' program can detect a link that points to itself and also a broken link, so we can look into that program. The third case when a proper link exists but points to a file that is not a svndigest configuration file should be detected as a badly formatted config file is done now ... and now I understand your question and my answer; do we need to differentiate? Just say file not found in both cases. So, now there is only the case of a link to it self that must be resolved. Of course everything must also be coded into svndigest.

You don't agree this is a defect?

comment:11 Changed 13 years ago by Peter Johansson

Type: enhancementdefect

if you insist I could start think this is a defect

I think we could solve this by combining calls to stat and lstat

comment:12 Changed 13 years ago by Peter Johansson

btw previously we have not required that there is a config file. If there is no config file we use default settings. So I disagree with your statement that we don't need to differentiate and can complain about missing config.

comment:13 Changed 13 years ago by Peter Johansson

so to clarify, this is how I think it should work

1) If config file is given with --config-file complain if node does not exist (see ticket:351)

2) If node exists complain if node is not a file, or a symlink pointing to a file, or a symlink pointing to a symlink pointing to a file, etc. This also includes complaining if node is a dir or any other funky type of node.

3) If node exists but is not readable, complain (I think this already taken care of in main).

4) If node exists but is not a svndigest config file, complain. Well this is not a clear-cut case, but incorrectly formatted config files should be taken care of by Configuration class. If the load function finds something funky, throw an exception.

5) If no --config-file switch is given and there exists no /path/to/root/.svndigest/config, then silently use a default configuration.

comment:14 Changed 13 years ago by Jari Häkkinen

Description: modified (diff)

Perfect, I was just writing on this but could not submit. I moved your last entry do the description, and yes I made some modifications.

comment:15 Changed 13 years ago by Peter Johansson

ok if you have a symlink -> symlink, then

struct stat nodestat;
std::cout << stat("symlink",&nodestat) << "\n";
std::cout << lstat("symlink",&nodestat) << "\n";

will give you

-1
0

in other words, lstat tells us that there is a node and stat tells us that the link is not pointing to anything valid. I quote from man lstat:

Lstat() is like stat() except in the case where the named file is a symbolic link, in which case lstat() returns information about the link, while stat() returns information about the file the link references.

So I guess this gives us the recipe on how to differentiate.

comment:16 Changed 13 years ago by Peter Johansson

Owner: changed from Jari Häkkinen to Peter Johansson
Status: reopenednew

comment:17 Changed 13 years ago by Peter Johansson

Status: newassigned

comment:18 Changed 12 years ago by Peter Johansson

Resolution: fixed
Status: assignedclosed

(In [705]) importing classes for commandline parsing from yat. This fixes #349 and #265

comment:19 Changed 12 years ago by Peter Johansson

(In [708]) refs #349. Fixed so svndigest complains about broken symlink for config file also when no argument was given at cmd, i.e., default file is a broken symlink

Note: See TracTickets for help on using tickets.