240036 - throwing invalid address causes internal error when tables locked

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, defect, P3)

Description

[Jon]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4) Gecko/20030630 Galeon/1.3.8
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4) Gecko/20030630 Galeon/1.3.8

if you specify an invalid address to the watch list it spews out an internal
error mssage


Reproducible: Always
Steps to Reproduce:

Comment 1

[Jon]

Attachment: internal error msg

This is the error message that was generated.

Comment 2

[Jon]

Attachment: internal error msg

sorry, saved the wrong file the first time.

Comment 3

[Joel Peshkin]

yep

Comment 4

[Joel Peshkin]

Attachment: Patch

This was caused by calling DBNameToIDAndCheck() with tables locked.  When
DBNameToIDAndCheck throws an error, it fails to specify that tables should be
unlocked and the footer then dies.

Comment 5

[Christian Reis]

Yeah, sounds about right. I'm wondering

  - how this didn't bite us before
  - which callsite is exercising this callsite

Looking. Hmmm, wow. This comes all the way through 

  - the RelationSet constructor being called with a list of invalid IDs
  - mergeFromString calling DBNameToIdAndCheck with an invalid ID
  - DBNameToIdAndCheck throwing a user error

as to why this hasn't bit us before, I'm not entirely sure, given that at least
DBNameToIdAndCheck is called in a number of places. Perhaps there's validation
done before calling it, though.

Comment 6

[Christian Reis]

Comment on attachment 145742 [details] [diff] [review]
Patch

Tested here and works as expected.

My only question is whether unlocking tables can have any adverse effect, given
that this function is used in a large number of callsites.

Comment 7

[Joel Peshkin]

Since throwerror never returns to anything that writes to the DB, unlocking
should be fine.  Whatever operation originally wanted the DB locked would have
been abandoned at this point.

Comment 8

[Dave Miller [:justdave]]

Comment on attachment 145742 [details] [diff] [review]
Patch

don't most other callsites use something that makes the meaning obvious like
"abort" or "unlock" in place of the 1 in the second param?

Comment 9

[Dave Miller [:justdave]]

*** Bug 240366 has been marked as a duplicate of this bug. ***

Comment 10

[Dave Miller [:justdave]]

Comment on attachment 145742 [details] [diff] [review]
Patch

review- per comment 8.	Just did a grep of the directory, and indeed, all the
other call sites use the string "abort" for the third param instead of a 1.

Comment 11

[Joel Peshkin]

Attachment: v2

Comment 12

[Joel Peshkin]

checked in

Comment 13

[Dave Miller [:justdave]]

*** Bug 233124 has been marked as a duplicate of this bug. ***

Comment 14

[Marc Schumann [:Wurblzap]]

A similar effect occurs if you enter non-numerical values for estimated hours.
So while you're at it, maybe you could change line 131 as well in a similar 
manner like

-ThrowUserError("need_numeric_value");
+ThrowUserError("need_numeric_value", "abort");

?

Comment 15

[Christian Reis]

Marc, you should open a new bug for that.

Comment 16

[Marc Schumann [:Wurblzap]]

Ok, we now have bug 246778 to address similar instances of this bug.

Comment 17

[Christian Reis]

*** Bug 217344 has been marked as a duplicate of this bug. ***