apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gang Shu <gangsh...@gmail.com>
Subject Bugs discovered by a research group at CWRU (2)
Date Thu, 20 Aug 2009 05:08:14 GMT
Dear Apache Portable Runtime (APR) developers:



I am a Ph.D student in the Software Engineering Research Group in Case
Western Reserve University, under the instruction of  Prof. Andy
Podgurski.  In our recent research we analyzed some of your fixed bugs
in your issue data base as well as some revisions which indicate
fixing a bug, and try to find out whether there are similar bugs left
in the code base which are left unfixed.



We applied our approach in your newest release APR
1.3.8 and we have identified a few potential bugs as follows.



It would be greatly appreciated if you can reply to this
email after you have gone over the bugs and tell  us whether you have
confirmed any of them, since these information are really valuable for
us for testing our current method.









*1. Analyzed bug-fix: revision 537393*

(http://svn.apache.org/viewvc?view=rev&revision=537393)



The log of this revision is as follows:

“Improve thread safety of assorted file_io functions. Patches by Davi
Arnaut.”



So we believe that the critical resource “thefile” should be protected when
it is used in the function “write(thefile->filedes, buf, *nbytes)”.

In the bug-fix, the “thefile” has been protected by “file_lock(thefile)” and
“file_unlock(thefile)” functions, as well as the code block
“write(thefile->filedes, buf, *nbytes)” has been put into a new function
“apr_file_flush_locked(thefile)”.

We’ve found two potential bugs where “file_lock(thefile)” and
“file_unlock(thefile)” are missing.



**************************original bug-fix**********************************



Filename: file_io/unix/readwrite.c, Function: apr_file_flush ()**



334     if (thefile->buffered) {

335 +        file_lock(thefile);

336 +        rv = apr_file_flush_locked(thefile);

337 +        file_unlock(thefile);

………….

309 + apr_status_t apr_file_flush_locked(apr_file_t *thefile)

310 + {

311 +     apr_status_t rv = APR_SUCCESS;

312 +

313 +     if (thefile->direction == 1 && thefile->bufpos) {

314 +         apr_ssize_t written;

315 +

316 +         do {

317 +             written = write(thefile->filedes, thefile->buffer,
thefile->bufpos);

318 +         } while (written == -1 && errno == EINTR);

319 +         if (written == -1) {

320 +             rv = errno;

321 +         } else {

322 +             thefile->filePtr += written;

323 +             thefile->bufpos = 0;

324 +         }

325 +     }

326 +

327 +    return rv;

328 + }



**************************discovered possible new bug(s)***********************

(1.1) Filename: file_io/unix/readwrite.c, Function: apr_file_write()



187                    do {

188                        rv = write(thefile->filedes, buf, *nbytes);

189                   } while (rv == (apr_size_t)-1 && errno == EINTR);





(1.2) Filename: file_io/unix/readwrite.c, Function: apr_file_write()



200                do {

201                    do {

202                        rv = write(thefile->filedes, buf, *nbytes);

203                    } while (rv == (apr_size_t)-1 && errno == EINTR);

204                    if (rv == (apr_size_t)-1 &&

205                        (errno == EAGAIN || errno == EWOULDBLOCK)) {

206                        *nbytes /= 2; /* yes, we'll loop if kernel lied

207                                       * and we can't even write 1 byte

208                                       */

209                    }

210                    else {

211                        break;

212                    }

213                } while (1);









*2. Analyzed bug-fix: 46425*

(https://issues.apache.org/bugzilla/show_bug.cgi?id=46425)



The comment of this bug-fix is as follows:

“apr should set FD_CLOEXEC if APR_FOPEN_NOCLEANUP is not set. This would in
general help with security and fix some existing issues with third party
httpd modules. Patches by Stefan Fritsch.”



 “there is a (possibly brief) period of time between the return of the
open() call or other function creating a file descriptor and the fcntl()
call to set the flag", so we believe that an object is changed to rely on
FD_CLOEXEC for closure after exec, the corresponding child cleanup should to
be changed since it will otherwise also try to close() the fd for a second
time.



**************************original bug-fix**********************************



Filename: file_io/unix/mktemp.c, Function: apr_file_mktemp()



206      if (!(flags & APR_FILE_NOCLEANUP)) {

207 +        int flags;

208 +

209 +       if ((flags = fcntl(fd, F_GETFD)) == -1)

210 +            return errno;

211 +

212 +        flags |= FD_CLOEXEC;

213 +        if (fcntl(fd, F_SETFD, flags) == -1)

214 +            return errno;

215 +

216          apr_pool_cleanup_register((*fp)->pool, (void *)(*fp),

217                                    apr_unix_file_cleanup,

218                                    apr_unix_child_file_cleanup);

219      }



**************************discovered possible new bug(s)***********************

(2.1) Filename: file_io/unix/open.c, Function: apr_file_open()



214    if (!(flag & APR_FILE_NOCLEANUP)) {

215        apr_pool_cleanup_register((*new)->pool, (void *)(*new),

216                                  apr_unix_file_cleanup,

217                                  apr_unix_child_file_cleanup);

218    }









*3. Analyzed bug-fix: 46425*

(https://issues.apache.org/bugzilla/show_bug.cgi?id=46425)



The comment of this bug-fix is as follows:

“apr should set FD_CLOEXEC if APR_FOPEN_NOCLEANUP is not set. This would in
general help with security and fix some existing issues with third party
httpd modules. Patches by Stefan Fritsch.”



**************************original bug-fix**********************************



Filename: file_io/unix/mktemp.c, Function: apr_file_mktemp()



176 apr_status_t apr_socket_accept(apr_socket_t **new, apr_socket_t *sock,

177                                apr_pool_t *connection_context)

………

195     alloc_socket(new, connection_context);

……..

276         (*new)->local_interface_unknown = 1;

277     }

278

279 + #ifndef HAVE_ACCEPT4

280 +    {

281 +           int flags;

282 +

283 +           if ((flags = fcntl((*new)->socketdes, F_GETFD)) == -1)

284 +               return errno;

285 +

286 +           flags |= FD_CLOEXEC;

287 +           if (fcntl((*new)->socketdes, F_SETFD, flags) == -1)

288 +            return errno;

289 +       }

290 +   #endif

291

292     (*new)->inherit = 0;



**************************discovered possible new bug(s)***********************

(3.1) Filename: network_io/unix/sockets.c, Function: apr_os_sock_make()



411 apr_status_t apr_os_sock_make(apr_socket_t **apr_sock,

412                               apr_os_sock_info_t *os_sock_info,

413                               apr_pool_t *cont)

414 {

415     alloc_socket(apr_sock, cont);

……..

439     else {

440         (*apr_sock)->remote_addr_unknown = 1;

441     }

442

443     (*apr_sock)->inherit = 0;









Thank you very much!



Sincerely,

Gang Shu



Computer Science Division, EECS

513 Olin Building

Case Western Reserve University

10900 Euclid Avenue

Cleveland, OH 44106

Email: gang.shu@case.edu

Mime
View raw message