ReadARgs() bug in Roadshow AddNetInterface

[Bruce Abbott]

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:-

	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:-

        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:-

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():-

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.

[daxb]

Did you send a bugreport to olsen?

[Bruce Abbott]

Originally Posted by daxb

Did you send a bugreport to olsen?

No. Where do I send it? Who is olsen?

[daxb]

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.

[Minuous]

Originally Posted by Bruce Abbott

SASC cannot handle identifiers longer than 32 characters.

That's not correct, you can easily use the IdentifierLength option to increase it.

[Bruce Abbott]

Originally Posted by Minuous

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?

[Thomas Richter]

Originally Posted by Bruce Abbott

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

 smake

and see all the magic at work. AmigaOs does not built any different (just that the makefile is a bit more involved.)

[Bruce Abbott]

Originally Posted by Thomas Richter

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!

[Olaf Barthel]

Originally Posted by Bruce Abbott

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:

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...

[A10001986]

Hi, I am running Roadshow under 2.04 and am heavily plagued by this bug. Would it be possible to provide me with a fixed AddNetInterface/bsd-lib until this is fixed officially?

[A10001986]

Never mind, I wrote a little patch for ReadArgs, faking 3.1 behavior. Now it works. Thanks for your research, Bruce! Much appreciated!

https://www.dropbox.com/s/5k7umvjqzi...argspatch?dl=0

[tiim]

Necro-thread alert.

I am using KS 2.04 and WB2.1 and this error has borked my Amiga, and I just bought the full version of Roadshow.

My WB is hung from HDD and I must boot from floppy to gain it back.

I will look into the ReadArgs patch above, not sure if just I put it somewhere or just execute it via cli or script...

EDIT:
I removed my x-surf device from the Devs/NetInterfaces drawer, rebooted into WB from HDD just fine.

Next, I put the x-surf device back in the drawer and copied the "readargspatch" from @A10001986's DropBox, to my C: drawer (where all the other Amiga commands live) and edited my S:startup-sequence with the following line near the top (third line):

C:readargspatch QUIET

Rebooted and everything is OK from HDD into WB 2.1 and I have network access.

Hopefully this is fixed in an update to Roadshow. I have version 1.14

Thanks!

Tim