Opened 3 years ago

Closed 3 years ago

Last modified 19 months ago

#814 closed enhancement (invalid)

sem_setup should use SEM_UNDO flag

Reported by: Darac Owned by: nobody
Priority: low Milestone:
Component: master Version: 1.4.2
Severity: normal Keywords:
Cc:

Description

The sem_setup function (in munin-cgi-graph and munin-fastcgi-graph) should use the SEM_UNDO flag in the call to semop to avoid overflowing the semaphore value.

man 2 semop states that the behaviour of semop() with a positive sem_op is to increment the semaphore value by this amount. The sem_*() functions use the value in the semaphore as a 'number of available jobs', decrementing the count before calling munin-graph and incrementing it afterwards. If sem_setup is called several times (as would happen with munin-cgi-graph), then the semaphore value is incremented by $max_cgi_graph_jobs each time. If the semaphore value would exceed SEMVMX, an error is returned and perl dies.

If we add SEM_UNDO, though, the amount we added to the semaphore in sem_setup is automatically removed when the process finishes.

The attached patch fixes this for 1.4.2

Attachments (1)

munin.patch (892 bytes) - added by Darac 3 years ago.
Add SEM_UNDO flag (0x1000) to the semop call

Download all attachments as: .zip

Change History (4)

Changed 3 years ago by Darac

Add SEM_UNDO flag (0x1000) to the semop call

comment:1 Changed 3 years ago by janl

  • Resolution set to fixed
  • Status changed from new to closed

munin-cgi-graph does not do a exec only, the preceding open is a implicit fork so the code after the exec is executed in the parent. But the undo is handy any way to make sure the process is cleaned up after it dies. Thanks. In r3251.

comment:2 Changed 3 years ago by janl

  • Component changed from web-interface to master
  • Milestone Munin 1.5 deleted
  • Resolution fixed deleted
  • Status changed from closed to reopened

The patch is wrong. Whatever you're trying to fix is broken some other way.

According to google SEM_UNDO is not very portable.

But most of all: We MUST NOT undo the semaphore initialization, it would leave the semaphore with a 0 value at process exit which is not what is needed.

Reverted the patch. If you still think it is correct please reopen the ticket and provide a better explanation.

comment:3 Changed 3 years ago by janl

  • Resolution set to invalid
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.