Hi Simon,

Not sure I see where a bug could be hiding. The scoping around those blocks seems fine to me.
setActiveDisplays(), though taking a const char * as an argument, never actually stores it internally, instead opting to store the results of another call to SplitStringEnvStyle().

SplitStringEnvStyle() makes a new std::string via pystring::strip as one of its first operations, so there is no possibility of char pointers falling out of scope.

This has been fairly stable code (since around 2011), though admittedly OCIOYaml.cpp has recently undergone some changes.

Would you happen to have some more information as to your development environment? Visual Studio / OCIO versions / ways to reproduce the crash you're seeing?

With a little more information I'll hopefully be able to root out the cause of the issue.

Cheers,
Mark

On Tue, Jul 22, 2014 at 4:03 PM, simon smith <si.c...@...> wrote:

I'm testing out some code and I've noticed a crash issue when a config (such as ACES, from the ICIO website) contain multiple active displays or views.

Basically, the code in ICIOYaml.cpp that takes an array of displays tries to join them together into one string using the JoinStringEnvStyle function (in ParseUtils.cpp) is flawed.

JoinStringEnvStyle will return a new std::string instance if more than one string is found in the array (it will pass the original "array" string back if there is only one entry in it).

The caller of JoinStringEnvStylein OCIOYaml.cpp takes a const pointer to this returned std::string to pass to the next function (setActiveDisplays) but it falls out of scope as soon as the const char* has been taken (it goes out of scope as it enters the next line of code). Thus on the next line the pointer is pointing to freed memory. You probably get away with this in release, but in debug (under windows) it will write freed memory markers as guards for this type of thing.

The code currently is:

std::vector<std::string> display;
load(second, display);
const char* displays = JoinStringEnvStyle(display).c_str();
c->setActiveDisplays(displays);

You can easily see the scope issue here.
I suspect a fix would be along the lines of:

std::vector<std::string> display;
load(second, display);
std::string strDisplays = JoinStringEnvStyle(display).c_str();
const char* displays = strDisplays.c_str();
c->setActiveDisplays(displays);

I thought I'd pass this on as I've not seen any notes on it anywhere, but it must be an issue for everyone.


 - Simon.

--
You received this message because you are subscribed to the Google Groups "OpenColorIO Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to ocio-dev+u...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Hi Mark,

I'm using Visual Studio 2012, 64-bit compilation in debug.
If you view the disassembly of OCIOYaml.cpp in the load function  (or set a breakpoint in ~basic_string) you can see where it's destroyed as follows:

                {

                    std::vector<std::string> display;

00007FFDA504E132  lea         rcx,[rsp+0C28h] 
00007FFDA504E13A  call        std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > >::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > (07FFDA4F35CAEh) 
00007FFDA504E13F  nop 
                    load(second, display);
00007FFDA504E140  lea         rdx,[rsp+0C28h] 
00007FFDA504E148  mov         rcx,qword ptr [rsp+3B0h] 
00007FFDA504E150  call        OpenColorIO::v1::`anonymous namespace'::load (07FFDA4F357D6h) 

                    const char* displays = JoinStringEnvStyle(display).c_str();

00007FFDA504E155  lea         rdx,[rsp+0C28h] 
00007FFDA504E15D  lea         rcx,[rsp+0C60h] 
00007FFDA504E165  call        OpenColorIO::v1::JoinStringEnvStyle (07FFDA4F331BBh) 
00007FFDA504E16A  mov         qword ptr [rsp+1898h],rax 
00007FFDA504E172  mov         rcx,qword ptr [rsp+1898h] 
00007FFDA504E17A  call        std::basic_string<char,std::char_traits<char>,std::allocator<char> >::c_str (07FFDA4F35EB6h) 
00007FFDA504E17F  mov         qword ptr [rsp+0C58h],rax 
00007FFDA504E187  lea         rcx,[rsp+0C60h] 
00007FFDA504E18F  call        std::basic_string<char,std::char_traits<char>,std::allocator<char> >::~basic_string<char,std::char_traits<char>,std::allocator<char> > (07FFDA4F36F82h) 
                    c->setActiveDisplays(displays);
00007FFDA504E194  mov         rcx,qword ptr [c] 
00007FFDA504E19C  call        boost::shared_ptr<OpenColorIO::v1::Config>::operator-> (07FFDA4F33972h) 
00007FFDA504E1A1  mov         rdx,qword ptr [rsp+0C58h] 
00007FFDA504E1A9  mov         rcx,rax 
00007FFDA504E1AC  call        OpenColorIO::v1::Config::setActiveDisplays (07FFDA4F33323h) 

Hopefully this displays OK for you.
You can see just before the c->setActiveDisplays(displays) the string destructor fires.
So the pointer passed to setActiveDisplay is thus now pointing to freed memory ....

This kind of makes sense - the scope is just for the return value as the object is not assigned to anything (the *contents* are, the c_str, but std::string doesn't reference count that!) but I suspect some compilers aren't so eager to destroy the returned string as Visual Studio 2012 ....


Simon.

On Wednesday, July 23, 2014 12:55:30 AM UTC+1, Mark Boorer wrote:

Hi Simon,

Not sure I see where a bug could be hiding. The scoping around those blocks seems fine to me.
setActiveDisplays(), though taking a const char * as an argument, never actually stores it internally, instead opting to store the results of another call to SplitStringEnvStyle().

SplitStringEnvStyle() makes a new std::string via pystring::strip as one of its first operations, so there is no possibility of char pointers falling out of scope.

This has been fairly stable code (since around 2011), though admittedly OCIOYaml.cpp has recently undergone some changes.

Would you happen to have some more information as to your development environment? Visual Studio / OCIO versions / ways to reproduce the crash you're seeing?

With a little more information I'll hopefully be able to root out the cause of the issue.

Cheers,
Mark

On Tue, Jul 22, 2014 at 4:03 PM, simon smith <si.c...@...> wrote:

I'm testing out some code and I've noticed a crash issue when a config (such as ACES, from the ICIO website) contain multiple active displays or views.

Basically, the code in ICIOYaml.cpp that takes an array of displays tries to join them together into one string using the JoinStringEnvStyle function (in ParseUtils.cpp) is flawed.

JoinStringEnvStyle will return a new std::string instance if more than one string is found in the array (it will pass the original "array" string back if there is only one entry in it).

The caller of JoinStringEnvStylein OCIOYaml.cpp takes a const pointer to this returned std::string to pass to the next function (setActiveDisplays) but it falls out of scope as soon as the const char* has been taken (it goes out of scope as it enters the next line of code). Thus on the next line the pointer is pointing to freed memory. You probably get away with this in release, but in debug (under windows) it will write freed memory markers as guards for this type of thing.

The code currently is:

std::vector<std::string> display;
load(second, display);
const char* displays = JoinStringEnvStyle(display).c_str();
c->setActiveDisplays(displays);

You can easily see the scope issue here.
I suspect a fix would be along the lines of:

std::vector<std::string> display;
load(second, display);
std::string strDisplays = JoinStringEnvStyle(display).c_str();
const char* displays = strDisplays.c_str();
c->setActiveDisplays(displays);

I thought I'd pass this on as I've not seen any notes on it anywhere, but it must be an issue for everyone.


 - Simon.

--
You received this message because you are subscribed to the Google Groups "OpenColorIO Developers" group.

To unsubscribe from this group and stop receiving emails from it, send an email to ocio-dev+u...@googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "OpenColorIO Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to ocio-dev+u...@....
For more options, visit https://groups.google.com/d/optout.

Hi, Just following up.

This issue should now be resolved. I have patched OCIOYaml.cpp, and pushed those changes to master. If you are able, could you confirm that this solves your issue?

Cheers,
Mark

On Thu, Jul 24, 2014 at 11:47 AM, Mark Boorer <mar...@...> wrote:

Ah right, now I see where the problem is!

Thanks for the breakdown, I've opened an issue on the github bug tracker, and I'll look to patching this tonight.

https://github.com/imageworks/OpenColorIO/issues/365

There are a couple of other places where this happens too

Thanks again!

Cheers,
Mark

On Thu, Jul 24, 2014 at 8:51 AM, simon smith <si.c...@...> wrote:

Hi Mark,

I'm using Visual Studio 2012, 64-bit compilation in debug.
If you view the disassembly of OCIOYaml.cpp in the load function  (or set a breakpoint in ~basic_string) you can see where it's destroyed as follows:

                {

                    std::vector<std::string> display;

00007FFDA504E132  lea         rcx,[rsp+0C28h] 
00007FFDA504E13A  call        std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > >::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > (07FFDA4F35CAEh) 
00007FFDA504E13F  nop 
                    load(second, display);
00007FFDA504E140  lea         rdx,[rsp+0C28h] 
00007FFDA504E148  mov         rcx,qword ptr [rsp+3B0h] 
00007FFDA504E150  call        OpenColorIO::v1::`anonymous namespace'::load (07FFDA4F357D6h) 

                    const char* displays = JoinStringEnvStyle(display).c_str();

00007FFDA504E155  lea         rdx,[rsp+0C28h] 
00007FFDA504E15D  lea         rcx,[rsp+0C60h] 
00007FFDA504E165  call        OpenColorIO::v1::JoinStringEnvStyle (07FFDA4F331BBh) 
00007FFDA504E16A  mov         qword ptr [rsp+1898h],rax 
00007FFDA504E172  mov         rcx,qword ptr [rsp+1898h] 
00007FFDA504E17A  call        std::basic_string<char,std::char_traits<char>,std::allocator<char> >::c_str (07FFDA4F35EB6h) 
00007FFDA504E17F  mov         qword ptr [rsp+0C58h],rax 
00007FFDA504E187  lea         rcx,[rsp+0C60h] 
00007FFDA504E18F  call        std::basic_string<char,std::char_traits<char>,std::allocator<char> >::~basic_string<char,std::char_traits<char>,std::allocator<char> > (07FFDA4F36F82h) 
                    c->setActiveDisplays(displays);
00007FFDA504E194  mov         rcx,qword ptr [c] 
00007FFDA504E19C  call        boost::shared_ptr<OpenColorIO::v1::Config>::operator-> (07FFDA4F33972h) 
00007FFDA504E1A1  mov         rdx,qword ptr [rsp+0C58h] 
00007FFDA504E1A9  mov         rcx,rax 
00007FFDA504E1AC  call        OpenColorIO::v1::Config::setActiveDisplays (07FFDA4F33323h) 

Hopefully this displays OK for you.
You can see just before the c->setActiveDisplays(displays) the string destructor fires.
So the pointer passed to setActiveDisplay is thus now pointing to freed memory ....

This kind of makes sense - the scope is just for the return value as the object is not assigned to anything (the *contents* are, the c_str, but std::string doesn't reference count that!) but I suspect some compilers aren't so eager to destroy the returned string as Visual Studio 2012 ....


Simon.

On Wednesday, July 23, 2014 12:55:30 AM UTC+1, Mark Boorer wrote:

Hi Simon,

Not sure I see where a bug could be hiding. The scoping around those blocks seems fine to me.
setActiveDisplays(), though taking a const char * as an argument, never actually stores it internally, instead opting to store the results of another call to SplitStringEnvStyle().

SplitStringEnvStyle() makes a new std::string via pystring::strip as one of its first operations, so there is no possibility of char pointers falling out of scope.

This has been fairly stable code (since around 2011), though admittedly OCIOYaml.cpp has recently undergone some changes.

Would you happen to have some more information as to your development environment? Visual Studio / OCIO versions / ways to reproduce the crash you're seeing?

With a little more information I'll hopefully be able to root out the cause of the issue.

Cheers,
Mark

On Tue, Jul 22, 2014 at 4:03 PM, simon smith <si.c...@...> wrote:

I'm testing out some code and I've noticed a crash issue when a config (such as ACES, from the ICIO website) contain multiple active displays or views.

Basically, the code in ICIOYaml.cpp that takes an array of displays tries to join them together into one string using the JoinStringEnvStyle function (in ParseUtils.cpp) is flawed.

JoinStringEnvStyle will return a new std::string instance if more than one string is found in the array (it will pass the original "array" string back if there is only one entry in it).

The caller of JoinStringEnvStylein OCIOYaml.cpp takes a const pointer to this returned std::string to pass to the next function (setActiveDisplays) but it falls out of scope as soon as the const char* has been taken (it goes out of scope as it enters the next line of code). Thus on the next line the pointer is pointing to freed memory. You probably get away with this in release, but in debug (under windows) it will write freed memory markers as guards for this type of thing.

The code currently is:

std::vector<std::string> display;
load(second, display);
const char* displays = JoinStringEnvStyle(display).c_str();
c->setActiveDisplays(displays);

You can easily see the scope issue here.
I suspect a fix would be along the lines of:

std::vector<std::string> display;
load(second, display);
std::string strDisplays = JoinStringEnvStyle(display).c_str();
const char* displays = strDisplays.c_str();
c->setActiveDisplays(displays);

I thought I'd pass this on as I've not seen any notes on it anywhere, but it must be an issue for everyone.


 - Simon.

--
You received this message because you are subscribed to the Google Groups "OpenColorIO Developers" group.

To unsubscribe from this group and stop receiving emails from it, send an email to ocio-dev+u...@googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "OpenColorIO Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to ocio-dev+u...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

I can't check that immediately, but when I have got the time to check it out I shall report back.

Thanks for the help Mark.


  - Simon.

On Friday, July 25, 2014 12:33:23 AM UTC+1, Mark Boorer wrote:

Hi, Just following up.

This issue should now be resolved. I have patched OCIOYaml.cpp, and pushed those changes to master. If you are able, could you confirm that this solves your issue?

Cheers,
Mark

On Thu, Jul 24, 2014 at 11:47 AM, Mark Boorer <mar...@...> wrote:

Ah right, now I see where the problem is!

Thanks for the breakdown, I've opened an issue on the github bug tracker, and I'll look to patching this tonight.

https://github.com/imageworks/OpenColorIO/issues/365

There are a couple of other places where this happens too

Thanks again!

Cheers,
Mark

On Thu, Jul 24, 2014 at 8:51 AM, simon smith <si.c...@...> wrote:

Hi Mark,

I'm using Visual Studio 2012, 64-bit compilation in debug.
If you view the disassembly of OCIOYaml.cpp in the load function  (or set a breakpoint in ~basic_string) you can see where it's destroyed as follows:

                {

                    std::vector<std::string> display;

00007FFDA504E132  lea         rcx,[rsp+0C28h] 
00007FFDA504E13A  call        std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > >::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > (07FFDA4F35CAEh) 
00007FFDA504E13F  nop 
                    load(second, display);
00007FFDA504E140  lea         rdx,[rsp+0C28h] 
00007FFDA504E148  mov         rcx,qword ptr [rsp+3B0h] 
00007FFDA504E150  call        OpenColorIO::v1::`anonymous namespace'::load (07FFDA4F357D6h) 

                    const char* displays = JoinStringEnvStyle(display).c_str();

00007FFDA504E155  lea         rdx,[rsp+0C28h] 
00007FFDA504E15D  lea         rcx,[rsp+0C60h] 
00007FFDA504E165  call        OpenColorIO::v1::JoinStringEnvStyle (07FFDA4F331BBh) 
00007FFDA504E16A  mov         qword ptr [rsp+1898h],rax 
00007FFDA504E172  mov         rcx,qword ptr [rsp+1898h] 
00007FFDA504E17A  call        std::basic_string<char,std::char_traits<char>,std::allocator<char> >::c_str (07FFDA4F35EB6h) 
00007FFDA504E17F  mov         qword ptr [rsp+0C58h],rax 
00007FFDA504E187  lea         rcx,[rsp+0C60h] 
00007FFDA504E18F  call        std::basic_string<char,std::char_traits<char>,std::allocator<char> >::~basic_string<char,std::char_traits<char>,std::allocator<char> > (07FFDA4F36F82h) 
                    c->setActiveDisplays(displays);
00007FFDA504E194  mov         rcx,qword ptr [c] 
00007FFDA504E19C  call        boost::shared_ptr<OpenColorIO::v1::Config>::operator-> (07FFDA4F33972h) 
00007FFDA504E1A1  mov         rdx,qword ptr [rsp+0C58h] 
00007FFDA504E1A9  mov         rcx,rax 
00007FFDA504E1AC  call        OpenColorIO::v1::Config::setActiveDisplays (07FFDA4F33323h) 

Hopefully this displays OK for you.
You can see just before the c->setActiveDisplays(displays) the string destructor fires.
So the pointer passed to setActiveDisplay is thus now pointing to freed memory ....

This kind of makes sense - the scope is just for the return value as the object is not assigned to anything (the *contents* are, the c_str, but std::string doesn't reference count that!) but I suspect some compilers aren't so eager to destroy the returned string as Visual Studio 2012 ....


Simon.

On Wednesday, July 23, 2014 12:55:30 AM UTC+1, Mark Boorer wrote:

Hi Simon,

Not sure I see where a bug could be hiding. The scoping around those blocks seems fine to me.
setActiveDisplays(), though taking a const char * as an argument, never actually stores it internally, instead opting to store the results of another call to SplitStringEnvStyle().

SplitStringEnvStyle() makes a new std::string via pystring::strip as one of its first operations, so there is no possibility of char pointers falling out of scope.

This has been fairly stable code (since around 2011), though admittedly OCIOYaml.cpp has recently undergone some changes.

Would you happen to have some more information as to your development environment? Visual Studio / OCIO versions / ways to reproduce the crash you're seeing?

With a little more information I'll hopefully be able to root out the cause of the issue.

Cheers,
Mark

On Tue, Jul 22, 2014 at 4:03 PM, simon smith <si.c...@...> wrote:

I'm testing out some code and I've noticed a crash issue when a config (such as ACES, from the ICIO website) contain multiple active displays or views.

Basically, the code in ICIOYaml.cpp that takes an array of displays tries to join them together into one string using the JoinStringEnvStyle function (in ParseUtils.cpp) is flawed.

JoinStringEnvStyle will return a new std::string instance if more than one string is found in the array (it will pass the original "array" string back if there is only one entry in it).

The caller of JoinStringEnvStylein OCIOYaml.cpp takes a const pointer to this returned std::string to pass to the next function (setActiveDisplays) but it falls out of scope as soon as the const char* has been taken (it goes out of scope as it enters the next line of code). Thus on the next line the pointer is pointing to freed memory. You probably get away with this in release, but in debug (under windows) it will write freed memory markers as guards for this type of thing.

The code currently is:

std::vector<std::string> display;
load(second, display);
const char* displays = JoinStringEnvStyle(display).c_str();
c->setActiveDisplays(displays);

You can easily see the scope issue here.
I suspect a fix would be along the lines of:

std::vector<std::string> display;
load(second, display);
std::string strDisplays = JoinStringEnvStyle(display).c_str();
const char* displays = strDisplays.c_str();
c->setActiveDisplays(displays);

I thought I'd pass this on as I've not seen any notes on it anywhere, but it must be an issue for everyone.


 - Simon.

--
You received this message because you are subscribed to the Google Groups "OpenColorIO Developers" group.

To unsubscribe from this group and stop receiving emails from it, send an email to ocio-dev+u...@googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "OpenColorIO Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to ocio-d...@....
For more options, visit https://groups.google.com/d/optout.