bug: user_name pointer is being used after it is freed

[vladimir-mencl-eresearch]

Hi,

Unfortunately, 7e60ec16c8b7f8b5fbf850e62fd190ffcdb495f4 re-introduces a bug fixed in #1: using the user_name pointer after it has been freed. That means at that point, the value it points to could have been corrupted by anything else happening in the system, or just overwritten by the memory manager for keeping track of free space (this behavior depends on the particular malloc implementation).

I am trying to understand the justification given in #1 for why to free the user_name pointer before setting home_dir.

Is it that to set the home directory to be set to just "/<zone>/home" ?

This would be best achieved by passing an empty string for the last parameter to the globus_common_create_string function. And this would be best done through a configuration entry.

But freeing the pointer and then using it again can lead to unpredictable results. If lucky, the first byte of the string the pointer points to gets set to '\0', causing it to appear as empty string. On my system, I am just getting plain garbage out of it - prior to also fixing the zone name, I was getting home_dir set to: "/(null)/home//(nu".

I might have a go at implementing this as a configuration item - would you think tweaking this via environment variables (passed from gridftp.conf) would be a good idea?

Cheers, Vlad

[muccix]

Hi Vladimir,

I understand your point. I will correct the code to avoid that unpredictable behavior.

Concerning the configuration of that item, it would be ok to use an environment variable to configure a variable at compile time that will be used to decide how to set the home dir (something similar to what is done with the env var "RESOURCE_MAP_PATH").

I would add it to the "setup.sh.template", but it should work also if you define the variable thought the gridftp.conf file.

What do you think?

Best, Roberto

[vladimir-mencl-eresearch]

Hi Roberto,

Thanks for getting back to me.

I've just recently stumbled upon how the log file name is configured in iRODS: it looks for an environment variable called "logfilePattern" and uses the contents of the variable as the format string (in this specific case passing it to strftime() ).

We should be doing the same - interpret the contents of the environment variable as a template. I can only think of two parameters to substitute, the zone and the user_name, but that should be enough.

The default value would be "/%s/home/%s", which would preserve the current default behaviour.

Specifying just "/%s/home" would cover your use case, sending all users to "/<zone>/home".

We might call the environment variable homeDirPattern (open to alternative suggestions).

Does this all sound reasonable to you?

If so, I'd be happy to take it on & implement and send in a pull request.

Cheers, Vlad

[muccix]

Hi Vlad,

let me double check if I understood correctly.

In the globus_gridftp_server_iRODS.c code, you would like to search for a env var (called for example "homeDirPattern") and if it is present, set the home dir pattern to the value of that env var (e.g. "/%s/home/%s" or "/%s/home").

The env variable should be set in the gridftp.conf.

If that is what you mean, it looks fine to me.

Just one note: since the current behavior is to set the home dir as "/<zone>/home", I would prefer to keep as default "/%s/home" in case no env variable is defined. In this way the current users don't need to modify anything updating to the new version.

Thanks again and best regards, Roberto

[vladimir-mencl-eresearch]

Hi Roberto,

Thanks for the reply. I've just come back from holidays and I'm now picking things up from where I left them.

The intention of the current code - line 588, https://github.com/EUDAT-B2STAGE/B2STAGE-GridFTP/blob/master/globus_gridftp_server_iRODS.c#L588:

finished_info.info.session.home_dir = globus_common_create_string("/%s/home/%s", iRODS_handle->zone, user_name);

appears to be to do "/<zone>/home/<username>" - and it was more just the interaction of this bug (using pointer after it is freed) and the particular malloc implementation interpreting such a pointer as empty string that resulted into defaulting to "/<zone>/home" on your system.

Also, the convention on most systems is to set the home directory to the actual home directory of the current user, not just the parent container directory.

I understand this scheme does not work for you, and I know if the default (together with the bugfix) changes the behaviour, you'd have to change the configuration file on your deployment .... but to me, that seem to be the right thing to do.

Would you be fine with that?

Cheers, Vlad

[muccix]

Hi Vlad,

I agree with you: let's set as default the full path, including the username.

Thanks again

Cheers, Roberto