[tahoe-lafs-trac-stream] [Tahoe-LAFS] #1085: we shouldn't use "assert" to validate incoming data in introducer client

Tahoe-LAFS trac at tahoe-lafs.org
Fri Jan 17 14:30:29 UTC 2020


#1085: we shouldn't use "assert" to validate incoming data in introducer client
------------------------------+------------------------
     Reporter:  zooko         |      Owner:
         Type:  defect        |     Status:  closed
     Priority:  minor         |  Milestone:  soon
    Component:  code-network  |    Version:  1.7β
   Resolution:  duplicate     |   Keywords:  introducer
Launchpad Bug:                |
------------------------------+------------------------
Changes (by exarkun):

 * status:  new => closed
 * resolution:   => duplicate


Old description:

> http://tahoe-lafs.org/trac/tahoe-
> lafs/browser/trunk/src/allmydata/introducer/client.py?rev=8df15e9f30a3bda7#L163
>
> Says:
>
> {{{
>
>     def _process_announcement(self, ann):
>         self._debug_counts["inbound_announcement"] += 1
>         (furl, service_name, ri_name, nickname_utf8, ver, oldest) = ann
>         if service_name not in self._subscribed_service_names:
>             self.log("announcement for a service we don't care about
> [%s]"
>                      % (service_name,), level=log.UNUSUAL, umid="dIpGNA")
>             self._debug_counts["wrong_service"] += 1
>             return
>         self.log("announcement for [%s]: %s" % (service_name, ann),
>                  umid="BoKEag")
>         assert type(furl) is str
>         assert type(service_name) is str
>         assert type(ri_name) is str
>         assert type(nickname_utf8) is str
>         nickname = nickname_utf8.decode("utf-8")
>         assert type(nickname) is unicode
>         assert type(ver) is str
>         assert type(oldest) is str
> }}}
> This means that validation of this incoming data is turned off by the
> PYTHONOPTIMIZE setting, and it means that introducers have the power to
> cause !AssertionFailure to be raised from this function. Now, in practice
> causing !AssertionFailure to be raised from this function won't hurt
> anything in the current version, but this is still not the right way to
> do it. We would like for failures of any of these validations to result
> in an exception that will get logged as explicitly being "we received an
> ill-formed announcement" rather than "!AssertionError", which is supposed
> to mean "there was an internal error in our source code".
>
> (Note that the introduction server may well just be passing this
> announcement through as it was given to the introduction server by
> someone else, and may not be responsible for the ill-formedness
> itself...)
>
> Relatedly, this code shouldn't catch all possible kinds of exceptions:
>
> http://tahoe-lafs.org/trac/tahoe-
> lafs/browser/trunk/src/allmydata/introducer/client.py?rev=8df15e9f30a3bda7#L149
> {{{
>             try:
>                 self._process_announcement(ann)
>             except:
>                 log.err(format="unable to process announcement %(ann)s",
>                         ann=ann)
> }}}
>
> But should instead catch the specific kind of exception that means "we
> received an ill-formed announcement". Any other kind of exception,
> including !AssertionFailure, should not be caught here.

New description:

 http://tahoe-lafs.org/trac/tahoe-
 lafs/browser/trunk/src/allmydata/introducer/client.py?rev=8df15e9f30a3bda7#L163

 Says:

 {{{

     def _process_announcement(self, ann):
         self._debug_counts["inbound_announcement"] += 1
         (furl, service_name, ri_name, nickname_utf8, ver, oldest) = ann
         if service_name not in self._subscribed_service_names:
             self.log("announcement for a service we don't care about [%s]"
                      % (service_name,), level=log.UNUSUAL, umid="dIpGNA")
             self._debug_counts["wrong_service"] += 1
             return
         self.log("announcement for [%s]: %s" % (service_name, ann),
                  umid="BoKEag")
         assert type(furl) is str
         assert type(service_name) is str
         assert type(ri_name) is str
         assert type(nickname_utf8) is str
         nickname = nickname_utf8.decode("utf-8")
         assert type(nickname) is unicode
         assert type(ver) is str
         assert type(oldest) is str
 }}}
 This means that validation of this incoming data is turned off by the
 PYTHONOPTIMIZE setting, and it means that introducers have the power to
 cause !AssertionFailure to be raised from this function. Now, in practice
 causing !AssertionFailure to be raised from this function won't hurt
 anything in the current version, but this is still not the right way to do
 it. We would like for failures of any of these validations to result in an
 exception that will get logged as explicitly being "we received an ill-
 formed announcement" rather than "!AssertionError", which is supposed to
 mean "there was an internal error in our source code".

 (Note that the introduction server may well just be passing this
 announcement through as it was given to the introduction server by someone
 else, and may not be responsible for the ill-formedness itself...)

 Relatedly, this code shouldn't catch all possible kinds of exceptions:

 http://tahoe-lafs.org/trac/tahoe-
 lafs/browser/trunk/src/allmydata/introducer/client.py?rev=8df15e9f30a3bda7#L149
 {{{
             try:
                 self._process_announcement(ann)
             except:
                 log.err(format="unable to process announcement %(ann)s",
                         ann=ann)
 }}}

 But should instead catch the specific kind of exception that means "we
 received an ill-formed announcement". Any other kind of exception,
 including !AssertionFailure, should not be caught here.

--

Comment:

 Essentially a duplicate of ticket:1944 and ticket:1968

--
Ticket URL: <https://tahoe-lafs.org/trac/tahoe-lafs/ticket/1085#comment:2>
Tahoe-LAFS <https://Tahoe-LAFS.org>
secure decentralized storage


More information about the tahoe-lafs-trac-stream mailing list