On Fri, Nov 13, 2009 at 12:18:49AM -0500, Garrett Wollman wrote:
> If you have a moment, please take a look at the following patch. It
> contains some very minor fixes to various parts of libc which were
> found by the clang static analyzer. They fall into a few categories:
> 1) Bug fixes in very rare situations (mostly error-handling code that
> has probably never been executed).
> 2) Dead store elimination.
> 3) Elimination of unused variables. (Or in a few cases, making use of
> them.)
> Some minor style problems were also fixed in the vicinity. There
> should be no functional changes except in very unusual conditions.
This looks mostly ok, with a few exceptions.
> [...]
> Index: gen/getpwent.c
> ===================================================================
> --- gen/getpwent.c (revision 199242)
> +++ gen/getpwent.c (working copy)
> @@ -257,22 +257,19 @@
> [...]
> @@ -1876,7 +1871,6 @@
> syslog(LOG_ERR,
> "getpwent memory allocation failure");
> *errnop = ENOMEM;
> - rv = NS_UNAVAIL;
> break;
> }
> st->compat = COMPAT_MODE_NAME;
I think this change hides the wrongness in the handling of malloc errors
while leaving it as broken as it is.
> [...]
> Index: net/getservent.c
> ===================================================================
> --- net/getservent.c (revision 199242)
> +++ net/getservent.c (working copy)
> @@ -476,11 +476,11 @@
> struct nis_state *st;
> int rv;
>
> - enum nss_lookup_type how;
> char *name;
> char *proto;
> int port;
>
> + enum nss_lookup_type how;
> struct servent *serv;
> char *buffer;
> size_t bufsize;
> @@ -491,7 +491,8 @@
>
> name = NULL;
> proto = NULL;
> - how = (enum nss_lookup_type)mdata;
> +
> + how = (intptr_t)mdata;
> switch (how) {
> case nss_lt_name:
> name = va_arg(ap, char *);
In what way is this an improvement?
> Index: posix1e/acl_delete_entry.c
> ===================================================================
> --- posix1e/acl_delete_entry.c (revision 199242)
> +++ posix1e/acl_delete_entry.c (working copy)
> @@ -89,20 +89,20 @@
> return (-1);
> }
>
> - if ((acl->ats_acl.acl_cnt < 1) ||
> - (acl->ats_acl.acl_cnt > ACL_MAX_ENTRIES)) {
> + if ((acl_int->acl_cnt < 1) ||
> + (acl_int->acl_cnt > ACL_MAX_ENTRIES)) {
> errno = EINVAL;
> return (-1);
> }
If you are changing this code anyway, consider fixing a style bug
(extraneous parentheses) here.
> - for (i = 0; i < acl->ats_acl.acl_cnt;) {
> - if (_entry_matches(&(acl->ats_acl.acl_entry[i]), entry_d)) {
> + for (i = 0; i < acl_int->acl_cnt;) {
> + if (_entry_matches(&(acl_int->acl_entry[i]), entry_d)) {
> /* ...shift the remaining entries... */
> - for (j = i; j < acl->ats_acl.acl_cnt - 1; ++j)
> - acl->ats_acl.acl_entry[j] =
> - acl->ats_acl.acl_entry[j+1];
> + for (j = i; j < acl_int->acl_cnt - 1; ++j)
> + acl_int->acl_entry[j] =
> + acl_int->acl_entry[j+1];
> /* ...drop the count and zero the unused entry... */
> - acl->ats_acl.acl_cnt--;
> - bzero(&acl->ats_acl.acl_entry[j],
> + acl_int->acl_cnt--;
> + bzero(&acl_int->acl_entry[j],
> sizeof(struct acl_entry));
> acl->ats_cur_entry = 0;
>
--
Jilles Tjoelker
_______________________________________________
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
