bugFreeciv - Bugs: bug #19175, lookup_*() in ruleset.c look...

 
 
Show feedback again

bug #19175: lookup_*() in ruleset.c look dubious in some error cases

Submitted by:  Jacob Nevins <jtn>
Submitted on:  Mon 12 Dec 2011 01:13:00 AM UTC  
 
Category: generalSeverity: 2 - Minor
Priority: 5 - NormalStatus: None
Assigned to: NoneOpen/Closed: Open
Release: S2_4 r20647Operating System: Any
Planned Release: 

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

Please log in, so followups can be emailed to you.

 

Mon 18 Mar 2013 10:47:27 PM UTC, comment #1:

patch #3785 changes this (as side effect) again for TRUNK. Given how you present some points arguable, that may lead to even more buggy behavior.

Marko Lindqvist <cazfi>
Project Administrator
Mon 12 Dec 2011 01:13:00 AM UTC, original submission:

lookup_tech() and friends return an advance (etc) from a secfile entry.

In S1_14 we had:

with the following possibilities:

  • Entry not in secfile: fatal (secfile_lookup_str() kills server)
  • Entry exists in secfile:
    • It's the string "Never":
      • required: fatal (find_tech_by_name() won't find it)
      • !required: return A_LAST
    • It's an unmatchable string (garbage):
      • required: fatal
      • !required: return A_LAST (and moan at LOG_ERROR)
    • It's a matchable string (success case):
      • return its id

Advances became "struct advance *"s rather than ints by S2_0. In the process, I think the logic got broken. Today we have:

(where the old "required" is logically loglevel>LOG_FATAL (i.e., LOG_ERROR etc)

  • Entry not in secfile: return A_NEVER (==NULL)
  • Entry exists in secfile:
    • It's the string "Never":
      • required: fatal (advance_by_rule_name() won't find it, ruleset_error() dies)
      • !required: return A_NEVER
    • It's an unmatchable string (garbage):
      • required: fatal
      • !required: return A_NEVER (and moan at loglevel)
    • It's a matchable string (success case):
      • return its id

So, the old version could never return A_LAST if "required" was set, but the new version can return A_NEVER even in this case. I suspect callers aren't prepared for that. Indeed, commenting out tech_req for a unit causes a segfault.

It would seem more useful for the "required" behaviour to be never to return A_NEVER. The details are arguable, however.

Jacob Nevins <jtn>
Project Administrator

 

(Note: upload size limit is set to 1024 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by cazfi (Posted a comment)
  • -unavailable- added by jtn (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    No Changes Have Been Made to This Item
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup