Re: MAXPATHLEN in ncpfs
On 09/07/2013 05:07 PM, Thomas Danckaert wrote:
Hello,
Hi!
attached patch fixes the MAXPATHLEN build problems for ncpfs. Is this
the right mailing list for discussion, or should I send this message
to bug-hurd? (Or directly submit a bug+patch to the Debian bug
tracking system?)
I think you should file a bug report and send your patch.
One issue with the patch, that I see, but don't know how to solve, is
the following: the char* mount_point is passed to library users as a
result of the function ncp_find_permanent(), exposed in the header
ncplib.h. Before, the function returned a pointer to the static
char[MAXPATHLEN] array, so the api user was not allowed to free() it,
but now it is a pointer to a malloc'ed buffer. With this patch, the
buffer is free'd on every function call, so there should be no real
memory leak as far as i see. However, it is not properly cleaned up
at program exit, and giving the user a pointer which is later free'd
might cause other problems as well (though the content of the array it
pointed to would have changed after every function call in the
original version as well, at least the pointer would have still been
valid).
I don't think that not freeing memory on exit is a huge issue, since the
OS should take care of that.
I inlined some comments. I don't really know ncpfs, but this looks good
to me.
kind regards,
Thomas Danckaert
remove-maxpathlen.patch
Avoid dependence on MAXPATHLEN.
replace fixed-length character array ncp_conn_ent.mount_point by a
char*.
Index: ncpfs_fix/include/ncp/ncplib.h
===================================================================
--- ncpfs_fix.orig/include/ncp/ncplib.h 2013-09-07 15:12:30.000000000 +0200
+++ ncpfs_fix/include/ncp/ncplib.h 2013-09-07 16:59:04.454074383 +0200
@@ -393,7 +393,7 @@
fixedCharArray server[NCP_BINDERY_NAME_LEN];
char* user;
uid_t uid;
- fixedCharArray mount_point[MAXPATHLEN];
+ char* mount_point;
};
#else
struct ncp_conn_ent
@@ -401,7 +401,7 @@
char server[NCP_BINDERY_NAME_LEN];
char* user;
uid_t uid;
- char mount_point[MAXPATHLEN];
+ char* mount_point;
};
#endif
Index: ncpfs_fix/lib/ncplib.c
===================================================================
--- ncpfs_fix.orig/lib/ncplib.c 2013-09-07 15:12:30.000000000 +0200
+++ ncpfs_fix/lib/ncplib.c 2013-09-07 17:04:32.982060643 +0200
@@ -2139,6 +2139,9 @@
struct mntent *mnt_ent;
int fid;
+ if( entry.mount_point != NULL ) {
+ free (entry.mount_point);
+ }
free(NULL) is a no-op, so you should drop the if(...).
memset(server, 0, sizeof(server));
memset(&entry, 0, sizeof(entry));
@@ -2158,10 +2161,16 @@
*user = '\0';
user += 1;
entry.user = user;
- if ((strlen(server) >= sizeof(entry.server))
- || (strlen(mnt_ent->mnt_dir) >= sizeof(entry.mount_point))) {
+ if (strlen(server) >= sizeof(entry.server)) {
continue;
}
+#ifdef MAXPATHLEN
+ if ( strlen(mnt_ent->mnt_dir) >= MAXPATHLEN) {
+ continue;
+ }
+#endif
You could probably use only one condition, like this:
if ((strlen(server) >= sizeof(entry.server))
#ifdef MAXPATHLEN
|| (strlen(mnt_ent->mnt_dir) >= MAXPATHLEN))
#endif
) {
continue;
}
I think you could even remove the extraneous parentheses, since, iirc,
">=" takes precedence over "||".
+ entry.mount_point = malloc(strlen(mnt_ent->mnt_dir) + 1);
+
strcpy(entry.server, server);
strcpy(entry.mount_point, mnt_ent->mnt_dir);
@@ -2242,35 +2251,49 @@
static NWCCODE
ncp_fopen_nwc(FILE** nwc)
{
- char path[MAXPATHLEN];
+ char *path;
char *home = NULL;
struct stat st;
FILE* f;
+ int retval = 0;
+ int pathlen;
This looks weirdly indented.
home = getenv("HOME");
- if ((home == NULL)
- || (strlen(home) + sizeof(NWCLIENT) + 2 > sizeof(path))) {
+ if (home == NULL) {
return ENAMETOOLONG;
}
+ pathlen = strlen(home) + strlen(NWCLIENT) + 2;
+#ifdef MAXPATHLEN
+ if (pathlen > MAXPATHLEN) {
+ return ENAMETOOLONG;
+ }
+#endif
Same as above.
+ path = malloc(pathlen);
strcpy(path, home);
strcat(path, "/");
strcat(path, NWCLIENT);
if (stat(path, &st) != 0) {
- return errno;
+ retval = errno;
+ goto cleanup;
}
if (st.st_uid != getuid()) {
- return EACCES;
+ retval = EACCES;
+ goto cleanup;
}
if ((st.st_mode & (S_IRWXO | S_IRWXG)) != 0) {
- return NCPLIB_INVALID_MODE;
+ retval = NCPLIB_INVALID_MODE;
+ goto cleanup;
}
f = fopen(path, "r");
if (!f) {
- return errno;
+ retval = errno;
+ goto cleanup;
}
*nwc = f;
- return 0;
+ cleanup:
+ free(path);
+ return retval;
}
NWCCODE
Cyril.
Reply to: