mainCompact Disc Input and Control Library - Support: sr #108751, Stack Overflow in cdio_logv

 
 

sr #108751: Stack Overflow in cdio_logv

Submitter:  None
Submitted:  Thu 19 Feb 2015 09:14:46 PM UTC
   
 
Category:  None Priority:  3 - Low
Severity:  2 - Minor Status:  Invalid
Privacy:  Public Assigned to:  None
Originator Email:  -email is unavailable- Open/Closed:  Closed
Operating System:  GNU/Linux
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 28 May 2015 07:43:02 PM UTC, comment #15: 

Kodi should use libcdio git. Most of the git changes are various bug fixes.

At this stage for libcdio, releases are about once a year in the fall.

Rocky Bernstein <rocky>
Group administrator
Thu 28 May 2015 07:22:45 PM UTC, comment #14: 

Hi,
this new version works fine.

Thank you very much!

Are you delivering it? Or do you think Kodi people should use libcdio git?

Claudio

Anonymous
Wed 27 May 2015 08:17:22 AM UTC, comment #13: 

git clone git://git.savannah.gnu.org/libcdio.git

Rocky Bernstein <rocky>
Group administrator
Wed 27 May 2015 08:13:08 AM UTC, comment #12: 

Thanks,
where can I fetch the new code?
It will take a couple of days before I manage to get it integrated in Kodi and run tests.

Anonymous
Wed 27 May 2015 07:30:14 AM UTC, comment #11: 


> 2. The second trace occurs, it goes to cdio_assert_not_reached() that calls again cdio_logv() and loops


This no longer happens. In commit 8fcd4ce909f4f04e77bee78f573a241d0e9f2c34 assert() rather than cdio_assert_not_reached() is called. Also as I wrote before:

> I've reordered loglevel tests so that log messages that won't appear are "handled" (that is nothing is done) without the recursion check. See if that addresses the specific situation that you encounter.


Did you try this? Does it address the specific situation you encounter?

Ensuring I/O is re-entrant is a good idea. But vsnprintf_r doesn't exist on GNU Linux and googling around it mostly doesn't exist anywhere else. cygwin has a private _vsnprintf_r and there is no guarantee it is reentrant. See http://codedbot.com/questions/5302534/how-do-i-use-vsnprintf-r-correctly.

As I wrote before, I don't have a good solution. But adding a timed wait also doesn't feel like a good solution either. I suspect the solution would be to completely revise this so that logging is part of the cdio object rather than exposed as a top-level function. That way, multi-threaded programs could replace the whole lot with a version that has mutex's.  This would be an incompatible change and more work than I am prepared to do. (But of course I invite others to do so.)

Rocky Bernstein <rocky>
Group administrator
Wed 27 May 2015 06:19:07 AM UTC, comment #10: 

I was thinking about and I think I understand hvr point of view.
vsnprintf() is not reentrant, thus if you call it recursively it will mess up internal buffers and pheraps make the system crash.
On the other hand, the current code as it's written only works if the log function is quick enough so that the following happens:
1. The first trace occurs, it sets in_recursion=1
2. The second trace occurs, it goes to cdio_assert_not_reached() that calls again cdio_logv() and loops
3. The first call completes logging and sets in_recursion=0
4. The loops stops because in_recursion=0
But if the stack is not big enough, it will crash first.

I'd say that a timer should be inserted to grant that the second, and following calls to cdio_logv() will wait for the first to be completed.

Otherwise a good alternative is to replace vsnprintf() with vsnprintf_r() that is reentrant.

What do you thing about?

Anonymous
Tue 26 May 2015 01:17:15 PM UTC, comment #9: 


> why libcdio contains the check for recursion in cdio_logv()?


hvr put this and things like this in as safety checks and I think they are good to have. _handler() can be user-defined and I suppose he's trying to guard against the handler calling a logging function. Ironic because he made exactly the mistake that he was trying to guard against in user code.

If there is multi-threaded code and each thread produces an error, then the recursion checking will get in the way. But without that it is also possible that error messages will get intermingled and there's the original problem hvr was guarding against. So I am not sure what to do.

I am not comfortable with removing the recursion checking altogether. As something to ameliorate the problem, I've reordered loglevel tests so that log messages that won't appear are "handled" (that is nothing is done) without the recursion check. See if that addresses the specific situation that you encounter.

As for the patch. While it is perfectly fine for you to use this for your purposes, the code is after all open-source and FSF, a project like this can't incorporate it if it doesn't address the root cause which you aren't even sure about.

Rocky Bernstein <rocky>
Group administrator
Tue 26 May 2015 12:02:31 PM UTC, comment #8: 

Hi,
I checked libcdio and how it's used in Kodi.
Actually the wrapper in Kodi produces no code in the production binary, it only produces log in debug binary, so I went back to the initial problem.
I have one big question: why libcdio contains the check for recursion in cdio_logv()?
Kodi is multithread, and it could be that cdio_logv() is executed cuncurrently, but is it a real problem?
If I use libcdio as it is, then it seldom crashes inside Kodi, causing Kodi to crash, but if I patch it then it works fine.

Is there any reason, coming from real use case, for having such
 if (in_recursion)
   cdio_assert_not_reached ();

???

Anonymous
Mon 25 May 2015 01:17:20 PM UTC, comment #7: 


> initialize va_list before using it,


lib/driver/logging.c has this:

> static void
> cdio_logv(cdio_log_level_t level, const char format[], va_list args) ...


So va_list is passed in as a parameter. If it is not initialized then that is because it isn't initialized in the caller. I have no idea whether the caller is in libcdio or not, but all of the uses inside logging.c look like this:

>
>  va_list args;
>  va_start (args, format);
>  cdio_logv (... format, args);


which should initialize va_list.

Given this, work with kodi folks and if they think there's a problem in libcdio, have them come up with a standalone example program.

Rocky Bernstein <rocky>
Group administrator
Mon 25 May 2015 01:00:55 PM UTC, comment #6: 

I am sorry,
I cannot write a simple code, because the fault doesn't happen with simple code, it happens with Kodi that's a rather complicated multithread code.
I'm not a Kodi developer, and I had hard time even compiling Kodi by carefully following the instruction.
At the end, I can only propose other Kodi developer to introduce my changes, and wait them for producing a binary.

About libcdio problem, I managed to understand that there was a problem in the logging mechanism, and that ugly patch makes it work.

I made a search for what can be the reason, and I have seen that Kodi designers have interfaced libcdio properly, then I digged into libcdio logging function.
There's something that is not according to the rules, and this is the use of vsnprintf in cdio_logv()
According to the documentation, you need to initialize va_list before using it, please have a look at
https://sourceware.org/newlib/libc.html
and
http://c.happycodings.com/gnu-linux/code28.html

I can ask Kodi team to patch, and can run test on the Raspberry.

Anonymous
Mon 25 May 2015 11:21:25 AM UTC, comment #5: 

Your inability or unwillingness to follow a simple request is frustrating.

Come up with an small standalone example that demonstrates the problem. If you can't or won't I am tempted to close this until we can find someone who can.

Rocky Bernstein <rocky>
Group administrator
Mon 25 May 2015 08:10:44 AM UTC, comment #4: 

What I see from this fragment, and that sounds odd to me, is that Kodi has wrapped the logging by means of
xbox_cdio_log_handler (cdio_log_level_t level, const char message[])
this would require that libcdio uses xbox_cdio_log_handler() rather than its own logger.
What I see instead is that libcdio keeps on using cdio_logv(),
and I suppose this is due to a bad interwork amonf Kodi and libcdio.

There are no problems in the x86 version of Kodi, that exploits libcdio 0.83
The libcdio version used in Kodi/Openelec/Raspberry is 0.92 and this has the problem.

Any guess?

Anonymous
Sun 24 May 2015 03:02:18 PM UTC, comment #3: 

No, this is not enough for me to understand what's going on. If it is enough for you, then write a small standalone C program. Thanks.

Rocky Bernstein <rocky>
Group administrator
Fri 22 May 2015 07:22:57 AM UTC, comment #2: 

Maybe it's just enough looking at the C++ wrapper used in Kodi,
that's most probably the source of the problem
https://github.com/xbmc/xbmc/blob/master/xbmc/storage/cdioSupport.cpp
https://github.com/xbmc/xbmc/blob/master/xbmc/storage/cdioSupport.h

I hope it helps

/Claudio

Anonymous
Fri 08 May 2015 12:15:29 AM UTC, comment #1: 

What would be really helpful is a small standalone C program such as are found in examples directory that shows the problem.

For example you could start with example/logging.c remove the custom logging that it has and just issue a command that you know will trigger a normal event log.

This will help me understand the problem. It will also help to create a test case so that we can be sure the problem doesn't occur again.

Thanks.

Rocky Bernstein <rocky>
Group administrator
Thu 19 Feb 2015 09:14:46 PM UTC, original submission:  

Libcdio v0.92 is used in Openelec, it's linked in kodi.
The HW used is Raspberry Pi.
The logging level is set to CDIO_LOG_ERROR in file /lib/driver/logging.c

When libcdio tries to log a normal event, it should be ignored with that logging level, instead it triggers cdio_logv, here for unknown reasons the static variable in_recursion is 1, this causes cdio_logv and cdio_log to call each the others forever.

Details and attached files are available at ​http://forum.kodi.tv/showthread.php?tid=216315

In order to get rid of the problem, the following temporary change has been applied to cdio_logv.

static void
cdio_logv(cdio_log_level_t level, const char format[], va_list args)
{
  char buf[1024] = { 0, };
  static int in_recursion = 0;

// Patched by Claudio
//  if (in_recursion)
//    cdio_assert_not_reached ();
// End of patch

  in_recursion = 1;

  vsnprintf(buf, sizeof(buf)-1, format, args);

  _handler(level, buf);

  in_recursion = 0;
}

Anonymous

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rocky (Posted a comment)
  • -email is unavailable- added by None (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2015-05-25 rocky StatusNone Invalid
    2015-05-25 rocky Open/ClosedOpen Closed
    2015-05-25 rocky Priority5 - Normal 3 - Low
        Severity3 - Normal 2 - Minor

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code