[ltt-dev] [UST PATCH 1/6] Make app socket directories per-user v2

[Nils Carlson]

On Mar 31, 2011, at 9:27 PM, Mathieu Desnoyers wrote:

> * Nils Carlson (nils.carlson at ericsson.com) wrote:
>> Changes since v1:
>> 	* Document memory allocation
>>
>> Make a separate app socket directories for each user, providing
>> some basic security and also the possibility of consistent cleanup.
>>
>> Signed-off-by: Nils Carlson <nils.carlson at ericsson.com>
>> ---
>> libust/tracectl.c    |   34 +++++++++++++++++++---------------
>> libustcomm/ustcomm.c |   34 +++++++++++++++++++++++++++++-----
>> libustcomm/ustcomm.h |    4 ++++
>> 3 files changed, 52 insertions(+), 20 deletions(-)
>>
>> diff --git a/libust/tracectl.c b/libust/tracectl.c
>> index 33c7280..ae92b7e 100644
>> --- a/libust/tracectl.c
>> +++ b/libust/tracectl.c
>> @@ -1221,37 +1221,41 @@ static void auto_probe_connect(struct  
>> marker *m)
>>
>> static struct ustcomm_sock * init_app_socket(int epoll_fd)
>
> Hi Nils,
>
> I think we really need to get a straight naming scheme for init vs  
> alloc
> functions.
>
> init = initialization of a structure (never allocates)
> alloc = allocation of a structure (maybe can also initialize it, but  
> not
> necessarily. This would be specified by the function header
> documentation)
>
> Otherwise reviewing correct allocation and free of a patch like this  
> one
> is made much more difficult than it should, and the problem will only
> grow as we add more code.
>

Sure, but this patch doesn't change the name of the function or its  
calling. I expect that would be a separate patch fixing consistency  
issues.

As far as strings are concerned though adding alloc and init is just  
confusing I think.

/Nils

> Thank you,
>
> Mathieu
>
>> {
>> -	char *name;
>> +	char *dir_name, *sock_name;
>> 	int result;
>> -	struct ustcomm_sock *sock;
>> +	struct ustcomm_sock *sock = NULL;
>>
>> -	result = asprintf(&name, "%s/%d", SOCK_DIR, (int)getpid());
>> +	dir_name = ustcomm_user_sock_dir();
>> +	if (!dir_name)
>> +		return NULL;
>> +
>> +	result = asprintf(&sock_name, "%s/%d", dir_name, (int)getpid());
>> 	if (result < 0) {
>> 		ERR("string overflow allocating socket name, "
>> 		    "UST thread bailing");
>> -		return NULL;
>> +		goto free_dir_name;
>> 	}
>>
>> -	result = ensure_dir_exists(SOCK_DIR);
>> +	result = ensure_dir_exists(dir_name);
>> 	if (result == -1) {
>> 		ERR("Unable to create socket directory %s, UST thread bailing",
>> -		    SOCK_DIR);
>> -		goto free_name;
>> +		    dir_name);
>> +		goto free_sock_name;
>> 	}
>>
>> -	sock = ustcomm_init_named_socket(name, epoll_fd);
>> +	sock = ustcomm_init_named_socket(sock_name, epoll_fd);
>> 	if (!sock) {
>> 		ERR("Error initializing named socket (%s). Check that directory"
>> -		    "exists and that it is writable. UST thread bailing", name);
>> -		goto free_name;
>> +		    "exists and that it is writable. UST thread bailing",  
>> sock_name);
>> +		goto free_sock_name;
>> 	}
>>
>> -	free(name);
>> -	return sock;
>> +free_sock_name:
>> +	free(sock_name);
>> +free_dir_name:
>> +	free(dir_name);
>>
>> -free_name:
>> -	free(name);
>> -	return NULL;
>> +	return sock;
>> }
>>
>> static void __attribute__((constructor)) init()
>> diff --git a/libustcomm/ustcomm.c b/libustcomm/ustcomm.c
>> index 43f4289..dce1e52 100644
>> --- a/libustcomm/ustcomm.c
>> +++ b/libustcomm/ustcomm.c
>> @@ -533,6 +533,21 @@ close_sock:
>> 	return -1;
>> }
>>
>> +/* Returns the current users socket directory, must be freed */
>> +char *ustcomm_user_sock_dir(void)
>> +{
>> +	int result;
>> +	char *sock_dir = NULL;
>> +
>> +	result = asprintf(&sock_dir, "%s%s", USER_SOCK_DIR,
>> +			  cuserid(NULL));
>> +	if (result < 0) {
>> +		ERR("string overflow allocating directory name");
>> +		return NULL;
>> +	}
>> +
>> +	return sock_dir;
>> +}
>>
>> /* Open a connection to a traceable app.
>>  *
>> @@ -545,21 +560,30 @@ int ustcomm_connect_app(pid_t pid, int *app_fd)
>> {
>> 	int result;
>> 	int retval = 0;
>> -	char *name;
>> +	char *dir_name, *sock_name;
>> +
>> +	dir_name = ustcomm_user_sock_dir();
>> +	if (!dir_name)
>> +		return -ENOMEM;
>>
>> -	result = asprintf(&name, "%s/%d", SOCK_DIR, pid);
>> +	result = asprintf(&sock_name, "%s/%d", dir_name, pid);
>> 	if (result < 0) {
>> 		ERR("failed to allocate socket name");
>> -		return -1;
>> +		retval = -1;
>> +		goto free_dir_name;
>> 	}
>>
>> -	result = ustcomm_connect_path(name, app_fd);
>> +	result = ustcomm_connect_path(sock_name, app_fd);
>> 	if (result < 0) {
>> 		ERR("failed to connect to app");
>> 		retval = -1;
>> +		goto free_sock_name;
>> 	}
>>
>> -	free(name);
>> +free_sock_name:
>> +	free(sock_name);
>> +free_dir_name:
>> +	free(dir_name);
>>
>> 	return retval;
>> }
>> diff --git a/libustcomm/ustcomm.h b/libustcomm/ustcomm.h
>> index 0ec04fc..db38119 100644
>> --- a/libustcomm/ustcomm.h
>> +++ b/libustcomm/ustcomm.h
>> @@ -25,6 +25,7 @@
>> #include <ust/kcompat/kcompat.h>
>>
>> #define SOCK_DIR "/tmp/ust-app-socks"
>> +#define USER_SOCK_DIR "/tmp/ust-socks-"
>>
>> struct ustcomm_sock {
>> 	struct cds_list_head list;
>> @@ -156,6 +157,9 @@ extern int ustcomm_req(int sock,
>> 		       char *res_data);
>>
>> extern int ustcomm_request_consumer(pid_t pid, const char *channel);
>> +
>> +/* Returns the current users socket directory, must be freed */
>> +extern char *ustcomm_user_sock_dir(void);
>> extern int ustcomm_connect_app(pid_t pid, int *app_fd);
>> extern int ustcomm_connect_path(const char *path, int  
>> *connection_fd);
>>
>> -- 
>> 1.7.1
>>
>>
>> _______________________________________________
>> ltt-dev mailing list
>> ltt-dev at lists.casi.polymtl.ca
>> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
>>
>
> -- 
> Mathieu Desnoyers
> Operating System Efficiency R&D Consultant
> EfficiOS Inc.
> http://www.efficios.com
>
> _______________________________________________
> ltt-dev mailing list
> ltt-dev at lists.casi.polymtl.ca
> http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev