NotificationsYou must be signed in to change notification settings
Fork6
Star31

Commitd4a43b2

committed

Convert libpgport's pqsignal() to a void function.

The protections added by commit3b00fdb introduced raceconditions to this function that can lead to bogus return values.Since nobody seems to inspect the return value, this is of littleconsequence, but it would have been nice to convert it to a voidfunction to avoid any possibility of a bogus return value. Ioriginally thought that doing so would have required also modifyinglegacy-pqsignal.c's version of the function (which would'verequired an SONAME bump), but commit9a45a89 gavelegacy-pqsignal.c its own dedicated extern for pqsignal(), therebydecoupling it enough that libpgport's pqsignal() can be modified.This commit also adds an assertion for the return value ofsigaction()/signal(). Since a failure most likely indicates acoding error, and nobody has ever bothered to check pqsignal()'sreturn value, it's probably not worth the effort to do anythingfancier.Reviewed-by: Tom LaneDiscussion:https://postgr.es/m/Z4chOKfnthRH71mw%40nathan

1 parent5cda4fd commitd4a43b2Copy full SHA for d4a43b2

File tree

2 files changed

-29

lines changed

src
- include
  - port.h
- port
  - pqsignal.c

2 files changed

-29

lines changed

`‎src/include/port.h`

Lines changed: 1 addition & 1 deletion

Original file line number	Diff line number	Diff line change
`@@ -520,7 +520,7 @@ extern intpg_mkdir_p(char *path, int omode);`
`520`	`520`	`#definepqsignal pqsignal_be`
`521`	`521`	`#endif`
`522`	`522`	`typedefvoid (*pqsigfunc) (SIGNAL_ARGS);`
`523`		`-externpqsigfuncpqsignal(intsigno,pqsigfuncfunc);`
	`523`	`+externvoidpqsignal(intsigno,pqsigfuncfunc);`
`524`	`524`
`525`	`525`	`/* port/quotes.c */`
`526`	`526`	`externcharescape_single_quotes_ascii(constcharsrc);`

`‎src/port/pqsignal.c`

Lines changed: 6 additions & 28 deletions

Original file line number	Diff line number	Diff line change
`@@ -112,31 +112,15 @@ wrapper_handler(SIGNAL_ARGS)`
`112`	`112`	`/*`
`113`	`113`	`* Set up a signal handler, with SA_RESTART, for signal "signo"`
`114`	`114`	`*`
`115`		`- * Returns the previous handler.`
`116`		`- *`
`117`		`- * NB: If called within a signal handler, race conditions may lead to bogus`
`118`		`- * return values. You should either avoid calling this within signal handlers`
`119`		`- * or ignore the return value.`
`120`		`- *`
`121`		`- * XXX: Since no in-tree callers use the return value, and there is little`
`122`		`- * reason to do so, it would be nice if we could convert this to a void`
`123`		`- * function instead of providing potentially-bogus return values.`
`124`		`- * Unfortunately, that requires modifying the pqsignal() in legacy-pqsignal.c,`
`125`		`- * which in turn requires an SONAME bump, which is probably not worth it.`
`126`		`- *`
`127`	`115`	`* Note: the actual name of this function is either pqsignal_fe when`
`128`	`116`	`* compiled with -DFRONTEND, or pqsignal_be when compiled without that.`
`129`	`117`	`* This is to avoid a name collision with libpq's legacy-pqsignal.c.`
`130`	`118`	`*/`
`131`		`-pqsigfunc`
	`119`	`+void`
`132`	`120`	`pqsignal(intsigno,pqsigfuncfunc)`
`133`	`121`	`{`
`134`		`-pqsigfuncorig_func=pqsignal_handlers[signo];/* assumed atomic */`
`135`	`122`	`#if !(defined(WIN32)&& defined(FRONTEND))`
`136`		`-structsigactionact,`
`137`		`-oact;`
`138`		`-#else`
`139`		`-pqsigfuncret;`
	`123`	`+structsigactionact;`
`140`	`124`	`#endif`
`141`	`125`
`142`	`126`	`Assert(signo<PG_NSIG);`
`@@ -155,17 +139,11 @@ pqsignal(int signo, pqsigfunc func)`
`155`	`139`	`if (signo==SIGCHLD)`
`156`	`140`	`act.sa_flags \|=SA_NOCLDSTOP;`
`157`	`141`	`#endif`
`158`		`-if (sigaction(signo,&act,&oact)<0)`
`159`		`-returnSIG_ERR;`
`160`		`-elseif (oact.sa_handler==wrapper_handler)`
`161`		`-returnorig_func;`
`162`		`-else`
`163`		`-returnoact.sa_handler;`
	`142`	`+if (sigaction(signo,&act,NULL)<0)`
	`143`	`+Assert(false);/* probably indicates coding error */`
`164`	`144`	`#else`
`165`	`145`	`/* Forward to Windows native signal system. */`
`166`		`-if ((ret=signal(signo,func))==wrapper_handler)`
`167`		`-returnorig_func;`
`168`		`-else`
`169`		`-returnret;`
	`146`	`+if (signal(signo,func)==SIG_ERR)`
	`147`	`+Assert(false);/* probably indicates coding error */`
`170`	`148`	`#endif`
`171`	`149`	`}`

0 commit comments

Comments

(0)

Movatterモバイル変換

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Commitd4a43b2

File tree

2 files changed

2 files changed

`‎src/include/port.h`

`‎src/port/pqsignal.c`

0 commit comments