Command "tech load" segfaults

[gt74745]

Using either magic 8.3.496 from the OCD website, or this repo, I'm unable to load any tech files.

Here's my console output:

$ magic -noc
Use openwrapper to create a new GUI-based layout window
Use closewrapper to remove a new GUI-based layout window

Magic 8.3 revision 496 - Compiled on Thu Oct 10 11:11:59 AM CDT 2024.
Starting magic under Tcl interpreter
Using the terminal as the console.
Using TrueColor, VisualID 0x21 depth 24
Processing system .magicrc file
New windows will not have a title caption.
New windows will not have scroll bars.
New windows will not have a border.
Using technology "minimum", version 0.0
Root cell box:
           width x height  (   llx,  lly  ), (   urx,  ury  )  area (units^2)

microns:   1.000 x 1.000   ( 0.000,  0.000), ( 1.000,  1.000)  1.000
lambda:        1 x 1       (     0,  0    ), (     1,  1    )  1
% tech load scmos
Input style lambda=1.0(gen): scaleFactor=100, multiplier=1
Segmentation fault (core dumped)

And trying to load the techfile at run exits immediately:

$ magic -T scmos -noc
Use openwrapper to create a new GUI-based layout window
Use closewrapper to remove a new GUI-based layout window

Magic 8.3 revision 496 - Compiled on Thu Oct 10 11:11:59 AM CDT 2024.
Starting magic under Tcl interpreter
Using the terminal as the console.
Using TrueColor, VisualID 0x21 depth 24
Input style lambda=1.0(gen): scaleFactor=100, multiplier=1
Segmentation fault (core dumped)

Trying to load a techfile in the gui produces this message:

    while executing
"magic::tech load scmos "
    invoked from within
".techmgr.title.tname.menu invoke active"
    ("uplevel" body line 1)
    invoked from within
"uplevel #0 [list $w invoke active]"
    (procedure "tk::MenuInvoke" line 50)
    invoked from within
"tk::MenuInvoke .techmgr.title.tname.menu 1"
    (command bound to event)

All of these problems persist no matter which tech file I try to use.

[RTimothyEdwards]

@dlmiles : The syntax change in DRCtech.c line 829 from

char *locargv[2][10] = {"style", "default};

to

char *locargv[2][10] = {{"style"}, {"default"}};

is causing this segfault. What is the solution that both has the correct behavior and does not produce compiler warnings?

[RTimothyEdwards]

I think the answer is that the original declaration was wrong although I'm not sure why it used to compile correctly. It should have been

char *locargv[2] = {"style", "default"};

and should not have a second array bound. The second array bound was being ignored (probably just wasting memory), but when the double brace was added, it became incorrect when passed to DRCTechLine which was expecting char *argv[]. I have a fix and will push it now.

[RTimothyEdwards]

@gt74745 : This has been fixed on opencircuitdesign.com and will update on github if I force a mirror by end of day today; otherwise, it will update automatically overnight. Thank you for the bug report.

[dlmiles]

The issue is the incorrect use of * let me attach test case to demonstrate. More fully char locargv[2][10] = {{"style"}, {"default"}}; // note the '*' is removed, these are not pointers

the idiom char [2][10] reads, like it is wanting to perform the equivalent of:

char locargv[2][10];
strcpy(&locargv[0], "style");
strcpy(&locargv[1], "default");
// This allows the passing is writable-strings to the API call

Just checking other changes for this pattern.

[RTimothyEdwards]

@dlmiles: But this works too, and is as far as I know syntactically "correct":

char *locargv[2] = {"style", "default"};

I do not expect that other changes would have caused an error; the error was in the original declaration which specified an array of one more dimension than needed, and the tools analyzing the code tried to fix the value instead of the declaration, not knowing any better. Which I guess validates the strict syntax checking. : )

[dlmiles]

Yes that can work as well (inferred array size of const char *) but this will not pass writable-strings to the API. Given the timescale / history of the codebase, I would assume the API needs writable-strings. If original code is written in that way, to copy from constant/rodata into writable data on the stack. The original buffer was meant to be 20 bytes.

[RTimothyEdwards]

Okay, you've convinced me that a fixed array + strcpy( ) is the safer way to go.

[dlmiles]

I think just removing the * from my commit, but no hurry if you have a fix for overnight, will attach testcase shortly to documented. As in char locargv[2][10] = {{"style"}, {"default"}}; will work as intended, but will know after a little more investigation.

[RTimothyEdwards]

I'm pretty sure that was the first thing I tried and it didn't work.

[dlmiles]

It is a recursive function call. So easy to audit if it requires writable strings, it will StrDup() when needed in some places to store data but has: https://github.com/RTimothyEdwards/magic/blob/master/drc/DRCtech.c#L745 https://github.com/RTimothyEdwards/magic/blob/master/drc/DRCtech.c#L803 https://github.com/RTimothyEdwards/magic/blob/master/drc/DRCtech.c#L895 https://github.com/RTimothyEdwards/magic/blob/master/drc/DRCtech.c#L897 that looks like it could write into the argv[] strings if these lines are triggered (needing writable-strings to be passed).

so the smallest and quickest to audit solution looks like:

    char bufargv[2][10];
    strcpy(bufargv[0], "style");
    strcpy(bufargv[1], "default");

    char *locargv[2];
    locargv[0] = bufargv[0];
    locargv[1] = bufargv[1];

    if (DRCTechLine(sectionName, 2, locargv) == FALSE)
        return FALSE;

Note the 2 phases, the preparation of a writable strings buffer, then the preparation of the argv[] pointers, to have the correct layout for the function call.

[dlmiles]

test_issue342.c.txt

Testcase file to help confirming.

[dlmiles]

I'm pretty sure that was the first thing I tried and it didn't work.

Probably because it did the writable-string buffer preparation part ok, but it doesn't provide the correct pointer layout as the function expects.

I believe the original code passed in the correct layout, with pointers to read-only/constant strings and did not trigger the 4 lines of code I link to in previous comment.

[RTimothyEdwards]

I have adopted your final recommendation and committed the code.

[gt74745]

This fix works on my system. Thanks for the prompt response, guys! :)