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: [email protected]
