Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#1407 closed defect (fixed)

isc.dns.Rdata should catch C++ exceptions

Reported by: jinmei Owned by: vorner
Priority: high Milestone: Sprint-20111206
Component: libdns++ Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket: none
Estimated Difficulty: 2 Add Hours to Ticket: 0
Total Hours: 3.75 Internal?: no

Description

In particular, Rdata_init() doesn't catch any exception while it can
have a bad input in nature, so the interface is quite fragile.

I'd like it to be fixed ASAP, so I'm pushing it to the next sprint
proposed queue.

Subtickets

Change History (13)

comment:1 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 2

comment:2 Changed 8 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20111206

comment:3 Changed 8 years ago by vorner

  • Owner changed from UnAssigned to vorner
  • Status changed from new to accepted

comment:4 follow-up: Changed 8 years ago by vorner

  • Owner changed from vorner to UnAssigned
  • Status changed from accepted to reviewing

Hello

I made it catch whatever I thought off and added the fallback to the _init function. I didn't add any catches to the other methods, they are far less likely to throw. Should they be added?

And, should this have a changelog entry?

Thank you

comment:5 Changed 8 years ago by jinmei

  • Owner changed from UnAssigned to jinmei
  • Status changed from reviewing to accepted

comment:6 in reply to: ↑ 4 ; follow-up: Changed 8 years ago by jinmei

Replying to vorner:

Hello

I made it catch whatever I thought off and added the fallback to the _init function. I didn't add any catches to the other methods, they are far less likely to throw. Should they be added?

Since the original task is quite small, I'd fix them using this
opportunity. I'd even fix some other minor bugs and make other
cleanups:

  • Rdata_toWire() seems to be a bit buggy: I suspect

PyBytes_FromStringAndSize() can return NULL, in which case a
disaster will happen.

  • we should be able to eliminate reinterpret_cast.
  • I'd add more complete documentation (more aligned with the original

C++ doxygen doc)

This wrapper was written at an earlier stage of development in a rush,
and was generally not very clean. By piggybacking cleanups with a
relatively bigger bug fixes we can incrementally improve the quality
of the code.

But on the other hand these may be an abuse of the ticket especially
because we gave a small point of estimation to this ticket. So I'd
leave the decision to you.

And, should this have a changelog entry?

I think so, because it's a bug in a public API.

The change in the branch basically looks okay. I have just two minor
comments:

  • To be consistent with most of other code of the source tree I'd place 'catch' at the same line as that for the closing brace of the previous clause:
        } catch (const isc::dns::rdata::InvalidRdataText& irdt) {
            PyErr_SetString(po_InvalidRdataText, irdt.what());
            return (-1);
        }
        ...
    
    but this may purely be a matter of taste.
  • the input for the CharStringTooLong? case in the test could be more simplified:
            self.assertRaises(CharStringTooLong, Rdata, RRType("TXT"),
                              RRClass("IN"), ' ' * 256)
    
    (unless you intentionally generated 's' this way for some particular reason)

comment:7 Changed 8 years ago by jinmei

  • Owner changed from jinmei to vorner
  • Status changed from accepted to reviewing

comment:8 in reply to: ↑ 6 ; follow-up: Changed 8 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

  • Rdata_toWire() seems to be a bit buggy: I suspect

PyBytes_FromStringAndSize() can return NULL, in which case a
disaster will happen.

As that would mean a python exception, with it already set, I just return NULL.

  • we should be able to eliminate reinterpret_cast.

Done. Actually, when I was doing this, I opened the wrong file by accident. When I noticed, it was already done, so I left it there. But if you think it is too much for the ticket, I could remove these changes.

  • I'd add more complete documentation (more aligned with the original

C++ doxygen doc)

Hmm, the docs don't seem even remotely related to the purpose of the task, and it doesn't seem as problematic as uncaught exceptions. So I'd leave them out.

For the exceptions, I added a wrapper function. Maybe it could be made generic and handle even the pointer casts for the self and be put into some kind of utils library?

I tried to make the function to it a template parameter, so I wouldn't need to write the internal and external function explicitly, but the compiler didn't seem to agree with me, so it is done explicitly. It could be done with a macro, but I know we don't like these.

I think so, because it's a bug in a public API.

So, something like:

[bug]		vorner
C++ exceptions in the isc.dns.Rdata wrapper are now converted
to python ones instead of just aborting the interpretter.
  • To be consistent with most of other code of the source tree I'd place 'catch' at the same line as that for the closing brace of the previous clause:
        } catch (const isc::dns::rdata::InvalidRdataText& irdt) {
            PyErr_SetString(po_InvalidRdataText, irdt.what());
            return (-1);
        }
        ...
    
    but this may purely be a matter of taste.

Hmm, it makes some sense. I was thinking about catch as a separate statement, but it is more like an else to if, so it belongs together. Done.

Thanks, I didn't know I can do this in python as well. It surprises me, if it doesn't have ++, but can multiply strings O:-).

Thank you

comment:9 in reply to: ↑ 8 Changed 8 years ago by jinmei

Replying to vorner:

  • we should be able to eliminate reinterpret_cast.

Done. Actually, when I was doing this, I opened the wrong file by accident. When I noticed, it was already done, so I left it there. But if you think it is too much for the ticket, I could remove these changes.

Do you mean the changes in rrset_python.cc? They are a trivial
cleanup, so I don't mind including them. For that matter, the rrset
wrapper seems to need some fixes regarding exceptions, too, but that
would certainly be beyond the scope.

I noticed just one additional style issue (not really related to this
ticket or the changes you made) and fixed it.

Other than that the branch seems okay and ready for merge.

Changelog looks okay, too.

comment:10 Changed 8 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:11 Changed 8 years ago by jinmei

  • Total Hours changed from 0 to 0.75

comment:12 Changed 8 years ago by vorner

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 0.75 to 3.75

Thanks, closing

comment:13 Changed 8 years ago by shane

  • Feature Depending on Ticket set to none
Note: See TracTickets for help on using tickets.