Re: [PATCH,HURD] sendto: do not crash when addr is NULL
Hi, Alle mercoledì 13 giugno 2012, Pino Toscano ha scritto: Alle martedì 12 giugno 2012, Roland McGrath ha scritto: That's almost right, but you should avoid calling __mach_port_deallocate if APORT is MACH_PORT_NULL. Also, avoid shadowing the local variable ERR with one of the same name in the subfunction. Hm ok, I saw other cases of shadowing by internal subroutines, and assumed it was okay (since inside them you want to refer to the local err instead of the outermost one. Thanks for the further review, patch attached. (I would use the non-ChangeLog lines of the patch header as commit log, would that be okay?) Ping, about this and the other glibc patches I sent and that are awaiting review. -- Pino Toscano signature.asc Description: This is a digitally signed message part.
Re: [PATCH,HURD] sendto: do not crash when addr is NULL
Hm ok, I saw other cases of shadowing by internal subroutines, and assumed it was okay (since inside them you want to refer to the local err instead of the outermost one. It's not the worst thing ever, and we don't compile with -Wshadow. But shadowing is usually a good thing to avoid just to make reading the code easier to follow. Thanks, Roland
Re: [PATCH,HURD] sendto: do not crash when addr is NULL
This looks fine to me.
Re: [PATCH,HURD] sendto: do not crash when addr is NULL
Hi,
Alle martedì 12 giugno 2012, Roland McGrath ha scritto:
That's almost right, but you should avoid calling
__mach_port_deallocate if APORT is MACH_PORT_NULL. Also, avoid
shadowing the local variable ERR with one of the same name in the
subfunction.
Hm ok, I saw other cases of shadowing by internal subroutines, and
assumed it was okay (since inside them you want to refer to the local
err instead of the outermost one.
Thanks for the further review, patch attached.
(I would use the non-ChangeLog lines of the patch header as commit log,
would that be okay?)
--
Pino Toscano
Hurd: sendto: do not crash when ADDR is NULL
Create a new create_address_port subroutine to isolate the address port creation
(for both local and remove sockets), and use it inside HURD_DPORT_USE.
Also intialize APORT to MACH_PORT_NULL and make sure to always deallocate it,
when not null.
2012-06-13 Pino Toscano toscano.p...@tiscali.it
* sysdeps/mach/hurd/sendto.c (create_address_port): New subroutine.
(__sendto): Use create_address_port. Initialize APORT and deallocate
it if not null.
--- a/sysdeps/mach/hurd/sendto.c
+++ b/sysdeps/mach/hurd/sendto.c
@@ -33,37 +33,50 @@ __sendto (int fd,
const struct sockaddr_un *addr,
socklen_t addr_len)
{
- addr_port_t aport;
+ addr_port_t aport = MACH_PORT_NULL;
error_t err;
size_t wrote;
- if (addr-sun_family == AF_LOCAL)
+ /* Get an address port for the desired destination address. */
+ error_t create_address_port (io_t port,
+ const struct sockaddr_un *addr,
+ socklen_t addr_len,
+ addr_port_t *aport)
{
- /* For the local domain, we must look up the name as a file and talk
- to it with the ifsock protocol. */
- file_t file = __file_name_lookup (addr-sun_path, 0, 0);
- if (file == MACH_PORT_NULL)
- return -1;
- err = __ifsock_getsockaddr (file, aport);
- __mach_port_deallocate (__mach_task_self (), file);
- if (err == MIG_BAD_ID || err == EOPNOTSUPP)
- /* The file did not grok the ifsock protocol. */
- err = ENOTSOCK;
- if (err)
- return __hurd_fail (err);
+ error_t err_port;
+
+ if (addr-sun_family == AF_LOCAL)
+ {
+ /* For the local domain, we must look up the name as a file and talk
+ to it with the ifsock protocol. */
+ file_t file = __file_name_lookup (addr-sun_path, 0, 0);
+ if (file == MACH_PORT_NULL)
+ return errno;
+ err_port = __ifsock_getsockaddr (file, aport);
+ __mach_port_deallocate (__mach_task_self (), file);
+ if (err_port == MIG_BAD_ID || err_port == EOPNOTSUPP)
+ /* The file did not grok the ifsock protocol. */
+ err_port = ENOTSOCK;
+ }
+ else
+ {
+ err_port = __socket_create_address (port,
+ addr-sun_family,
+ (char *) addr,
+ addr_len,
+ aport);
+ }
+
+ return err_port;
}
- else
-err = EIEIO;
- /* Get an address port for the desired destination address. */
err = HURD_DPORT_USE (fd,
({
- if (err)
- err = __socket_create_address (port,
- addr-sun_family,
- (char *) addr,
- addr_len,
- aport);
+ if (addr != NULL)
+ err = create_address_port (port, addr, addr_len,
+ aport);
+ else
+ err = 0;
if (! err)
{
/* Send the data. */
@@ -72,12 +85,13 @@ __sendto (int fd,
NULL,
MACH_MSG_TYPE_COPY_SEND, 0,
NULL, 0, wrote);
- __mach_port_deallocate (__mach_task_self (),
- aport);
}
err;
}));
+ if (aport != MACH_PORT_NULL)
+__mach_port_deallocate (__mach_task_self (), aport);
+
return err ? __hurd_sockfail (fd, flags, err) : wrote;
}
signature.asc
Description: This is a digitally signed message part.
Re: [PATCH,HURD] sendto: do not crash when addr is NULL
That's almost right, but you should avoid calling __mach_port_deallocate if APORT is MACH_PORT_NULL. Also, avoid shadowing the local variable ERR with one of the same name in the subfunction. Thanks, Roland
Re: [PATCH,HURD] sendto: do not crash when addr is NULL
Hi,
Alle lunedì 21 maggio 2012, Roland McGrath ha scritto:
Put the name of a local variable in all caps when mentioning it in
the log entry or comments.
Noted, and done.
It looks like the existing code also has a leak of APORT in the case
where HURD_DPORT_USE fails (i.e. invalid fd). So you should
reorganize in some way that fixes that too.
Done.
It might be simplest just to move the creation of the address port
(both the AF_LOCAL case and the default case) into a subroutine and
just call that inside HURD_DPORT_USE. It means that the descriptor
structure and port are kept alive around the AF_LOCAL case's calls,
but that seems OK.
Done.
Updated patch attached.
--
Pino Toscano
Hurd: sendto: do not crash when ADDR is NULL
Create a new create_address_port subroutine to isolate the address port creation
(for both local and remove sockets), and use it inside HURD_DPORT_USE.
Also intialize APORT to MACH_PORT_NULL and make sure to always deallocate it.
2012-06-07 Pino Toscano toscano.p...@tiscali.it
* sysdeps/mach/hurd/sendto.c (create_address_port): New subroutine.
(__sendto): Use create_address_port. Initialize APORT and always
deallocate it.
--- a/sysdeps/mach/hurd/sendto.c
+++ b/sysdeps/mach/hurd/sendto.c
@@ -33,37 +33,50 @@ __sendto (int fd,
const struct sockaddr_un *addr,
socklen_t addr_len)
{
- addr_port_t aport;
+ addr_port_t aport = MACH_PORT_NULL;
error_t err;
size_t wrote;
- if (addr-sun_family == AF_LOCAL)
+ /* Get an address port for the desired destination address. */
+ error_t create_address_port (io_t port,
+ const struct sockaddr_un *addr,
+ socklen_t addr_len,
+ addr_port_t *aport)
{
- /* For the local domain, we must look up the name as a file and talk
- to it with the ifsock protocol. */
- file_t file = __file_name_lookup (addr-sun_path, 0, 0);
- if (file == MACH_PORT_NULL)
- return -1;
- err = __ifsock_getsockaddr (file, aport);
- __mach_port_deallocate (__mach_task_self (), file);
- if (err == MIG_BAD_ID || err == EOPNOTSUPP)
- /* The file did not grok the ifsock protocol. */
- err = ENOTSOCK;
- if (err)
- return __hurd_fail (err);
+ error_t err;
+
+ if (addr-sun_family == AF_LOCAL)
+ {
+ /* For the local domain, we must look up the name as a file and talk
+ to it with the ifsock protocol. */
+ file_t file = __file_name_lookup (addr-sun_path, 0, 0);
+ if (file == MACH_PORT_NULL)
+ return errno;
+ err = __ifsock_getsockaddr (file, aport);
+ __mach_port_deallocate (__mach_task_self (), file);
+ if (err == MIG_BAD_ID || err == EOPNOTSUPP)
+ /* The file did not grok the ifsock protocol. */
+ err = ENOTSOCK;
+ }
+ else
+ {
+ err = __socket_create_address (port,
+ addr-sun_family,
+ (char *) addr,
+ addr_len,
+ aport);
+ }
+
+ return err;
}
- else
-err = EIEIO;
- /* Get an address port for the desired destination address. */
err = HURD_DPORT_USE (fd,
({
- if (err)
- err = __socket_create_address (port,
- addr-sun_family,
- (char *) addr,
- addr_len,
- aport);
+ if (addr != NULL)
+ err = create_address_port (port, addr, addr_len,
+ aport);
+ else
+ err = 0;
if (! err)
{
/* Send the data. */
@@ -72,12 +85,12 @@ __sendto (int fd,
NULL,
MACH_MSG_TYPE_COPY_SEND, 0,
NULL, 0, wrote);
- __mach_port_deallocate (__mach_task_self (),
- aport);
}
err;
}));
+ __mach_port_deallocate (__mach_task_self (), aport);
+
return err ? __hurd_sockfail (fd, flags, err) : wrote;
}
signature.asc
Description: This is a digitally signed message part.
Re: [PATCH,HURD] sendto: do not crash when addr is NULL
Put the name of a local variable in all caps when mentioning it in the log entry or comments. Initializing ERR at the top is confusingly subtle. Instead, you make the code that actually cares look like: if (addr == NULL || addr-sun_family == AF_LOCAL) err = 0; else err = __socket_create_address (...); if (! err) ... It looks like the existing code also has a leak of APORT in the case where HURD_DPORT_USE fails (i.e. invalid fd). So you should reorganize in some way that fixes that too. It might be simplest just to move the creation of the address port (both the AF_LOCAL case and the default case) into a subroutine and just call that inside HURD_DPORT_USE. It means that the descriptor structure and port are kept alive around the AF_LOCAL case's calls, but that seems OK. Thanks, Roland
