Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Writing to readonly data gives SIGBUS error; should be CT error instead #8463

Closed
mratsim opened this issue Jul 27, 2018 · 10 comments
Closed

Comments

@mratsim
Copy link
Collaborator

mratsim commented Jul 27, 2018

While wrapping a C library and trying to understand why it could write to ptr char but segfaulted when I asked it to write to (var) cstring, I came upon the following issue:

var x: cstring = "0123456789"
x[1] = 'A' # SIGBUS: Illegal storage access. (Attempt to read from nil?)

echo x

import typetraits
echo x.type.name

Note that this is a SIGBUS (accessing unaddressable memory) not a SIGSEGV (accessing unauthorized memory).

@mratsim
Copy link
Collaborator Author

mratsim commented Jul 27, 2018

Tagging @yglukhov, in C we can reproduce it with:

int main(int argc, char **argv)
{
    char* s = "0123456789";
    s[0] = 'c';
    return 0;
}

which points to constant memory in the executable

and it can be fixed by using

int main(int argc, char **argv)
{
    char s[10] = "0123456789";
    s[0] = 'c';
    return 0;
}

@Bulat-Ziganshin
Copy link

Bulat-Ziganshin commented Jul 27, 2018

It may be related to #8370 - Nim doesn't distinguish char * and const char *, so it can try to write into constant C strings. The "solution", indeed, is to make all Nim literal strings mutable, i.e. "123" Nim string constant should be declared as char str[] = "123" rather than erroneous char* str = "123" (for the later, modern C++ compilers complain that you assign const char * to char*).

@mratsim
Copy link
Collaborator Author

mratsim commented Jul 27, 2018

I think we can just distinguish between const/let literals and var cases for cstring. This would avoid unnecessary runtime allocation.

@Bulat-Ziganshin
Copy link

Bulat-Ziganshin commented Jul 27, 2018

I don't have an idea whether and to which extent it's possible. I explained here that while we have let/const on higher level, we can't include such fields into structures, so the protection may be leaky. But it's just a wild assumption from general theoretical viewpoint :)

At least, compiler writers will need to check how their final solution plays with deeper object hierarchies.

@Araq
Copy link
Member

Araq commented Jul 30, 2018

I don't think there is anything to fix here, cstring literals are read-only.

@Araq Araq closed this as completed Jul 30, 2018
@Araq Araq added the Won't Fix label Jul 30, 2018
@ghost
Copy link

ghost commented Jul 30, 2018

@Araq Give a better error message than SIGBUS?

@Araq
Copy link
Member

Araq commented Jul 30, 2018

SIBGUS vs SIGSEGV is just a OSX thing and custom signal handlers for nicer error messages cause more problems than they solve. cstring is for C interop, sharp edges.

@timotheecour
Copy link
Member

timotheecour commented May 22, 2020

I don't think there is anything to fix here, cstring literals are read-only.

there should be a CT error, it should be illegal to write to rodata

I was going to file an issue but I searched and found this. Here's another example, same root cause:

const a = "abc"
var x = a.cstring
x[0]='t' # SIGBUS: Illegal storage access.

This is related also to nim-lang/RFCs#126 (comment):

D distinguishes between enum a = expr;: enum creates a compile-time only construct; it has no address; pretty close to nim's const a = expr
static const a = new Foo(): this computes at CT as well but puts in the static data segment, and has an address you can access at runtime. in nim, that'd be the analog, for example, of let a = "foo".cstring, which puts "foo" in the data segment as readonly memory

and solving this issue properly would enable default values for object properties with safe semantics even for ptr-like types, among other things.

There should be a storage class to represent rodata, and prevent write access at CT. How exactly to do that needs to be determined but this problem is solvable (and other languages, eg D, handle this aspect well).

I have a concrete proposal in mind but no RFC yet, that would work not only for this problem but also for escape analysis (for first class openarrays)

note

a simpler fix might be possible by using C arrays (char[]) instead of c string litterals in cgen, but this might have performance implications; EDIT: this would prevent returning a cstring from a proc, as it'd allocate on stack, so it's not a viable alternative in most cases

related

@timotheecour timotheecour changed the title Writing to cstring, SIGBUS error Writing to readonly data gives SIGBUS error; should be CT error instead May 22, 2020
@Araq
Copy link
Member

Araq commented May 22, 2020

there should be a CT error, it should be illegal to write to rodata

There is no bug here. Let's keep this closed please. If some RFC gets accepted that improves the situation, that's fine, but with the current spec it is not a bug the compiler needs to detect.

@Araq Araq closed this as completed May 22, 2020
ringabout added a commit to ringabout/Nim that referenced this issue Nov 7, 2020
Araq added a commit that referenced this issue Nov 12, 2020
… allowed (#15878)

* follow #8463 #14157 and document cstring literals
* Update doc/manual.rst

Co-authored-by: Juan Carlos <[email protected]>
Co-authored-by: Andreas Rumpf <[email protected]>
PMunch pushed a commit to PMunch/Nim that referenced this issue Jan 6, 2021
…ification is not allowed (nim-lang#15878)

* follow nim-lang#8463 nim-lang#14157 and document cstring literals
* Update doc/manual.rst

Co-authored-by: Juan Carlos <[email protected]>
Co-authored-by: Andreas Rumpf <[email protected]>
mildred pushed a commit to mildred/Nim that referenced this issue Jan 11, 2021
…ification is not allowed (nim-lang#15878)

* follow nim-lang#8463 nim-lang#14157 and document cstring literals
* Update doc/manual.rst

Co-authored-by: Juan Carlos <[email protected]>
Co-authored-by: Andreas Rumpf <[email protected]>
@timotheecour
Copy link
Member

this could be fixed by timotheecour#524

irdassis pushed a commit to irdassis/Nim that referenced this issue Mar 16, 2021
…ification is not allowed (nim-lang#15878)

* follow nim-lang#8463 nim-lang#14157 and document cstring literals
* Update doc/manual.rst

Co-authored-by: Juan Carlos <[email protected]>
Co-authored-by: Andreas Rumpf <[email protected]>
ardek66 pushed a commit to ardek66/Nim that referenced this issue Mar 26, 2021
…ification is not allowed (nim-lang#15878)

* follow nim-lang#8463 nim-lang#14157 and document cstring literals
* Update doc/manual.rst

Co-authored-by: Juan Carlos <[email protected]>
Co-authored-by: Andreas Rumpf <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants