Register forum user name Search FAQ

Gammon Forum

Notice: Any messages purporting to come from this site telling you that your password has expired, or that you need to verify your details, confirm your email, resolve issues, making threats, or asking for money, are spam. We do not email users with any such messages. If you have lost your password you can obtain a new one by using the password reset link.

Due to spam on this forum, all posts now need moderator approval.

 Entire forum ➜ SMAUG ➜ Compiling the server ➜ Suggested improvement to SmaugFUSS

Suggested improvement to SmaugFUSS

It is now over 60 days since the last post. This thread is closed.     Refresh page


Pages: 1 2  3  

Posted by Nick Gammon   Australia  (23,133 posts)  Bio   Forum Administrator
Date Thu 29 Jun 2006 10:37 PM (UTC)
Message
Samson, in case you are reading this, can I suggest a fix to FUSS?

I am playing with writing a few snippets, and noticed this in fight.c:


void new_dam_message( CHAR_DATA * ch, CHAR_DATA * victim, int dam, unsigned int dt, OBJ_DATA * obj )
{
   char buf1[256], buf2[256], buf3[256];

... (and further on) ...

      snprintf( buf1, 256, "$n %s $N%c", vp, punct );
      snprintf( buf2, 256, "You %s $N%c", vs, punct );
      snprintf( buf3, 256, "$n %s you%c", vp, punct );


Wouldn't it be much safer to do this:


      snprintf( buf1, sizeof buf1, "$n %s $N%c", vp, punct );
      snprintf( buf2, sizeof buf2, "You %s $N%c", vs, punct );
      snprintf( buf3, sizeof buf3, "$n %s you%c", vp, punct );


This is safer, in case you ever change the sizes of buf1 etc., but removes the "magic number" of 256, and replaces it with a proper explanation for what that paremeter really is.

There are similar instances further down the function.

- Nick Gammon

www.gammon.com.au, www.mushclient.com
Top

Posted by Metsuro   USA  (389 posts)  Bio
Date Reply #1 on Thu 29 Jun 2006 11:14 PM (UTC)
Message
That happens in many places in smaugfuss, where the place the size several places even if its all the same.

Everything turns around in the end
Top

Posted by David Haley   USA  (3,881 posts)  Bio
Date Reply #2 on Fri 30 Jun 2006 02:14 AM (UTC)
Message
Isn't SmaugFUSS compiled using C++? In that case the ideal would be to just use an ostringstream; it'd be the safest of all.

David Haley aka Ksilyan
Head Programmer,
Legends of the Darkstone

http://david.the-haleys.org
Top

Posted by Nick Gammon   Australia  (23,133 posts)  Bio   Forum Administrator
Date Reply #3 on Fri 30 Jun 2006 03:24 AM (UTC)
Message
No, it's C. I've just been making changes.

- Nick Gammon

www.gammon.com.au, www.mushclient.com
Top

Posted by Samson   USA  (683 posts)  Bio
Date Reply #4 on Fri 30 Jun 2006 02:06 PM (UTC)
Message
So is that supposed to be "sizeof buf1" and not "sizeof( buf1 )"?

And would this only be necessary for places like the above, or would it be more or less everywhere? I don't mind having this done but it wouldn't happen quickly with the limited time I have available these days - unless someone were to provide a patch against the latest version :)
Top

Posted by David Haley   USA  (3,881 posts)  Bio
Date Reply #5 on Fri 30 Jun 2006 02:46 PM (UTC)
Message
I don't think you need the parens, even though most people put them in.

And you'd use it every place you used the hard-coded, numerical size of a buffer. The idea is to pull the size directly from the compiler's information, which can't really be wrong, instead of depending on the programmer to update all places the size is referenced if the buffer size is changed.

David Haley aka Ksilyan
Head Programmer,
Legends of the Darkstone

http://david.the-haleys.org
Top

Posted by Nick Gammon   Australia  (23,133 posts)  Bio   Forum Administrator
Date Reply #6 on Fri 30 Jun 2006 10:37 PM (UTC)
Message
This is crying out for an automated fixup with regular expressions, which I am almost ready to release. However this caught my eye while I was doing it:

imc.c line 6379:


   snprintf( buf + strlen( buf ), MAX_STRING_LENGTH,
             ... blah blah ...


In most other places in the code where this technique is used, it has read:



   snprintf( buf + strlen( buf ), MAX_STRING_LENGTH - strlen ( buf ),


It probably isn't a big deal, but that line should probably be amended to have the "- strlen" bit added to it.


- Nick Gammon

www.gammon.com.au, www.mushclient.com
Top

Posted by Nick Gammon   Australia  (23,133 posts)  Bio   Forum Administrator
Date Reply #7 on Fri 30 Jun 2006 11:00 PM (UTC)

Amended on Sat 01 Jul 2006 03:30 AM (UTC) by Nick Gammon

Message
Samson, I'm assuming you have access to (or use) MUSHclient? I wrote this script using the Immediate window in MUSHclient, which will fixup the entire FUSS source directory - writing to a new directory for safety. You can do a diff on the two directories to check the changes look reasonable, before copying the changed files back over the other ones.

This is the script:


-- file paths, amend as required
olddir = "C:\\smaugfuss\\src\\"
newdir = "C:\\smaugfuss\\src_fixed\\"

-->> see below for improvements to this bit
standard_replace = "snprintf( %1, sizeof (%1),"

replacements = {
    { find = "snprintf%s*%(%s*([A-Za-z0-9_]+)%s*,%s*[0-9]+%s*,",
      repl = standard_replace
    },

    { find = "snprintf%s*%(%s*([A-Za-z0-9_]+)%s*,%s*MAX_[A-Z]+_LENGTH%s*,",
      repl = standard_replace
    },

    { find = "snprintf%s*%(%s*([A-Za-z0-9_]+)%s*,%s*SMST%s*,",
      repl = standard_replace
    },

    { find = "snprintf%s*%(%s*([A-Za-z0-9_]+)%s*,%s*LGST%s*,",
      repl = standard_replace
    },

    { find = "snprintf%s*%(%s*([A-Za-z0-9_]+)%s*,%s*IMC_BUFF_SIZE%s*,",
      repl = standard_replace
    },
    
  }  -- end replacements table

-->> end of part that has been improved (copy from below)

function process_file (name)
  local f, s
  local total = 0

  print ("Processing: " .. name)
  f = assert (io.open (olddir .. name, "rb"))  -- open input
  s = f:read ("*a")  -- read all of it
  f:close ()  -- close it

  -- apply a series of changes
  for k, v in ipairs (replacements) do
     local count
     s, count = string.gsub (s, v.find, v.repl)
     total = total + count
  end -- for each replacement

  if total > 0 then
    print (" -- " .. total .. " changes made.")
    f = assert (io.open (newdir .. name, "wb")) -- open output
    f:write (s) -- write to it
    f:close ()  -- close that file now
  end -- changes made
 
  return total
end -- process_file 

-- START HERE --

local grand_total = 0

local t = assert (utils.readdir (olddir .. "*.c"))
local k

for k in pairs (t) do
  grand_total = grand_total + process_file (k)
end -- for

print ("** Total of " .. grand_total .. " changes made.")



After running this (you will need to change the directory locations to match your setup) I got this:


Processing: save.c
-- 11 changes made.
Processing: act_obj.c
-- 13 changes made.
Processing: hashstr.c
-- 3 changes made.
Processing: shops.c
-- 24 changes made.
Processing: comm.c
-- 21 changes made.
Processing: mud_prog.c
-- 6 changes made.
Processing: planes.c
Processing: skills.c
-- 29 changes made.
Processing: fight.c
-- 22 changes made.
Processing: act_wiz.c
-- 80 changes made.
Processing: handler.c
-- 7 changes made.
Processing: const.c
Processing: mud_comm.c
-- 16 changes made.
Processing: act_comm.c
-- 31 changes made.
Processing: imm_host.c
Processing: services.c
-- 2 changes made.
Processing: polymorph.c
-- 5 changes made.
Processing: sha256.c
Processing: boards.c
-- 22 changes made.
Processing: act_info.c
-- 33 changes made.
Processing: interp.c
-- 10 changes made.
Processing: mapout.c
-- 1 changes made.
Processing: imc.c
-- 91 changes made.
Processing: build.c
-- 57 changes made.
Processing: mpxset.c
-- 26 changes made.
Processing: magic.c
-- 16 changes made.
Processing: hotboot.c
-- 10 changes made.
Processing: deity.c
-- 12 changes made.
Processing: update.c
-- 16 changes made.
Processing: act_move.c
-- 10 changes made.
Processing: special.c
-- 7 changes made.
Processing: color.c
-- 57 changes made.
Processing: misc.c
-- 3 changes made.
Processing: makeobjs.c
-- 7 changes made.
Processing: reset.c
-- 8 changes made.
Processing: comments.c
Processing: mccp.c
Processing: player.c
-- 163 changes made.
Processing: db.c
-- 25 changes made.
Processing: track.c
-- 8 changes made.
Processing: clans.c
-- 13 changes made.
Processing: tables.c
-- 7 changes made.
Processing: ban.c
-- 4 changes made.
** Total of 876 changes made.


This is a partial diff of what it did:


diff src/act_comm.c src_fixed/act_comm.c
404c404
< snprintf( buf, MAX_STRING_LENGTH, "%s what?\r\n", verb );
---
> snprintf( buf, sizeof (buf), "%s what?\r\n", verb );
423c423
< snprintf( buf, MAX_STRING_LENGTH, "$n %ss '$t'", verb );
---
> snprintf( buf, sizeof (buf), "$n %ss '$t'", verb );
428c428
< snprintf( buf, MAX_STRING_LENGTH, "$n %ss '$t'", verb );
---
> snprintf( buf, sizeof (buf), "$n %ss '$t'", verb );



I can just send you the diff if you prefer, but this gives you the tool to make similar changes yourself.

What the script does is use MUSHclient's Lua extension utils.readdir to get a list of all the .c files in the directory, and then opens each one and applies a series of string.gsub to it. The main one is to look for the case:


snprintf (buf, (some number)


... and replace "(some number)" by sizeof "(buf)".

However there are a few special cases that seem to occur frequently, like MAX_STRING_LENGTH, SMST, and LGST, so I have added those as extras.

There are still a handful of cases this script doesn't handle (52 of them), along the lines of:


snprintf( buf + strlen( buf ), IMC_BUFF_SIZE - strlen( buf ),


You can find those with:


grep -n snprintf *.c | grep -v sizeof


... and then fix them manually.

- Nick Gammon

www.gammon.com.au, www.mushclient.com
Top

Posted by Nick Gammon   Australia  (23,133 posts)  Bio   Forum Administrator
Date Reply #8 on Fri 30 Jun 2006 11:43 PM (UTC)

Amended on Sat 01 Jul 2006 12:03 AM (UTC) by Nick Gammon

Message
With a slightly more complex regexp you can handle the cases of the buf + strlen (buf). This part replaces the section above which defines the regular expressions:



snprintf1 = "snprintf%s*%(%s*([%a%d_]+)%s*,%s*"
snprintf2 = "snprintf%s*%(%s*([%a%d_]+)%s*%+%s*strlen%s*%(%s*%1%s*%)%s*,%s*"

standard_replace1 = "snprintf( %1, sizeof ( %1 ),"
standard_replace2 = "snprintf( %1 + strlen( %1 ), sizeof ( %1 ) - strlen( %1 ),"

replacements = {
    { find = snprintf1 .. "%d+%s*,",
      repl = standard_replace1
    },

    { find = snprintf1 .. "MAX_%u+_LENGTH%s*,",
      repl = standard_replace1
    },

    { find = snprintf1 .. "SMST%s*,",
      repl = standard_replace1
    },

    { find = snprintf1 .. "LGST%s*,",
      repl = standard_replace1
    },

    { find = snprintf1 .. "IMC_BUFF_SIZE%s*,",
      repl = standard_replace1
    },

    { find = snprintf2 .. "IMC_BUFF_SIZE%s*%-%s*strlen%s*%(%s*%1%s*%)%s*,",
      repl = standard_replace2
    },

    { find = snprintf2 .. "%(?%s*MAX_%u+_LENGTH%s*%-%s*strlen%s*%(%s*%1%s*%)%s*%)?%s*,",
      repl = standard_replace2
    },

    { find = snprintf2 .. "LGST%s*%-%s*strlen%s*%(%s*%1%s*%)%s*,",
      repl = standard_replace2
    },
    
  }  -- end replacements table



The extra parts (that use standard_replace2) now check for "+ strlen (%1)" which is the case of taking the strlen of whatever we are doing the snprintf to, and using that for a slightly different sort of replacement.

After running that on the entire source, there are only 19 special cases that need manually fixing, for example:


vsnprintf( buf, MAX_STRING_LENGTH * 2, fmt, args );


- Nick Gammon

www.gammon.com.au, www.mushclient.com
Top

Posted by Samson   USA  (683 posts)  Bio
Date Reply #9 on Mon 03 Jul 2006 03:38 AM (UTC)

Amended on Mon 03 Jul 2006 03:47 AM (UTC) by Samson

Message
So all I'd need to do is have this script run inside of mushclient and it will patch up the smaug source to change all of the buffer usage in the code? And how will I be able to spot the remaining special cases?

Also going to need to know what the "Immediate window" is in Mushclient since I can't find a menu option called anything like that.
Top

Posted by Davion   Canada  (10 posts)  Bio
Date Reply #10 on Mon 03 Jul 2006 04:51 AM (UTC)
Message
So is that supposed to be "sizeof buf1" and not "sizeof( buf1 )"?

So many people think this, but iirc, the only time the sizeof operator requires ()'s is when your using a type instead of an actual variable.
Top

Posted by David Haley   USA  (3,881 posts)  Bio
Date Reply #11 on Mon 03 Jul 2006 05:36 AM (UTC)
Message
#include <stdio.h>

int main()
{
    char buf[256];

    printf("%d %d\n", sizeof int, sizeof(int));
    printf("%d %d\n", sizeof buf, sizeof(buf));

    return 0;
}


This program fails to compile, with a parse error at the first printf. But it works with parens around the int for the sizeof; so it appears that parens are indeed only necessary for a type. I didn't test for structures etc., though.

David Haley aka Ksilyan
Head Programmer,
Legends of the Darkstone

http://david.the-haleys.org
Top

Posted by Nick Gammon   Australia  (23,133 posts)  Bio   Forum Administrator
Date Reply #12 on Mon 03 Jul 2006 06:57 AM (UTC)

Amended on Mon 03 Jul 2006 06:59 AM (UTC) by Nick Gammon

Message
Quote:

So is that supposed to be "sizeof buf1" and not "sizeof( buf1 )"?


My initial suggestion omitted the brackets, however because of the confusion I put them into the script. Whichever you prefer. I don't think they do any harm.

Quote:

So all I'd need to do is have this script run inside of mushclient and it will patch up the smaug source to change all of the buffer usage in the code?


Yes, although since MUSHclient is a Windows program, unless you are using Cygwin, you would need to copy the source to a suitable directory, put its path into the script, and run it. Make a blank directory for the src_fixed (output files) so you can run a diff on the before and after.


Quote:

And how will I be able to spot the remaining special cases?


As I mentioned, this command in Linux or Cygwin will do it:


grep -n snprintf *.c | grep -v sizeof


That will give you the files and line numbers, namely:


act_wiz.c:7675: snprintf( race_table[rcindex]->race_name, 16, "%-.16s", argument );
act_wiz.c:7807: snprintf( race->race_name, 16, "%s", capitalize( argument ) );
color.c:1542: vsnprintf( buf, MAX_STRING_LENGTH * 2, fmt, args );
color.c:1554: vsnprintf( buf, MAX_STRING_LENGTH * 2, fmt, args );
color.c:1570: vsnprintf( buf, MAX_STRING_LENGTH * 2, fmt, args );
color.c:1582: vsnprintf( buf, MAX_STRING_LENGTH * 2, fmt, args );
color.c:1594: vsnprintf( buf, MAX_STRING_LENGTH * 2, fmt, args );
db.c:4182: vsnprintf( buf, MAX_STRING_LENGTH * 2, fmt, args );
db.c:4194: vsnprintf( buf, MAX_STRING_LENGTH * 2, fmt, args );
imc.c:427: snprintf( buf, LGST * 2, "%s\033[0m", color_itom( txt, ch ) );
imc.c:462: snprintf( buf, LGST * 2, "%s\033[0m", color_itom( txt, ch ) );
imc.c:1541: snprintf( data->field, IMC_BUFF_SIZE, " %s", escape_string( pkt ) );
imc.c:1569: snprintf( p->from, SMST, "%s@%s", from, this_imcmud->localname );
imc.c:6379: snprintf( buf + strlen( buf ), MAX_STRING_LENGTH,
interp.c:383: snprintf( lastplayercmd, ( MAX_INPUT_LENGTH * 2 ), "%s used %s", ch->name, logline );
misc.c:2002: snprintf( p, ( XBI * 12 ) - ( p - buf ), "%d", bits->bits[x] );
skills.c:759: snprintf( buf + 3, MAX_STRING_LENGTH - 3, ") lvl: %3d max: %2d%%", skill->skill_level[iClass],
tables.c:383: snprintf( race_table[i]->race_name, 16, "%s", "unused" );
tables.c:626: snprintf( race->race_name, 16, "%-.16s", race_name );


(This was with the amended regexps in the later post).

The double grep is a powerfull tool. The first one finds the snprintf lines, the second one omits (-v) the ones which already have sizeof in them.

Quote:

Also going to need to know what the "Immediate window" is in Mushclient since I can't find a menu option called anything like that.


Game Menu -> Immediate (Ctrl+I).

- Nick Gammon

www.gammon.com.au, www.mushclient.com
Top

Posted by Samson   USA  (683 posts)  Bio
Date Reply #13 on Mon 03 Jul 2006 02:49 PM (UTC)
Message
Error number: 0
Event: Run-time error
Description: [string "Immediate"]:51: attempt to index global `io' (a nil value)
stack traceback:
[string "Immediate"]:51: in function `process_file'
[string "Immediate"]:80: in main chunk
Called by: Immediate execution
Top

Posted by David Haley   USA  (3,881 posts)  Bio
Date Reply #14 on Mon 03 Jul 2006 03:10 PM (UTC)
Message
I think the default sandboxing disables a lot of the system libraries. I think the sandboxing takes place in the world properties, scripting section.

David Haley aka Ksilyan
Head Programmer,
Legends of the Darkstone

http://david.the-haleys.org
Top

The dates and times for posts above are shown in Universal Co-ordinated Time (UTC).

To show them in your local time you can join the forum, and then set the 'time correction' field in your profile to the number of hours difference between your location and UTC time.


81,030 views.

This is page 1, subject is 3 pages long: 1 2  3  [Next page]

It is now over 60 days since the last post. This thread is closed.     Refresh page

Go to topic:           Search the forum


[Go to top] top

Information and images on this site are licensed under the Creative Commons Attribution 3.0 Australia License unless stated otherwise.