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

NRT realloc failure #1537

Closed
sonoro1234 opened this issue Jun 12, 2015 · 19 comments
Closed

NRT realloc failure #1537

sonoro1234 opened this issue Jun 12, 2015 · 19 comments
Labels
bug Issues that relate to unexpected/unwanted behavior. Don't use for PRs. comp: scsynth

Comments

@sonoro1234
Copy link
Contributor

Hello,

I had too comment this lines (SC_World.cpp:553)

packet->mData = (char *)realloc((void *)packet->mData, (size_t)msglen);
if(!packet->mData) throw std::runtime_error("nextOSCPacket: realloc
failed...\n");

to avoid error in realloc in NRT

I cant see why (msglen was not 0)
any ideas?
(win32 vista mingw-w64 32 bits)

victor

@telephon
Copy link
Member

What did you realloc? A Bus.realloc would, as it seems, not concern the server.

@sonoro1234
Copy link
Contributor Author

I did not realloc anything.

NRT does realloc according to commit f3f0f81 to avoid 8192 limit.

It is the first time I try NRT so the issue appears now

@sonoro1234
Copy link
Contributor Author

solved the issue but need some supervision:

in SC_ World.cpp
we have

# if SC_DEBUG_MEMORY
#  define malloc(size)          sc_dbg_malloc((size), __FUNCTION__, __LINE__)
#  define free(ptr)             sc_dbg_free((ptr), __FUNCTION__, __LINE__)
#  define zalloc_(n, size)      sc_dbg_zalloc((n), (size), __FUNCTION__, __LINE__)
# else
#  define malloc(size)          sc_malloc((size))
#  define free(ptr)             sc_free((ptr))
#  define zalloc_(n, size)      sc_zalloc((n), (size))
# endif // SC_DEBUG_MEMORY

so that malloc is not malloc but nova:malloc_aligned
but realloc needs a pointer previously allocated with malloc

solutions:
1--first of all I think that malloc should not be redefined as it is too hacky. If I am right and this define is only valid in SC_World.cpp all malloc should be sc_malloc except the really intended mallocs as this one

2--wraping malloc as:

void* malloc_orig(size_t size){
    return ::malloc(size);
}

and using malloc_orig worked for me

3-changing defines as:

# if SC_DEBUG_MEMORY
#  define malloc(size)          sc_dbg_malloc((size), __FUNCTION__, __LINE__)
#  define free(ptr)             sc_dbg_free((ptr), __FUNCTION__, __LINE__)
#  define zalloc_(n, size)      sc_dbg_zalloc((n), (size), __FUNCTION__, __LINE__)
# else
#  define malloc_orig(size)        ::malloc(size)
#  define free_orig(ptr)              ::free(ptr)
#  define malloc(size)          sc_malloc((size))
#  define free(ptr)             sc_free((ptr))
#  define zalloc_(n, size)      sc_zalloc((n), (size))
# endif // SC_DEBUG_MEMORY

did not work for me althought it should have

@danstowell
Copy link
Member

Good catch Victor. I just checked a spec and yes realloc behaviour is undefined if applied to memory not allocated from the standard functions.

The #define is used only in SC_World.cpp and then used a small number of times, so it should be safe to rename it as something local such as "debuggable_malloc(size)", i.e. your option 1.

@sonoro1234
Copy link
Contributor Author

As the problem only seems to appear on win32 (macosx uses malloc as malloc_aligned and nobody complains on linux) I can make a commit and PR for that.

@sonoro1234
Copy link
Contributor Author

Can anybody explain me why option 3 is not working?

@sonoro1234
Copy link
Contributor Author

first I would like to ask tim @timblechmann which are the allocations that must be aligned an which not

@timblechmann
Copy link
Contributor

sc_malloc was always aligned and it is passed to plugins via the interface table. in general memory should be aligned. many malloc implementations provide aligned memory by default (most notably apple's malloc), while some don't (win32 malloc).

@sonoro1234
Copy link
Contributor Author

Should OSC_Packet.mData be aligned?

@timblechmann
Copy link
Contributor

it doesn't harm ... but it probably won't crash if it isn't

@sonoro1234
Copy link
Contributor Author

Dan says #define malloc sc_malloc only affects SC_World.cpp but I just found

The #define tokens have global scope during compilation. They get defined when the compiler first encounters them and once defined they remain defined across files. Their scope only ends when the compilation completes or when the compiler encounters a explicit #undef directive for the token.

So if I change this define it will affect other cpp files perhaps?

@timblechmann
Copy link
Contributor

it will not affect other cpp files. but it is added as a function pointer to the interface table. this means that it can potentially be called everywhere:

ft->fNRTAlloc = &malloc;

    ft->fNRTAlloc = &malloc;
    ft->fNRTRealloc = &realloc;
    ft->fNRTFree = &free;

obviously this is broken as well.

best way is to implement an sc_realloc based on sc_malloc and sc_free.

@sonoro1234
Copy link
Contributor Author

I have seen some realloc calls in sc code.
for example

refs = (int16*)realloc(refs, refsMaxSize*2*sizeof(int16));

in SC_GraphDef.cpp:827
I am surprised of not getting crashes.
Is it just good fortune?

@timblechmann
Copy link
Contributor

check where the memory is coming from! is it allocated via the function table?

@sonoro1234
Copy link
Contributor Author

Thats true it comes from calloc.
Implementining an sc_realloc is difficult for getting block previous size. Can be done but perhaps I will try something easier.
BTW why option 3 is failing

# if SC_DEBUG_MEMORY
#  define malloc(size)          sc_dbg_malloc((size), __FUNCTION__, __LINE__)
#  define free(ptr)             sc_dbg_free((ptr), __FUNCTION__, __LINE__)
#  define zalloc_(n, size)      sc_dbg_zalloc((n), (size), __FUNCTION__, __LINE__)
# else
#  define malloc_orig(size)        ::malloc(size)
#  define free_orig(ptr)              ::free(ptr)
#  define malloc(size)          sc_malloc((size))
#  define free(ptr)             sc_free((ptr))
#  define zalloc_(n, size)      sc_zalloc((n), (size))
# endif // SC_DEBUG_MEMORY

@timblechmann
Copy link
Contributor

calloc is equivalent to malloc+memset: http://en.cppreference.com/w/c/memory/calloc

cannot comment about your "option 3", but make sure that malloc_orig won't resolve to ::sc_malloc ... e.g. by checking the preprocessed sources

@sonoro1234
Copy link
Contributor Author

made PR #1549

@sonoro1234
Copy link
Contributor Author

How could this bug live for so long time without complains. One conclusion could be: nobody uses NRT in win32 (until now)

@sonoro1234
Copy link
Contributor Author

Could somebody merge?

@crucialfelix crucialfelix added bug Issues that relate to unexpected/unwanted behavior. Don't use for PRs. comp: scsynth labels Jan 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues that relate to unexpected/unwanted behavior. Don't use for PRs. comp: scsynth
Projects
None yet
Development

No branches or pull requests

5 participants