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

Temporary values are not destructed in C, e.g. string interpolation leaks memory #26

Open
yeputons opened this issue Oct 9, 2021 · 10 comments
Assignees
Labels
bug Something isn't working C

Comments

@yeputons
Copy link

yeputons commented Oct 9, 2021

Consider OpAddAssignString.ci:

		s2 += p + $"{p}" + s; //FAIL: cpp - should work with C++20

gets compiled to

	CiString_Assign(&s2, CiString_Format("%s%s%s%s", s2, p, p, s));

here, the result of CiString_Format is a result of malloc. It's passed to CiString_Assign and abandoned afterwards, leaking memory. This does not happen in C++ because temporary values are automatically destructed in C++, including std::string.

I assume there may be other instances where temporary objects are created and not immediately stored into variables.

@pfusik
Copy link
Collaborator

pfusik commented Oct 9, 2021

All tests are run in Travis CI. C, C++ and Swift tests are run with the Address Sanitizer and OpAddAssignString passes.

There's this line in the C translation of OpAddAssignString:

free(s2);

Why do you think it leaks memory?

@yeputons
Copy link
Author

yeputons commented Oct 9, 2021

I'm sorry, I misread the example. Here is a simpler one:

public static class Test
{
	static void Foo(string x) {
	}
	public static void Run()
	{
	    int x;
	    Foo($"{x}");
	}
}

gets compiled to

static void Test_Foo(const char *x)
{
}

void Test_Run(void)
{
	int x;
	Test_Foo(CiString_Format("%d", x));
}

There is no free in this program at all.

pfusik added a commit that referenced this issue Oct 9, 2021
pfusik added a commit that referenced this issue Oct 9, 2021
@pfusik
Copy link
Collaborator

pfusik commented Oct 9, 2021

Yes, it's a known issue. I've just added a test and will fix it.

Interpolated strings are even worse when targetting C++, because they use C++20 <format>. Even though C++20 was ratified some time ago, clang only now starts implementing <format>.

@pfusik pfusik self-assigned this Oct 9, 2021
@yeputons
Copy link
Author

yeputons commented Oct 9, 2021

I believe the problem also exists with dynamic references in general. E.g. the following code:

class Foo {
}
public static class Test
{
	public static void Consume(Foo f) {}
	public static Foo# Create() { return new Foo(); }
	public static void Run() {
		Consume(Create());
	}
}

leaks memory and does not induce the Dynamically allocated objects must be assigned to a Foo# reference error, as Consume(new Foo()) would.

@pfusik
Copy link
Collaborator

pfusik commented Oct 9, 2021

cito cannot determine all the dangling reference scenarios. If it were easy, C and C++ compilers would do it, given five decades of their development.
Consume(Create()) would be valid code if Create() stored the dynamic reference somewhere. The Create method itself is valid if used correctly.

@jedwards1211
Copy link
Contributor

FWIW, I just learned that smart pointers are possible in C with GNU compilers: https://stackoverflow.com/questions/799825/smart-pointers-safe-memory-management-for-c

@pfusik pfusik added the C label Nov 5, 2021
pfusik added a commit that referenced this issue Nov 22, 2022
@pfusik pfusik added the bug Something isn't working label Aug 13, 2023
@pfusik
Copy link
Collaborator

pfusik commented Jan 3, 2024

@Timerix22 af1f308 fixes the concatenation of multiple strings.

I will work on the remaining cases.

pfusik added a commit that referenced this issue Jan 15, 2024
pfusik added a commit that referenced this issue Jan 15, 2024
pfusik added a commit that referenced this issue Jan 17, 2024
pfusik added a commit that referenced this issue Mar 12, 2024
pfusik added a commit that referenced this issue Mar 13, 2024
pfusik added a commit that referenced this issue Mar 13, 2024
pfusik added a commit that referenced this issue Mar 14, 2024
@pfusik
Copy link
Collaborator

pfusik commented Mar 25, 2024

All cases reported here are now fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working C
Projects
None yet
Development

No branches or pull requests

4 participants
@pfusik @yeputons @jedwards1211 and others