English Amiga Board


Go Back   English Amiga Board > Coders > Coders. System

 
 
Thread Tools
Old 15 August 2020, 09:13   #1
Bruce Abbott
Registered User

Bruce Abbott's Avatar
 
Join Date: Mar 2018
Location: Hastings, New Zealand
Posts: 374
ReadARgs() bug in Roadshow AddNetInterface

I recently purchased Roadshow, which is 5 times faster in IBrowse than AmiTCP on my A1200. Unfortunately I had intermittent problems with it failing to add the CNet network card. Sometimes it would progressively eat up all the RAM, other times it would crash or say the hard drive had a checksum failure. Mungwall occasionally complained of trashed free memory, so I suspected a buffer overflow, memory used after being freed, or a wild pointer.

Roadshow kindly supplies source code to the utility programs in its SDK, so I browsed the source of 'AddNetInterface'. Nothing jumped out me though, and it would need extensive modifications to compile under SASC 6.58. So I disassembled the program, reassembled it to get symbols and stepped through it in Monam (Hisoft DEVPAC debugger). A few days and many happy(?) computing hours later, I finally found the problem:-

'AddNetInterface' uses the DOS library function ReadArgs() to parse the lines in the network interface configuration file. It actually parses all the lines twice, the first time to open the network card driver and the second time to configure it. As it does this it builds a taglist which is then passed to Roadshow's BSDSocket library function AddInterfaceTagList(). This taglist was sometimes corrupted, causing Roadshow to attempt to allocate a huge number of network buffers.

But why?

In the C source of AddNetInterface we see:-
Code:
	cc->cc_RDA->RDA_Source.CS_Buffer = s;
	cc->cc_RDA->RDA_Source.CS_Length = len;
	cc->cc_RDA->RDA_Source.CS_CurChr = 0;
	cc->cc_RDA->RDA_Flags |= RDAF_NOPROMPT;

	if(CANNOT ReadArgs((STRPTR)args_template,(LONG *)&args,cc->cc_RDA))
This fills in the necessary fields of a RDArgs structure, which is defined as:-
Code:
        struct  RDArgs {
        struct  CSource RDA_Source;   /* Select input source */
        LONG    RDA_DAList;           /* PRIVATE. */
        UBYTE   *RDA_Buffer;          /* Optional string parsing space. */
        LONG    RDA_BufSiz;           /* Size of RDA_Buffer (0..n) */
        UBYTE   *RDA_ExtHelp;         /* Optional extended help */
        LONG    RDA_Flags;            /* Flags for any required control */
        };
This is supplied as the third parameter to ReadArgs(), to give it the line buffer address (inserted into the CSource structure). RDA_Buffer is not used, and so doesn't have to be initialized, right? Wrong!

The problem was that the program reused the same RDArgs structure for each line without clearing RDA_Buffer, but in WB3.0 and below ReadArgs() inserts its own buffer address into this space. Therefore the next time ReadArgs() was called it would think the caller was supplying an external buffer, when it was actually its own buffer which had been freed via FreeArgs(). Most of the time this would work even though it was using deallocated memory, except when the taglist memory was allocated in the same spot.

In WB3.1 and above ReadArgs() clears RDA_Buffer on return if the caller didn't supply it, so this bug does not occur. However the documentation for ReadArgs() says:-

Quote:
If you pass in a RDArgs structure, you MUST reset (clear or set)
RDA_Buffer for each new call to RDArgs. The exact behavior if you
don't do this varies from release to release and case to case; don't
count on the behavior!
The solution was to add the following line before calling ReadArgs():-

Code:
cc->cc_RDA->RDA_Buffer = NULL;
I inserted the assembler equivalent of this into my disassembled source code. I also attempted to modify the C source to suit SASC 6.58, which was a lot more work than I expected because SASC cannot handle identifiers longer than 32 characters. Someone with a more modern C compiler setup should be able to just add the line above and recompile it in a few minutes.

Last edited by Bruce Abbott; 15 August 2020 at 09:20.
Bruce Abbott is offline  
Old 15 August 2020, 11:44   #2
daxb
Registered User
 
Join Date: Oct 2009
Location: Germany
Posts: 2,679
Did you send a bugreport to olsen?
daxb is offline  
Old 15 August 2020, 17:02   #3
Bruce Abbott
Registered User

Bruce Abbott's Avatar
 
Join Date: Mar 2018
Location: Hastings, New Zealand
Posts: 374
Quote:
Originally Posted by daxb View Post
Did you send a bugreport to olsen?
No. Where do I send it? Who is olsen?
Bruce Abbott is offline  
Old 15 August 2020, 18:26   #4
daxb
Registered User
 
Join Date: Oct 2009
Location: Germany
Posts: 2,679
Sorry, I mean the author of Roadshow also know as olsen. On eab he is a member with his real name: https://eab.abime.net/member.php?u=19176

You can also use the Roadshow support forum: https://www.amigafuture.de/viewforum.php?f=34

The Roadshow documentation has a "Publisher and support" part with contact addresses.
daxb is offline  
Old 15 August 2020, 18:35   #5
Minuous
Coder/webmaster/gamer
Minuous's Avatar
 
Join Date: Oct 2001
Location: Canberra/Australia
Posts: 2,164
Quote:
Originally Posted by Bruce Abbott View Post
SASC cannot handle identifiers longer than 32 characters.
That's not correct, you can easily use the IdentifierLength option to increase it.
Minuous is offline  
Old 17 August 2020, 05:43   #6
Bruce Abbott
Registered User

Bruce Abbott's Avatar
 
Join Date: Mar 2018
Location: Hastings, New Zealand
Posts: 374
Quote:
Originally Posted by Minuous View Post
That's not correct, you can easily use the IdentifierLength option to increase it.
Thanks for that. The text manual I have for SASC 6.5 says default length is 255, so I figured something else must be limiting it. Perhaps 255 is actually the maximum.

With IdentifierLength increased to 255 only a couple of other minor changes were needed to get it to compile. The modified AddNetInterface works fine as a CLI command, but causes a Mungwall hit on exit if invoked from Workbench (as default tool in the interface icon). Not a big deal, but I will try to find what is causing it.

I mostly program in assembler and find the plethora of options in some C compilers to be quite confusing. When compiler settings aren't supplied with the source code, how do you decide what setting to use?
Bruce Abbott is offline  
Old 17 August 2020, 16:28   #7
Thomas Richter
Registered User
 
Join Date: Jan 2019
Location: Germany
Posts: 600
Quote:
Originally Posted by Bruce Abbott View Post
I mostly program in assembler and find the plethora of options in some C compilers to be quite confusing. When compiler settings aren't supplied with the source code, how do you decide what setting to use?
You do not. The author (Olaf) does. You typically do not compile the source manually, but use the (hopefully supplied) makefile for that, which contains all the options, either directly, or as SCOPTIONS file. Just type
Code:
 smake
and see all the magic at work. AmigaOs does not built any different (just that the makefile is a bit more involved.)
Thomas Richter is offline  
Old 18 August 2020, 11:29   #8
Bruce Abbott
Registered User

Bruce Abbott's Avatar
 
Join Date: Mar 2018
Location: Hastings, New Zealand
Posts: 374
Quote:
Originally Posted by Thomas Richter View Post
You do not. The author (Olaf) does. You typically do not compile the source manually, but use the (hopefully supplied) makefile for that, which contains all the options, either directly, or as SCOPTIONS file.
In this case a makefile was not supplied, but I see your point. Examining some makefiles should help me to get familiar with what settings are typically used. I am also studying all the compiler documentation and trying the examples. Should be an expert in no time!
Bruce Abbott is offline  
Old 21 October 2020, 14:35   #9
Olaf Barthel
Registered User
 
Join Date: Aug 2010
Location: Lehrte, Germany
Posts: 262
Quote:
Originally Posted by Bruce Abbott View Post
In this case a makefile was not supplied, but I see your point. Examining some makefiles should help me to get familiar with what settings are typically used. I am also studying all the compiler documentation and trying the examples. Should be an expert in no time!
Thank you for finding the problem, reporting it and providing a fix

I have just updated the AddNetInterface command as well as bsdsocket.library itself which all were affected by the ReadArgs() problem (as present in Kickstart 2.04 and 3.0). Some testing will have to follow before yet another Roadshow update can be released...

As for the missing SMakefile: the source code for the commands was provided so that it might be reviewed, copied and adapted (which is both permitted and encouraged by yours truly). You'd still have to do the legwork for integrating that code into your own work. Also, I'm a lazy fellow and didn't want to write another SMakefile for inclusion with the source code...

That said, the compiler command needed to build the programs is rather simplistic, e.g. here is how it looks like for the AddNetInterface command using SAS/C 6.50:

Code:
sc cpu=any params=r data=faronly idlen=96 modified addsym link nostartup idir=include-sdk to AddNetInterface noicons AddNetInterface.c swap_stack.asm
The "idlen" option allows identifier names to become up to 96 characters before they get truncated. This somewhat excessive length is due to the Roadshow locale catalog header file identifiers being rather long...

Last edited by Olaf Barthel; 21 October 2020 at 14:47.
Olaf Barthel is offline  
 


Currently Active Users Viewing This Thread: 1 (0 members and 1 guests)
 
Thread Tools

Similar Threads
Thread Thread Starter Forum Replies Last Post
ReadArgs implementation for kickstart 1.3 jotd Coders. Asm / Hardware 18 31 December 2018 15:16
Roadshow and AmigaExplorer McTrinsic support.Other 16 27 December 2017 22:33
Help please strugglng to get multiple options out of ReadArgs chocsplease Coders. C/C++ 28 23 July 2017 19:07
Bug in x64 file requester and bug in Blizzard PPC ROM filesize headkase support.WinUAE 5 26 June 2016 14:17
Using ReadArgs() from asm oRBIT Coders. General 4 11 May 2010 16:11

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off

Forum Jump


All times are GMT +2. The time now is 19:34.


Powered by vBulletin® Version 3.8.11
Copyright ©2000 - 2020, vBulletin Solutions Inc.
Page generated in 0.08057 seconds with 15 queries