On Wed, Jul 01, 2015 at 02:52:45PM +0200, Patrik Jakobsson wrote:
[...]
> --- a/defs.h
> +++ b/defs.h
> @@ -266,6 +266,13 @@ struct tcb {
> int u_error; /* Error code */
> long scno; /* System call number */
> long u_arg[MAX_ARGS]; /* System call arguments */
> +
> + /*
> + * Private data for the decoding functions of the syscall. TCB core does
> + * _not_ handle allocation / deallocation of this data.
> + */
> + void *priv_data;
> +
This will result to memory leaks if droptcb() is called before the
syscall parser that allocated memory had a chance to deallocate it.
As this data is no longer relevant after leaving trace_syscall_exiting(),
I suggest to perform deallocation directly from trace_syscall_exiting.
This API could be made more flexible by adding another pointer -
the function to be called to deallocate memory, e.g.
struct tcb {
...
void *priv_data;
void (*free_priv_data)(void *);
...
};
...
void
free_priv_data(struct tcb *tcp)
{
if (tcp->priv_data) {
if (tcp->free_priv_data) {
tcp->free_priv_data(tcp->priv_data);
tcp->free_priv_data = NULL;
}
tcp->priv_data = NULL;
}
}
...
droptcb(struct tcb *tcp)
{
...
free_priv_data(tcp);
...
}
...
trace_syscall_exiting(struct tcb *tcp)
{
...
ret:
free_priv_data(tcp);
...
}
[...]
On Wed, Jul 01, 2015 at 02:52:46PM +0200, Patrik Jakobsson wrote:
> * Makefile.am: Add compilation of drm.c
> * defs.h: Declarations of drm functions
> * drm.c: Utility functions for drm driver detection
> * io.c: Dispatch drm ioctls
> * ioctl.c: Distpatch generic and driver specific ioctls
This is not quite a GNU style changelog entry. Please have a look at
http://www.gnu.org/prep/standards/html_node/Style-of-Change-Logs.html
and examples in strace.git history.
[...]
> +#include "defs.h"
> +
> +#include <drm.h>
> +#include <linux/limits.h>
Please include <sys/param.h> instead of <linux/limits.h>.
> +#define DRM_MAX_NAME_LEN 128
> +
> +struct drm_ioctl_priv {
> + char name[DRM_MAX_NAME_LEN];
> +};
> +
> +inline int drm_is_priv(const unsigned int num)
> +{
> + return (_IOC_NR(num) >= DRM_COMMAND_BASE &&
> + _IOC_NR(num) < DRM_COMMAND_END);
> +}
> +
> +static int drm_get_driver_name(struct tcb *tcp, char *name, size_t bufsize)
> +{
> + char path[PATH_MAX];
> + char link[PATH_MAX];
> + int ret;
> +
> + ret = getfdpath(tcp, tcp->u_arg[0], path, PATH_MAX - 1);
> + if (ret < 0)
> + return ret;
> +
> + snprintf(link, PATH_MAX, "/sys/class/drm/%s/device/driver",
> + basename(path));
> +
> + ret = readlink(link, path, PATH_MAX - 1);
> + if (ret < 0)
> + return ret;
> +
> + path[ret] = '\0';
> + strncpy(name, basename(path), bufsize);
> +
> + return 0;
> +}
I think this is getting too complicated. This function could just return
strdup(basename(path)) or NULL in case of any error:
static char *
drm_get_driver_name(struct tcb *tcp, const char *name)
{
char path[PATH_MAX];
char link[PATH_MAX];
int ret;
if (getfdpath(tcp, tcp->u_arg[0], path, PATH_MAX - 1) < 0)
return NULL;
if (snprintf(link, PATH_MAX, "/sys/class/drm/%s/device/driver",
basename(path)) >= PATH_MAX)
return NULL;
ret = readlink(link, path, PATH_MAX - 1);
if (ret < 0)
return NULL;
path[ret] = '\0';
return strdup(basename(path));
}
> +
> +int drm_is_driver(struct tcb *tcp, const char *name)
> +{
> + struct drm_ioctl_priv *priv;
> + int ret;
> +
> + /*
> + * If no private data is allocated we are detecting the driver name for
> + * the first time and must resolve it.
> + */
> + if (tcp->priv_data == NULL) {
> + tcp->priv_data = xcalloc(1, sizeof(struct drm_ioctl_priv));
xcalloc shouldn't be used if a potential memory allocation error is not
fatal. In a parser that performs verbose syscall decoding no memory
allocation error is fatal.
> + priv = tcp->priv_data;
> +
> + ret = drm_get_driver_name(tcp, priv->name, DRM_MAX_NAME_LEN);
> + if (ret)
> + return 0;
> + }
> +
> + priv = tcp->priv_data;
> +
> + return strncmp(name, priv->name, DRM_MAX_NAME_LEN) == 0;
Then with priv_data+free_priv_data interface this would looks smth like
...
if (!tcp->priv_data) {
tcp->priv_data = drm_get_driver_name(tcp, name);
if (tcp->priv_data) {
tcp->free_priv_data = free;
} else {
tcp->priv_data = (void *) "";
tcp->free_priv_data = NULL;
}
}
return !strcmp(name, (char *) tcp->priv_data);
> +}
> +
> +int drm_decode_number(struct tcb *tcp, unsigned int arg)
This is an ioctl request code, let's consistently call it "code" to
distinguish from its argument.
[...]
> --- a/ioctl.c
> +++ b/ioctl.c
> @@ -182,7 +182,7 @@ hiddev_decode_number(unsigned int arg)
> }
>
> int
> -ioctl_decode_command_number(unsigned int arg)
> +ioctl_decode_command_number(struct tcb *tcp, unsigned int arg)
I've already changed ioctl_decode_command_number's signature:
struct tcb * is already there and "arg" is now called "code".
--
ldv
pgpTeCUn8sbKO.pgp
Description: PGP signature
------------------------------------------------------------------------------ Don't Limit Your Business. Reach for the Cloud. GigeNET's Cloud Solutions provide you with the tools and support that you need to offload your IT needs and focus on growing your business. Configured For All Businesses. Start Your Cloud Today. https://www.gigenetcloud.com/
_______________________________________________ Strace-devel mailing list Strace-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/strace-devel
