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 Samson   USA  (683 posts)  Bio
Date Reply #15 on Mon 03 Jul 2006 03:40 PM (UTC)
Message
The only thing relating to scripting I see in the world properties that seems to be relevant is the "enable" checkbox. Which I have enabled. I don't see anything about sandboxing or enabling system libraries.
Top

Posted by David Haley   USA  (3,881 posts)  Bio
Date Reply #16 on Mon 03 Jul 2006 05:11 PM (UTC)
Message
It might be in global preferences, then. But I know there's an input field for sandboxing somewhere. Sorry to not be more specific, I'm at work at don't have the client in front of me.

David Haley aka Ksilyan
Head Programmer,
Legends of the Darkstone

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

Posted by Samson   USA  (683 posts)  Bio
Date Reply #17 on Mon 03 Jul 2006 05:30 PM (UTC)
Message
Aha, there it is. Thanks. Script works nicely. I'll run it again once I'm done catching up on other fixes :)
Top

Posted by Nick Gammon   Australia  (23,158 posts)  Bio   Forum Administrator
Date Reply #18 on Tue 04 Jul 2006 11:27 PM (UTC)
Message
Quote:

attempt to index global `io' (a nil value)


The "io" library is sandboxed out (suppressed) by default for the Lua scripting engine, as doing thing like io.open of an output file could be used to inject viruses or trojan horses into a target system. The intention is that you allow the io library for trusted plugins, or your own code.

- Nick Gammon

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

Posted by Gatewaysysop2   USA  (146 posts)  Bio
Date Reply #19 on Thu 06 Jul 2006 03:22 AM (UTC)
Message
Nick,

Fist off, thanks for making this available. As Samson reported, it works beautifully!

I suppose a natural question at this point would be about the mudstrlcpy and mudstrlcat calls, which are only slightly different in how they are set up, but could be made safer in the same fashion.

Is it difficult to expand this script to encompass those cases as well? I was tempted to go in and correct them by hand to use sizeof() but when I grepped to jog my memory of just how many we were talking about, I figured it might be worthwhile to ask about changing the script first. ;)

Again, thanks for putting this together and making it available to the community! :-)


"The world of men is dreaming, it has gone mad in its sleep, and a snake is strangling it, but it can't wake up." -D.H. Lawrence
Top

Posted by Samson   USA  (683 posts)  Bio
Date Reply #20 on Thu 06 Jul 2006 04:01 AM (UTC)
Message
That would assume this can be done on strcpy/strcat and related type functions. I got the feeling it's only really useful on sprintf type functions. But hell, what do I know about strings, I could be wrong :)
Top

Posted by Gatewaysysop2   USA  (146 posts)  Bio
Date Reply #21 on Sat 08 Jul 2006 12:45 AM (UTC)
Message
Hmm.

Noticing now that something may have been improperly replaced with this. I found that the prompt was not displaying properly and there were other issues in unrelated areas as well. Has anyone else had similar issues? I'm seeing if I can track down the problem, but so far I've not figured out exactly what is causing it. I figure it might be one bad replacement somewhere, but I'm not sure yet.


"The world of men is dreaming, it has gone mad in its sleep, and a snake is strangling it, but it can't wake up." -D.H. Lawrence
Top

Posted by Nick Gammon   Australia  (23,158 posts)  Bio   Forum Administrator
Date Reply #22 on Sat 08 Jul 2006 01:22 AM (UTC)
Message
I haven't noticed any problems with standard SmaugFUSS. The prompt looks OK. Can you be more specific about "other issues?". Did you just do the initial changes I recommended or make some manual ones as well, or maybe the mudstrlcpy calls you talked about?

- Nick Gammon

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

Posted by Nick Gammon   Australia  (23,158 posts)  Bio   Forum Administrator
Date Reply #23 on Sat 08 Jul 2006 01:29 AM (UTC)
Message
I can however see a serious problem. Take this test program:


#include <stdio.h>

int main ()
  {
  char buf [1000];
  char * buf2 = (char *) malloc (1000);
  printf ("buf is %i bytes.\n", sizeof (buf));
  printf ("buf2 is %i bytes.\n", sizeof (buf2));
  return 0;

 }


Both buf and buf2 represent 1000 bytes of memory. However look at the output:


buf is 1000 bytes.
buf2 is 4 bytes.


It has correctly reported that buf2 (the pointer) is 4 bytes long, however from the point of view of using it in a printf or similar it is really (pointing to) 1000 bytes.

I can't see a simple solution to this, but to manually review each of the changes to make sure that the change is for a locally-declared buffer and not a pointer passed down to the function.

- Nick Gammon

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

Posted by Nick Gammon   Australia  (23,158 posts)  Bio   Forum Administrator
Date Reply #24 on Sat 08 Jul 2006 01:31 AM (UTC)
Message
A lint-style program may well pick up that misuse of sizeof in this context, not sure where you can get a free one that would detect this case.

- Nick Gammon

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

Posted by Nick Gammon   Australia  (23,158 posts)  Bio   Forum Administrator
Date Reply #25 on Sat 08 Jul 2006 04:14 AM (UTC)
Message
I found Splint at http://www.splint.org/ which seems to do a reasonable job in general, however it does not detect this particular problem.

I can't offhand think of a way of automating this. However I think it will "fail safe" - that is, instead of taking the correct buffer size will take 4 (the size of a pointer) so it won't probably lead to a crash, and if you happen to see a display where there are only 4 characters, you have probably stumbled across a case in point.

- Nick Gammon

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

Posted by Gatewaysysop2   USA  (146 posts)  Bio
Date Reply #26 on Sun 09 Jul 2006 02:05 AM (UTC)
Message
Actually that makes quite a bit of sense and you're probably exactly right about what is happening. Thanks for the insight. :)

"The world of men is dreaming, it has gone mad in its sleep, and a snake is strangling it, but it can't wake up." -D.H. Lawrence
Top

Posted by Samson   USA  (683 posts)  Bio
Date Reply #27 on Sun 09 Jul 2006 03:01 AM (UTC)
Message
So if this hasn't been done yet would it be advisable to hold off and wait? The script makes a great many changes and I'd really rather not have to manually verify them all :/
Top

Posted by Nick Gammon   Australia  (23,158 posts)  Bio   Forum Administrator
Date Reply #28 on Sun 09 Jul 2006 03:47 AM (UTC)
Message
Much as I hate to say it, I think that would be wise. It is hard to be certain the changes will always be for the better, bearing in mind the possible problem with pointers rather than fixed buffers. I notice while researching this, this rather strange sequence, in act_comm.c starting at line 469:


         char lbuf[MAX_INPUT_LENGTH + 4]; /* invis level string + buf */

... and later ...

            snprintf( lbuf, MAX_STRING_LENGTH, "(%d) ", ( !IS_NPC( ch ) ) ? ch->pcdata->wizinvis : ch->mobinvis );



Now bearing in mind these defines:


#define MAX_STRING_LENGTH 4096 /* buf */
#define MAX_INPUT_LENGTH 1024 /* arg */


This code (before my changes) just seems plain wrong. The snprintf is to a buffer of 4096 bytes (MAX_STRING_LENGTH) but the actual memory available is 1028 bytes (MAX_INPUT_LENGTH + 4).

After my automated change you get:


            snprintf( lbuf, sizeof ( lbuf ), "(%d) ", ( !IS_NPC( ch ) ) ? ch->pcdata->wizinvis : ch->mobinvis );


That must be much safer than the original.

Ideally there would be a way of comparing the before and after, automatically. eg. something like this:


assert (sizeof ( lbuf ) == MAX_STRING_LENGTH)


However I can't think of a way of making the C compiler do that.

- Nick Gammon

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

Posted by Nick Gammon   Australia  (23,158 posts)  Bio   Forum Administrator
Date Reply #29 on Sun 09 Jul 2006 06:33 AM (UTC)

Amended on Sun 09 Jul 2006 07:09 AM (UTC) by Nick Gammon

Message
I wanted to get this project finished, so I wrote another bit of Lua code to try to detect the cases where the "before" and "after" of my automated changes were not identical, and then follow those ones up.

This slightly hacky bit of code below does the trick:


-- pattern to detect a function declaration
pat = "[%a_]+%s+%*?%s*([%a%d_]+)%s*%b().-(%b{})"

olddir = "C:\\smaugfuss\\src\\"

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

found = 0
not_found = 0

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

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

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

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

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

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

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

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


function check_arg (s, size)
  match = string.find (saved_body,
                       "char%s+" .. s .. "%s*%[%s*" ..
                       size .. "%s*%]")
  if match then
    found = found + 1
  else
    not_found = not_found + 1
    key = s .. ":" .. size
    if not reported [key] then
      reported [key] = true
      ColourNote ("white", "red", 
                  "Cannot find declaration for " .. s .. " (size " .. size ..
                  ") in function " .. saved_name
                 )
    end -- if not already reported it
  end -- if

end -- check_arg

function process_function (name, body)
--  Note (" +++ function: " .. name)
  reported = {}
  saved_name = name -- in case we need to report it
  saved_body = body -- move to global scope for check_arg

  -- check each change
  for k, v in ipairs (replacements) do
     s, count = string.gsub (body, v.find, check_arg)
     file_count = file_count + count
  end -- for each replacement

end -- process_function

function process_file (name)
  local f, s
  file_count = 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

  -- fix up imc strange function defs

  s = string.gsub (s, "PFUN%( ([%a%d_]+) %)",
                   "void %1( IMC_PACKET *q, char *packet )")
  s = string.gsub (s, "IMC_CMD%( ([%a%d_]+) %)",
                   "void %1( CHAR_DATA *ch, char *argument )")

  s = string.gsub (s, "'}'", "")  -- confuses function finding

  string.gsub (s, pat, process_function)
 
  print (" -- " .. file_count .. " changes found")
end -- process_file 

print (string.rep ("-", 60))

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

for k in pairs (t) do
  process_file (k)
end -- for

Note ("Matches found = ", found)
Note ("No declaration found = ", not_found)



What this is trying to do is break up each file into a function declaration and body of the general form:


return_type name ( args ) { body }


It then scans each function body for the "snprintf" declarations, and for each individual one, then rescans the function body to try to find a local declaration of the form:


char <name> [ <size> ]


For example, if it locates:


snprintf( buf, MAX_STRING_LENGTH, "%s\r\n", pArea->resetmsg );


Then the name is "buf" and the size is "MAX_STRING_LENGTH".

So, it looks for:


char buf [MAX_STRING_LENGTH]


If it finds this (bearing in mind it must be inside the same function), then it assumes that it was a local declaration, and all is well.

There are a couple of kludgy aspects to this, in particular in the file imc.c a lot of function declarations are done using defines, like this:


PFUN( imc_recv_tell )


Thus there is a bit of pre-processing to put those back into standard function form.

Running this script on my copy of SmaugFUSS gave these results:


Processing: save.c
-- 11 changes found
Processing: act_obj.c
-- 13 changes found
Processing: hashstr.c
-- 3 changes found
Processing: shops.c
-- 24 changes found
Processing: comm.c
Cannot find declaration for pbuf (size MAX_STRING_LENGTH) in function display_prompt
-- 21 changes found
Processing: mud_prog.c
Cannot find declaration for rval (size MAX_STRING_LENGTH) in function isoperator
-- 6 changes found
Processing: planes.c
-- 0 changes found
Processing: skills.c
-- 29 changes found
Processing: fight.c
Cannot find declaration for buf2 (size 256) in function new_dam_message
Cannot find declaration for buf3 (size 256) in function new_dam_message
-- 22 changes found
Processing: act_wiz.c
Cannot find declaration for buf (size ( MAX_STRING_LENGTH) in function do_owhere
Cannot find declaration for buf (size MAX_STRING_LENGTH) in function do_bestow
Cannot find declaration for reboot_time (size 50) in function get_reboot_string
Cannot find declaration for arg2 (size MAX_INPUT_LENGTH) in function do_sedit
Cannot find declaration for buf (size ( MAX_STRING_LENGTH) in function do_ipcompare
-- 88 changes found
Processing: handler.c
-- 7 changes found
Processing: const.c
-- 0 changes found
Processing: mud_comm.c
-- 16 changes found
Processing: act_comm.c
Cannot find declaration for lbuf (size MAX_STRING_LENGTH) in function talk_channel
-- 31 changes found
Processing: imm_host.c
-- 0 changes found
Processing: services.c
-- 2 changes found
Processing: polymorph.c
Cannot find declaration for buf (size MAX_STRING_LENGTH) in function do_morphset
-- 5 changes found
Processing: sha256.c
-- 0 changes found
Processing: boards.c
-- 22 changes found
Processing: act_info.c
Cannot find declaration for buf (size ( MAX_STRING_LENGTH) in function do_exits
-- 34 changes found
Processing: interp.c
Cannot find declaration for buf (size MAX_STRING_LENGTH) in function write_watch_files
Cannot find declaration for cmd_flag_buf (size MAX_STRING_LENGTH) in function check_cmd_flags
-- 10 changes found
Processing: mapout.c
Cannot find declaration for buf (size MAX_STRING_LENGTH) in function count_lines
-- 1 changes found
Processing: imc.c
Cannot find declaration for buf2 (size LGST) in function imclog
Cannot find declaration for buf2 (size LGST) in function imcbug
Cannot find declaration for buf (size LGST) in function imc_recv_tell
Cannot find declaration for buf (size LGST) in function update_imchistory
Cannot find declaration for name (size SMST) in function imc_display_channel
Cannot find declaration for buf (size IMC_BUFF_SIZE) in function imc_recv_who
Cannot find declaration for lbuf2 (size LGST) in function imcfread_config_file
Cannot find declaration for to (size SMST) in function imccommand
Cannot find declaration for buf (size LGST) in function imcsetup
Cannot find declaration for buf1 (size LGST) in function imctell
Cannot find declaration for to (size SMST) in function imcremoteadmin
Cannot find declaration for buf (size LGST) in function imc_send_social
-- 107 changes found
Processing: build.c
Cannot find declaration for buf (size MAX_STRING_LENGTH) in function edit_buffer
-- 57 changes found
Processing: mpxset.c
-- 26 changes found
Processing: magic.c
-- 16 changes found
Processing: hotboot.c
Cannot find declaration for buf2 (size 100) in function do_hotboot
Cannot find declaration for buf3 (size 100) in function do_hotboot
-- 10 changes found
Processing: deity.c
-- 12 changes found
Processing: update.c
-- 16 changes found
Processing: act_move.c
-- 10 changes found
Processing: special.c
-- 7 changes found
Processing: color.c
-- 57 changes found
Processing: misc.c
-- 3 changes found
Processing: makeobjs.c
-- 7 changes found
Processing: reset.c
Cannot find declaration for objname (size MAX_STRING_LENGTH) in function sprint_reset
-- 14 changes found
Processing: comments.c
-- 0 changes found
Processing: mccp.c
-- 0 changes found
Processing: player.c
Cannot find declaration for buf (size MAX_STRING_LENGTH) in function do_statreport
-- 163 changes found
Processing: db.c
Cannot find declaration for buf (size ( MAX_STRING_LENGTH) in function bug
Cannot find declaration for buf (size ( MAX_STRING_LENGTH) in function boot_log
-- 27 changes found
Processing: track.c
-- 8 changes found
Processing: clans.c
-- 13 changes found
Processing: tables.c
-- 7 changes found
Processing: ban.c
-- 4 changes found
Matches found = 834
No declaration found = 75


As you can see there were a handful reported (75 counted but duplicates were suppressed from the display).

I then went through to see if any of the automated conversions were likely to be wrong (ie. converting from the original values to "sizeof (buf)". Most could be accounted for by the fact that the function used this syntax:


char arg1[MAX_INPUT_LENGTH], arg2[MAX_INPUT_LENGTH];


In this case "arg2" does not match my test (it is not directly preceded by "char") however it is obviously still correct.

However I have detected 5 cases where I think the actual SMAUG code (in its current form) is wrong. See next post for those.

- Nick Gammon

www.gammon.com.au, www.mushclient.com
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.


83,982 views.

This is page 2, subject is 3 pages long:  [Previous page]  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.