libinotifytools: Rename init variable to fix conflict with entry point by Qeole · Pull Request #170 · inotify-tools/inotify-tools

Qeole · 2022-10-05T23:29:40Z

Running inotifywait 3.22.1.0 on Ubuntu 22.04, I observed a bug apparently due to a bad file descriptor:

$ inotifywait -e modify foo
Setting up watches.
Couldn't watch foo: Bad file descriptor

I could also reproduce with the master branch. Investigating further, it appears that inotifytools_init() in inotifytools.c exits immediately after entry, because the global variable init is non-null, even though we just called it at the beginning of inotifywait.c's main(), and the global variable is supposed to be initialised at 0.

Bisecting showed that this variable used to be static, but was changed at some point to non-static to share it with stats.c, and this is where the bug first occurs. As far as I understand, the symbol conflicts with the init() function from the program's entry point. When entering inotifytools_init(), it seems that the value for that function is picked up and compared to 0, found non-null, and the initialization function exits, leaving inotify_fd uninitialized (-1) and a bad file descriptor.

Renaming the variable to something other than init is enough to fix the bug.

$ ./src/inotifywait -e modify foo
Setting up watches.
Watches established
<waits>

Fixes: dd59c53 ("Extract stats into their own file (#129)")

ericcurtin · 2022-10-06T09:40:40Z

Very interesting this conflicts with the programs entry point, consider this merged if it passes the build and thanks for pushing this commit.

codecov · 2022-10-06T10:22:42Z

Codecov Report

Merging #170 (dbdabc8) into master (37916c2) will decrease coverage by 0.16%.
The diff coverage is 86.66%.

@@            Coverage Diff             @@
##           master     #170      +/-   ##
==========================================
- Coverage   44.76%   44.59%   -0.17%     
==========================================
  Files           7        7              
  Lines        2377     2379       +2     
==========================================
- Hits         1064     1061       -3     
- Misses       1313     1318       +5

Impacted Files	Coverage Δ
libinotifytools/src/inotifytools.c	`49.82% <85.71%> (-0.35%)`	⬇️
libinotifytools/src/stats.c	`11.92% <100.00%> (ø)`
libinotifytools/src/redblack.c	`65.97% <0.00%> (-0.35%)`	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Qeole · 2022-10-06T10:36:38Z

I see one failure is triggered by clang-format. Launching the tool locally, I get many suggested changes on the files, most unrelated to the PR. There are a few that I could squash to the commit, depending on your preferences:

Suggested formatting changes

diff --git a/libinotifytools/src/inotifytools.c b/libinotifytools/src/inotifytools.c
index a4106cb7a540..873de650fe81 100644
--- a/libinotifytools/src/inotifytools.c
+++ b/libinotifytools/src/inotifytools.c
@@ -299,7 +299,8 @@ watch *watch_from_filename( char const *filename ) {
  *         obtained from inotifytools_error().
  */
 int inotifytools_init(int fanotify, int watch_filesystem, int verbose) {
-	if (initialized) return 1;
+	if (initialized)
+		return 1;
 
 	error = 0;
 	verbosity = verbose;
@@ -366,7 +367,8 @@ void cleanup_tree(const void *nodep,
  * again before any other functions can be used.
  */
 void inotifytools_cleanup() {
-	if (!initialized) return;
+	if (!initialized)
+		return;
 
 	initialized = 0;
 	close(inotify_fd);
@@ -875,7 +877,7 @@ const char* inotifytools_filename_from_watch(watch* w) {
  *       filename returned will still be the original name.
  */
 const char* inotifytools_filename_from_wd(int wd) {
-	niceassert( initialized, "inotifytools_initialize not called yet" );
+	niceassert(initialized, "inotifytools_initialize not called yet");
 	if (!wd)
 		return "";
 	watch *w = watch_from_wd(wd);
@@ -980,7 +982,7 @@ char* inotifytools_dirpath_from_event(struct inotify_event* event) {
  *       establish the watch.
  */
 int inotifytools_wd_from_filename( char const * filename ) {
-	niceassert( initialized, "inotifytools_initialize not called yet" );
+	niceassert(initialized, "inotifytools_initialize not called yet");
 	if (!filename || !*filename)
 		return -1;
 	watch *w = watch_from_filename(filename);
@@ -1003,7 +1005,7 @@ int inotifytools_wd_from_filename( char const * filename ) {
  * @param filename New filename.
  */
 void inotifytools_set_filename_by_wd( int wd, char const * filename ) {
-	niceassert( initialized, "inotifytools_initialize not called yet" );
+	niceassert(initialized, "inotifytools_initialize not called yet");
 	watch *w = watch_from_wd(wd);
 	if (!w) return;
 	if (w->filename) free(w->filename);
@@ -1125,7 +1127,7 @@ watch* create_watch(int wd,
  *         obtained from inotifytools_error().
  */
 int inotifytools_remove_watch_by_wd( int wd ) {
-	niceassert( initialized, "inotifytools_initialize not called yet" );
+	niceassert(initialized, "inotifytools_initialize not called yet");
 	watch *w = watch_from_wd(wd);
 	if (!w) return 1;
 
@@ -1150,7 +1152,7 @@ int inotifytools_remove_watch_by_wd( int wd ) {
  *       establish the watch.
  */
 int inotifytools_remove_watch_by_filename( char const * filename ) {
-	niceassert( initialized, "inotifytools_initialize not called yet" );
+	niceassert(initialized, "inotifytools_initialize not called yet");
 	watch *w = watch_from_filename(filename);
 	if (!w) return 1;
 
@@ -1197,7 +1199,7 @@ int inotifytools_watch_file(char const* filename, int events) {
  *         obtained from inotifytools_error().
  */
 int inotifytools_watch_files(char const* filenames[], int events) {
-	niceassert( initialized, "inotifytools_initialize not called yet" );
+	niceassert(initialized, "inotifytools_initialize not called yet");
 	error = 0;
 
 	static int i;
@@ -1423,7 +1425,7 @@ struct inotify_event * inotifytools_next_event( long int timeout ) {
  *       the @a timeout period begins again each time a matching event occurs.
  */
 struct inotify_event * inotifytools_next_events( long int timeout, int num_events ) {
-	niceassert( initialized, "inotifytools_initialize not called yet" );
+	niceassert(initialized, "inotifytools_initialize not called yet");
 	niceassert( num_events <= MAX_EVENTS, "too many events requested" );
 
 	if ( num_events < 1 ) return NULL;
@@ -1718,7 +1720,7 @@ int inotifytools_watch_recursively(char const* path, int events) {
 int inotifytools_watch_recursively_with_exclude(char const* path,
 						int events,
 						char const** exclude_list) {
-	niceassert( initialized, "inotifytools_initialize not called yet" );
+	niceassert(initialized, "inotifytools_initialize not called yet");
 
 	DIR * dir;
 	char * my_path;
diff --git a/libinotifytools/src/stats.c b/libinotifytools/src/stats.c
index 1a0dcaff09e0..316fdd6ab505 100644
--- a/libinotifytools/src/stats.c
+++ b/libinotifytools/src/stats.c
@@ -250,7 +250,7 @@ int inotifytools_get_stat_by_filename( char const * filename,
  * event tallies to 0.
  */
 void inotifytools_initialize_stats() {
-	niceassert( initialized, "inotifytools_initialize not called yet" );
+	niceassert(initialized, "inotifytools_initialize not called yet");
 
 	// if already collecting stats, reset stats
 	if (collect_stats) {

The ones on the niceassert()s will sometimes make the style inconsistent with the surrounding code.

Not sure what Codecov's complaning about 🤔.

ericcurtin · 2022-10-06T10:52:58Z

The "rule" is, all new code must adhere to the style in the .clang-format file, so we can achieve eventual consistency in the source code style, even if that makes the style inconsistent with the surrounding code in a specific area.

The code coverage failure can be ignored for this change, 0.17% is negligible and this didn't add extra lines of code.

Running inotifywait 3.22.1.0 on Ubuntu 22.04, I observed a bug apparently due to a bad file descriptor: $ inotifywait -e modify foo Setting up watches. Couldn't watch foo: Bad file descriptor I could also reproduce with the master branch. Investigating further, it appears that inotifytools_init() in inotifytools.c exits immediately after entry, because the global variable "init" is non-null, even though we just called it at the beginning of inotifywait.c's main(), and the global variable is supposed to be initialised at 0. Bisecting showed that this variable used to be static, but was changed at some point to non-static to share it with stats.c, and this is where the bug fist occurs. As far as I understand, the symbol conflicts with the init() function from the program's entry point [0]. When entering inotifytools_init(), it seems that the value for that function is picked up and compared to 0, found non-null, and the initialization function exits, leaving inotify_fd uninitialized (-1) and a bad file descriptor. Renaming the variable to something other than "init" is enough to fix the bug. $ ./src/inotifywait -e modify foo Setting up watches. Watches established <waits> [0] https://www.geeksforgeeks.org/executing-main-in-c-behind-the-scene/ Fixes: dd59c53 ("Extract stats into their own file (inotify-tools#129)")

Qeole · 2022-10-06T10:56:15Z

all new code must adhere to the style

Alright, I just added the formatting changes to the commit.

sonarqubecloud · 2022-10-06T11:43:20Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

ericcurtin · 2022-10-06T11:45:37Z

Squashed and merged @Qeole the contribution is appreciated!

ericcurtin · 2022-10-06T11:47:18Z

If you like, you'll see lines that the Codecov tool identified as not covered, feel free to contribute tests

Qeole · 2022-10-06T11:53:05Z

Thanks a lot for merging, and for the assistance! I'm sorry, I wouldn't mind helping a bit with the tests, but really won't have the cycles in the coming weeks. That was just a bugfix in passing :).

Qeole force-pushed the pr/init branch from 64e834e to dbdabc8 Compare October 6, 2022 10:55

ericcurtin merged commit be8426c into inotify-tools:master Oct 6, 2022

Qeole deleted the pr/init branch October 6, 2022 11:50

chenrui333 mentioned this pull request Aug 21, 2023

inotify-tools 4.23.8.0 Homebrew/homebrew-core#140114

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Comments

libinotifytools: Rename init variable to fix conflict with entry point#170

libinotifytools: Rename init variable to fix conflict with entry point#170
ericcurtin merged 1 commit intoinotify-tools:masterfrom
Qeole:pr/init

Qeole commented Oct 5, 2022 •

edited

Loading

Uh oh!

ericcurtin commented Oct 6, 2022 •

edited

Loading

Uh oh!

codecov bot commented Oct 6, 2022 •

edited

Loading

Uh oh!

Qeole commented Oct 6, 2022

Uh oh!

ericcurtin commented Oct 6, 2022

Uh oh!

Qeole commented Oct 6, 2022

Uh oh!

sonarqubecloud bot commented Oct 6, 2022

Uh oh!

ericcurtin commented Oct 6, 2022

Uh oh!

ericcurtin commented Oct 6, 2022

Uh oh!

Qeole commented Oct 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Comments

Conversation

Qeole commented Oct 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericcurtin commented Oct 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Oct 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Qeole commented Oct 6, 2022

Uh oh!

ericcurtin commented Oct 6, 2022

Uh oh!

Qeole commented Oct 6, 2022

Uh oh!

sonarqubecloud bot commented Oct 6, 2022

Uh oh!

ericcurtin commented Oct 6, 2022

Uh oh!

ericcurtin commented Oct 6, 2022

Uh oh!

Qeole commented Oct 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Qeole commented Oct 5, 2022 •

edited

Loading

ericcurtin commented Oct 6, 2022 •

edited

Loading

codecov bot commented Oct 6, 2022 •

edited

Loading