Gentoo Forums
Gentoo Forums
Gentoo Forums
Quick Search: in
Learning C: please criticize my patch
View unanswered posts
View posts from last 24 hours

 
Reply to topic    Gentoo Forums Forum Index Portage & Programming
View previous topic :: View next topic  
Author Message
halcon
Apprentice
Apprentice


Joined: 15 Dec 2019
Posts: 279

PostPosted: Sun Oct 18, 2020 11:02 am    Post subject: Learning C: please criticize my patch Reply with quote

Hi, all.

I've written a patch to glib-gio-gunixvolumemonitor, simulating the functionality of x-gvfs-hide, and I would like to hear views.

============
The issue is that I see a long annoying list of all possible mounts and drives when I am opening the file-choosing dialogue (with "Open" or "Save as") in any program using GTK graphical interface. The option "x-gvfs-hide" is intended to prevent the mounts from being shown, but for some reason doesn't work, for several years. This option is intended to be put in /etc/fstab, in the column of mount options.

What results I want: to give the user ability to prevent certain mounts/drives from being shown, or via /etc/fstab, or via a simple text file with a list of mounts/drives.
============

I think I have to answer some questions in anticipation:

1. I have a very little experience in C. I got my first segfault while working on this patch (but finally fixed it; in the code that I post it does not happen anymore).

2. I've chosen a real task instead of a training exercise because with a training exercise I would have less motivation to spend my time on it. For several years already I am going to learn C and I've been always found some reason to postpone it :)

3. I understand that glib is one of fundamental libs, that it is a complicated lib and that one should not patch it lightly each time he meet an issue with it. My patch, fixing one thing in glib, sooner of all, breaks something else.

4. I know that the corresponding bug was resolved and closed a year ago. But for some reason I still face the issue with glib-2.64.5. Likely that the reason is in some specific (or mis-) configuration in my system.

5. I know that all the pull-requests fixing that bug are for gunixmounts, not for gunixvolumemonitor: PR1, PR2, PR3. I started this trial working with gunixmounts. But I discovered that not all mountpoints existing in my GTK Open/Save dialogues are being handled with gunixmounts. There are even var, tmp, shm in my Dialogues... Weird, yes? As I said already, the reason may be in my system.

6. Yes, I can just ask about my issue on gitlab.gnome.org, can reopen that bug. But I am more interested in learning, doing trials... Before writing this patch I tried:
(naturally, besides adding gvfs-hide and x-gvfs-hide to /etc/fstab, which didn't work)
- Changing /etc/fstab permissions to 600. It worked perfectly, but it broke some of my wine programs accessing cdrom.
- Uninstalling gvfs. I had to write my own ebuild for evince, without dependency for gvfs (available in my overlay) for achieving that. Bit it didn't help at all. As if nothing changed.
So, it's just for fun.

Code:
--- a/gio/gunixvolumemonitor.c   2020-08-18 14:05:22.000000000 +0300
+++ a/gio/gunixvolumemonitor.c   2020-10-18 04:13:37.183259013 +0300
@@ -20,6 +20,12 @@
  * Author: Alexander Larsson <alexl@redhat.com>
  *         David Zeuthen <davidz@redhat.com>
  */
+
+#include <syslog.h>
+
+void openlog(const char *ident, int option, int facility);
+void syslog (int priority, const char *format, ...);
+void closelog(void);
 
 #include "config.h"
 
@@ -321,6 +327,13 @@
   GList *removed, *added;
   GList *l;
   GUnixVolume *volume;
+  const char *volume_name;
+  const char *user_forbidden_volumes[] = { "cdaudio", "cdrom", "shm", "tmp", "var", "sambadir1", "sambadir2" };
+  char logged_str_volume[101] = " ";
+  unsigned int loop_i_volume;
+  unsigned int found_user_forbidden_volume;
+
+  openlog("glib::gunixvolumemonitor.c::update_volumes", LOG_PID, LOG_USER);
   
   new_mountpoints = g_unix_mount_points_get (NULL);
   
@@ -349,19 +362,42 @@
   for (l = added; l != NULL; l = l->next)
     {
       GUnixMountPoint *mountpoint = l->data;
-     

       volume = _g_unix_volume_new (G_VOLUME_MONITOR (monitor), mountpoint);
-      if (volume)
-   {
-     monitor->volumes = g_list_prepend (monitor->volumes, volume);
-     g_signal_emit_by_name (monitor, "volume-added", volume);

+      volume_name = g_volume_get_name (volume);

+      found_user_forbidden_volume = 0;
+      for (loop_i_volume = 0; loop_i_volume < sizeof (user_forbidden_volumes) / sizeof (user_forbidden_volumes[0]); loop_i_volume++) {
+        if (strcmp (volume_name, user_forbidden_volumes[loop_i_volume]) == 0 ) {
+          found_user_forbidden_volume = 1;
+          memset (logged_str_volume, 0, sizeof (logged_str_volume));
+          strcat (logged_str_volume, "user forbidden volume_name: ");
+          strcat (logged_str_volume, volume_name);
+          syslog (LOG_EMERG, "%s", logged_str_volume);
+          break;
    }
+      }

+      if (found_user_forbidden_volume == 0) {
+        memset (logged_str_volume, 0, sizeof (logged_str_volume));
+        strcat (logged_str_volume, "user allowed volume_name: ");
+        strcat (logged_str_volume, volume_name);
+        syslog (LOG_EMERG, "%s", logged_str_volume);
+        if (volume)
+     {
+       monitor->volumes = g_list_prepend (monitor->volumes, volume);
+       g_signal_emit_by_name (monitor, "volume-added", volume);
+     }
+      }
     }
   
   g_list_free (added);
   g_list_free (removed);
   g_list_free_full (monitor->last_mountpoints, (GDestroyNotify) g_unix_mount_point_free);
   monitor->last_mountpoints = new_mountpoints;

+  closelog();
 }
 
 static void
@@ -373,6 +409,12 @@
   GUnixMount *mount;
   GUnixVolume *volume;
   const char *mount_path;
+  const char *user_forbidden_mounts[] = { "/dev", "/tmp", "/var" };
+  char logged_str_mount[101] = " ";
+  unsigned int loop_i_mount;
+  unsigned int found_user_forbidden_mount;

+  openlog("glib::gunixvolumemonitor.c::update_mounts", LOG_PID, LOG_USER);
   
   new_mounts = g_unix_mounts_get (NULL);
   
@@ -403,17 +445,37 @@
 
       mount_path = g_unix_mount_get_mount_path (mount_entry);
       
-      volume = _g_unix_volume_monitor_lookup_volume_for_mount_path (monitor, mount_path);
-      mount = _g_unix_mount_new (G_VOLUME_MONITOR (monitor), mount_entry, volume);
-      if (mount)
-   {
-     monitor->mounts = g_list_prepend (monitor->mounts, mount);
-     g_signal_emit_by_name (monitor, "mount-added", mount);
+      found_user_forbidden_mount = 0;
+      for (loop_i_mount = 0; loop_i_mount < sizeof (user_forbidden_mounts) / sizeof (user_forbidden_mounts[0]); loop_i_mount++) {
+        if (strstr (mount_path, user_forbidden_mounts[loop_i_mount]) != NULL) {
+          found_user_forbidden_mount = 1;
+          memset (logged_str_mount, 0, sizeof (logged_str_mount));
+          strcat (logged_str_mount, "user forbidden mount_path: ");
+          strcat (logged_str_mount, mount_path);
+          syslog (LOG_EMERG, "%s", logged_str_mount);
+          break;
    }
-    }
+      }
+     
+      if (found_user_forbidden_mount == 0) {
+        memset (logged_str_mount, 0, sizeof (logged_str_mount));
+        strcat (logged_str_mount, "user allowed mount_path: ");
+        strcat (logged_str_mount, mount_path);
+        syslog (LOG_EMERG, "%s", logged_str_mount);
+        volume = _g_unix_volume_monitor_lookup_volume_for_mount_path (monitor, mount_path);
+        mount = _g_unix_mount_new (G_VOLUME_MONITOR (monitor), mount_entry, volume);
+        if (mount)
+     {
+       monitor->mounts = g_list_prepend (monitor->mounts, mount);
+       g_signal_emit_by_name (monitor, "mount-added", mount);
+     }
+        }
+      }
   
   g_list_free (added);
   g_list_free (removed);
   g_list_free_full (monitor->last_mounts, (GDestroyNotify) g_unix_mount_free);
   monitor->last_mounts = new_mounts;

+  closelog();
 }


Last edited by halcon on Sun Oct 18, 2020 7:12 pm; edited 4 times in total
Back to top
View user's profile Send private message
dwpaul
n00b
n00b


Joined: 18 Aug 2004
Posts: 10

PostPosted: Sun Oct 18, 2020 12:08 pm    Post subject: Reply with quote

strcat is generally not a very good way to build strings. It's not all that efficient, since it always has to find the end of the string, and you can easily have buffer overruns -- which is likely what is happening here.

In the first block, it is probably not a problem, since you are using strcmp and the string you are building should always fit in your buffer. In the second one, controlled by strstr, you could easily overrun the buffer. I would recommend to run in gdb, and you should see the problem happen. In this case, you are smashing the stack badly enough that you get the segfault during strcat. You could use gdb to see the strings involved when it crashes. In case the crash happened elsewhere, you could use a tool like valgrind to see.

One more general note: you don't need to use a memset then strcat -- you could instead just use strcpy for the first part of the string.

Since you're learning C, I would recommend that you look at the printf family of functions, as that is the normal way to build strings. If you want to be very safe, you should look at the asprintf function. (man asprintf) It is a GNU extension, so not everyone wants to use it in their code. However, you can also do something similar with sprintf. But make sure you do not run out of buffer space (i.e., use snprintf and allocate a larger buffer if necessary based on its return value)
Back to top
View user's profile Send private message
ff11
Guru
Guru


Joined: 10 Mar 2014
Posts: 592

PostPosted: Sun Oct 18, 2020 12:20 pm    Post subject: Reply with quote

I don't think anyone could or should criticize other people's learning path. What could be criticized would be the efficiency of the method, but as you have already mentioned the motivating factor and that you are doing it for fun, then I also have nothing to say about it.

That said, I have a recommendation, which works for any programming language: when working with the code that others have developed, try to keep the same style as the code used in the other files (spacing, tabs or blanks, indentation, line breaks ...), even if that style isn't documented yet. And in the case of glib: https://developer.gnome.org/programming-guidelines/stable/c-coding-style.html.en

So what could I say? Just: have fun (^_^)/

EDIT: from https://developer.gnome.org/programming-guidelines/stable/c-coding-style.html.en:
Quote:
The Single Most Important Rule
The single most important rule when writing code is this: check the surrounding code and try to imitate it.

As a maintainer it is dismaying to receive a patch that is obviously in a different coding style to the surrounding code. This is disrespectful, like someone tromping into a spotlessly-clean house with muddy shoes.

So, whatever this document recommends, if there is already written code and you are patching it, keep its current style consistent even if it is not your favorite style.

I can't stress this enough.
_________________
| Proverbs 26:12 |
| There is more hope for a fool than for a wise man that are wise in his own eyes. |
* AlphaGo - The Movie - Full Documentary
"I want to apologize for being so powerless" - Lee
Back to top
View user's profile Send private message
halcon
Apprentice
Apprentice


Joined: 15 Dec 2019
Posts: 279

PostPosted: Sun Oct 18, 2020 1:18 pm    Post subject: Reply with quote

dwpaul wrote:
I would recommend to run in gdb
dwpaul wrote:
One more general note: you don't need to use a memset then strcat -- you could instead just use strcpy for the first part of the string.
dwpaul wrote:
Since you're learning C, I would recommend that you look at the printf family of functions, as that is the normal way to build strings.

Thank you for the recommendations! I'll look at each. I don't yet know how to run gdb...

All this work with memory is totally new for me, I dealt with more high-level languages only.

About segfault. Sorry, I didn't expressed it in right English. I meant, I got my first segfault while working on this patch but finally fixed it. In the code that I posted it does not happen anymore. It was happening when I had logged_str like there. I'm going to edit my original post to make it clear.
Back to top
View user's profile Send private message
dwpaul
n00b
n00b


Joined: 18 Aug 2004
Posts: 10

PostPosted: Sun Oct 18, 2020 1:33 pm    Post subject: Reply with quote

Ah, thanks for clarifying. I see the original issue now.

There is still a risk of an eventual crash, due to the potential buffer overflow -- if ever the parts of the string are more than 100 characters, you will overrun the buffer and write over other data on the stack.

However, you do have an easy out in this case: the syslog functions already seem to be taking a 'printf'-style format string, so you can avoid all of the string and memory work.

For example, you can replace this:

Code:
+          memset (logged_str_volume, 0, sizeof (logged_str_volume));
+          strcat (logged_str_volume, "user forbidden volume_name: ");
+          strcat (logged_str_volume, volume_name);
+          syslog (LOG_EMERG, "%s", logged_str_volume);


with this:
Code:
+          syslog (LOG_EMERG, "user forbidden volume_name: %s", volume_name);


At least, it doesn't look like you use the logged_str_* buffers for anything other than the call to syslog.

Understanding memory issues, pointers, and things like stack vs heap, is probably one of the most challenging things for beginning C programmers. It looks like you're well on your way learning that completely, though.

For running gdb -- there is definitely a learning curve there. It's worth it, though. There are various front-ends that can help with that, including the fairly old but still widely used ddd. But there are also integrations to vim and emacs that are worth checking out.
Back to top
View user's profile Send private message
halcon
Apprentice
Apprentice


Joined: 15 Dec 2019
Posts: 279

PostPosted: Sun Oct 18, 2020 1:37 pm    Post subject: Reply with quote

ff11 wrote:
That said, I have a recommendation, which works for any programming language: when working with the code that others have developed, try to keep the same style as the code used in the other files (spacing, tabs or blanks, indentation, line breaks ...), even if that style isn't documented yet. And in the case of glib: https://developer.gnome.org/programming-guidelines/stable/c-coding-style.html.en
ff11 wrote:
The Single Most Important Rule
...

Thank you, very useful, because it's about netiquette. I knew about indentation (and tried to imitate it, two spaces) and a few things like that, but I didn't know almost all other details (described in that glib document, I did not see it).

I thought that I don't have to perfect my code further (like moving most of it to separate functions; replacing the hardcoded arrays to reading a text file - the same /etc/fstab or other). Now I see a reason for continuing the work. It's a good challenge to try to write in a good style.

ff11 wrote:
So what could I say? Just: have fun (^_^)/

Thanks! :)
Back to top
View user's profile Send private message
halcon
Apprentice
Apprentice


Joined: 15 Dec 2019
Posts: 279

PostPosted: Sun Oct 18, 2020 2:20 pm    Post subject: Reply with quote

dwpaul wrote:
the syslog functions already seem to be taking a 'printf'-style format string, so you can avoid all of the string and memory work.
Code:
+          syslog (LOG_EMERG, "user forbidden volume_name: %s", volume_name);

Ah, it's beautiful! :) Thanks!

dwpaul wrote:
There are various front-ends that can help with that, including the fairly old but still widely used ddd. But there are also integrations to vim and emacs that are worth checking out.

I used to use nano and SciTE, since those days when I started to use Linux, and it's too late to relearn...
Back to top
View user's profile Send private message
Hu
Moderator
Moderator


Joined: 06 Mar 2007
Posts: 15985

PostPosted: Sun Oct 18, 2020 5:29 pm    Post subject: Re: Learning C: please criticize my patch Reply with quote

It might be a bit easier to critique this if you described specifically what results you want. You mentioned emulating a particular feature, but not everyone will know what that feature does when it works, and therefore how to judge whether you are emulating it well. Since you are using a hard-coded list, I think it is clear enough what you want.
halcon wrote:
Code:
--- a/gio/gunixvolumemonitor.c   2020-08-18 14:05:22.000000000 +0300
+++ a/gio/gunixvolumemonitor.c   2020-10-18 04:13:37.183259013 +0300
@@ -20,6 +20,12 @@
+#include <syslog.h>
+
+void openlog(const char *ident, int option, int facility);
+void syslog (int priority, const char *format, ...);
+void closelog(void);
Generally, you should not declare a non-static function in a source file. Instead, include the header that declares it for you. What you have will work, assuming you gave the correct prototype. If you get the prototype from the header, then the maintainer of that header bears the responsibility of providing the correct prototype. Ideally, the maintainer of the header also includes that header in the source file that defines the function. If so, then the compiler will warn that maintainer at build time if the header and definition disagree.
halcon wrote:
Code:

@@ -321,6 +327,13 @@
+  const char *user_forbidden_volumes[] = { "cdaudio", "cdrom", "shm", "tmp", "var", "sambadir1", "sambadir2" };
There is room for another const here. Based on how you use this, I would declare this as static const char *const user_forbidden_volumes[], so that the initialization is done at load time and stored in a read-only segment. Your version stores the pointers on the stack and could change them, but you never actually change them. Therefore, your version does more work and is less space efficient, despite not needing the flexibility provided by a stack-local array.
halcon wrote:
Code:
+  unsigned int loop_i_volume;
In C99 and later, you could declare this as part of the for loop itself, to keep the variable closer to its usage site. The generated code would be the same, but it would be show the reader the variable's expected scope.
halcon wrote:
Code:
+  openlog("glib::gunixvolumemonitor.c::update_volumes", LOG_PID, LOG_USER);
You may, or may not, want to use __FILE__ and/or __LINE__ in this message. Those are managed by the compiler to expand to the current filename and current line number, respectively. __FILE__ is typically the filename as passed to the compiler, so it might include a relative or even absolute path. You may want to avoid __FILE__ if its expansion does not follow the style you want. On the other hand, it is very convenient for writing debug messages, because the message will always tell exactly where it came from, even if someone moved the debug code elsewhere. __FILE__ can be pasted into a string. __LINE__ is a number, so it would need to be formatted as %u via printf or similar.
halcon wrote:
Code:
       volume = _g_unix_volume_new (G_VOLUME_MONITOR (monitor), mountpoint);
You don't move this line, but it looks to me like it (might) be an allocation. If so, that would leak the underlying object whenever your new code suppresses adding it to the list.
halcon wrote:
Code:
-      if (volume)
-   {
-     monitor->volumes = g_list_prepend (monitor->volumes, volume);
-     g_signal_emit_by_name (monitor, "volume-added", volume);

+      volume_name = g_volume_get_name (volume);
The old code tests for if volume is not NULL, which suggests to me that it could be NULL. Is it legal to call g_volume_get_name on a NULL object? If it is, do you get a return value that is legal to pass to strcmp? My guess is that you need to skip your new code if volume == NULL.
halcon wrote:
Code:
@@ -373,6 +409,12 @@
   GUnixMount *mount;
   GUnixVolume *volume;
   const char *mount_path;
+  const char *user_forbidden_mounts[] = { "/dev", "/tmp", "/var" };
Same static const char *const comment as above.
halcon wrote:
Code:

+        if (strstr (mount_path, user_forbidden_mounts[loop_i_mount]) != NULL) {
This is probably not what you want. As written, it will suppress /home/me/variables, because /var is contained in the string. You probably want strncmp instead, so that you can check for an initial substring, rather than any substring. If so, you probably also need special handling so that /variable is not matched by the test for /var. If you want more specific guidance here, ask. Otherwise, think of it as a learning opportunity to turn my suggestion into code. ;)

I like Valgrind, as another poster mentioned. Another alternative, which is faster to run, but requires some advance setup, is to use the compiler's -fsanitize=address to build an instrumented object that will abort on certain types of errors, and try its best to tell you what happened.
Back to top
View user's profile Send private message
halcon
Apprentice
Apprentice


Joined: 15 Dec 2019
Posts: 279

PostPosted: Sun Oct 18, 2020 6:30 pm    Post subject: Re: Learning C: please criticize my patch Reply with quote

Hi Hu,

Thank you very much for all the remarks!

I'm replying now briefly, until I have time to look at everything with attention.

Hu wrote:
Instead, include the header that declares it for you.

I didn't do that because I haven't yet dealt with .h files... Will fix it.

Hu wrote:
the message will always tell exactly where it came from, even if someone moved the debug code elsewhere.

Good point! It's much better than writing the function name manually.

Hu wrote:
My guess is that you need to skip your new code if volume == NULL

Exactly! I should have seen that, it's agnostic to the language... I think the best solution will be to move my code inside if (volume). And inside if (mount) too - it would be just logical.

Hu wrote:
As written, it will suppress /home/me/variables

Same here, agnostic, you are absolutely right :)

EDIT:
Hu wrote:
It might be a bit easier to critique this if you described specifically what results you want.

I added the explanations to the original post.
Back to top
View user's profile Send private message
GDH-gentoo
Guru
Guru


Joined: 20 Jul 2019
Posts: 484
Location: South America

PostPosted: Sun Oct 18, 2020 7:20 pm    Post subject: Reply with quote

The log messages that the added code produces differ only in a single word, based on the value of found_user_forbidden_volume, so you could use the conditional operator and have only a single syslog() call:
Code:
syslog(LOG_EMERG, "user %s volume_name: %s", found_user_forbidden_volume? "forbidden": "allowed", volume_name);
Then, the for loop that iterates over user_forbidden_volumes can be reduced to an if statement that conditionally sets found_user_forbidden_volume to 1.

Some people also prefer to move declarations of objects with automatic storage duration close to the the first place they are used, to the smallest scope where they are needed, and prefer to initialize them right away if their initial value can be computed. This is the case of volume_name and found_user_forbidden_volume here:
Code:
for (l = added; l != NULL; l = l->next)
  {
    GUnixMountPoint *mountpoint = l->data;

    volume = _g_unix_volume_new (G_VOLUME_MONITOR (monitor), mountpoint);
    if (volume)
      {
        const char *volume_name = g_volume_get_name (volume);
        unsigned int found_user_forbidden_volume = 0;
        for (unsigned int loop_i_volume = 0; loop_i_volume < sizeof user_forbidden_volumes / sizeof *user_forbidden_volumes; loop_i_volume++)
          if (!strcmp(volume_name, user_forbidden_volumes[loop_i_volume]))
            found_user_forbidden_volume = 1;

        syslog(LOG_EMERG, "user %s volume_name: %s", found_user_forbidden_volume? "forbidden": "allowed", volume_name);

        if (found_user_forbidden_volume)
          // Free the resources that _g_unix_volume_new() might have allocated.
        else
          // Do whatever the unpatched code does.
      }
  }

Same for the patch to update_mounts().


Last edited by GDH-gentoo on Sun Oct 18, 2020 7:24 pm; edited 2 times in total
Back to top
View user's profile Send private message
halcon
Apprentice
Apprentice


Joined: 15 Dec 2019
Posts: 279

PostPosted: Sun Oct 18, 2020 7:20 pm    Post subject: Reply with quote

I noticed that I linked (in the origianl post) a specific bug for bind mounts. And there is a more general one.

EDIT:
Though, there is a link to bind bug from the general...

Interesting, does anybody else have this issue with the modern glib?
Back to top
View user's profile Send private message
halcon
Apprentice
Apprentice


Joined: 15 Dec 2019
Posts: 279

PostPosted: Sun Oct 18, 2020 7:46 pm    Post subject: Reply with quote

Hola GDH-gentoo,

GDH-gentoo wrote:
Code:
for (l = added; l != NULL; l = l->next)
  {
    GUnixMountPoint *mountpoint = l->data;

    volume = _g_unix_volume_new (G_VOLUME_MONITOR (monitor), mountpoint);
    if (volume)
      {
        const char *volume_name = g_volume_get_name (volume);
        unsigned int found_user_forbidden_volume = 0;
        for (unsigned int loop_i_volume = 0; loop_i_volume < sizeof user_forbidden_volumes / sizeof *user_forbidden_volumes; loop_i_volume++)
          if (!strcmp(volume_name, user_forbidden_volumes[loop_i_volume]))
            found_user_forbidden_volume = 1;

        syslog(LOG_EMERG, "user %s volume_name: %s", found_user_forbidden_volume? "forbidden": "allowed", volume_name);

        if (found_user_forbidden_volume)
          // Free the resources that _g_unix_volume_new() might have allocated.
        else
          // Do whatever the unpatched code does.
      }
  }

Same for the patch to update_mounts().

Thank you for this improvement!

I usually am a little bit afraid of shortening code, because I usually prefer readability, but in this case, considering my multiple syslogs, your solution is much more elegant, and even ? : don't seem a shortening here :)

EDIT:
And I think your code corresponds to glib requirements... Without my mandatory (and beginning at the same line) curly brackets which I am used to, but which are not welcome, as I understand now, after the ff11' link.
Back to top
View user's profile Send private message
halcon
Apprentice
Apprentice


Joined: 15 Dec 2019
Posts: 279

PostPosted: Thu Oct 22, 2020 10:57 am    Post subject: Reply with quote

Hi all, again!

I've made some progress in fixing almost all the discovered errors but now I'm stuck at the following:

I need to replace two hardcoded arrays with one hardcoded array and to work with this one hardcoded array so:

Code:
function A {
takes pointer to array, creates *one* hardcoded array with values, makes pointer point to *one* hardcoded array // will be replaced with reading from a text file
}

function B {
takes pointer to array, calls function A, gets *one* hardcoded array, creates the second array by changing each element of *one* hardcoded array, makes pointer point to the second array
}

function C {
creates an empty array, creates pointer to it, passes this pointer to function B, gets the second array with values
}

function D {
creates an empty array, creates pointer to it, passes this pointer to function A, gets *one* hardcoded array with values
}


Currently, the values of *one* hardcoded array are not visible outside function A...

(read_forbidden_mounts = A, read_forbidden_volumes = B, update_volumes = C, update_mounts = D)

pass_ptr_to_empty_array_through_two_funcs_and_return_full_array_back.h
Code:
#include <syslog.h>

void openlog(const char *ident, int option, int facility);
void syslog (int priority, const char *format, ...);
void closelog(void);

pass_ptr_to_empty_array_through_two_funcs_and_return_full_array_back.c
Code:
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#include "pass_ptr_to_empty_array_through_two_funcs_and_return_full_array_back.h"

static int
read_forbidden_mounts (char **array,
                         unsigned int *length)
{
  char *user_forbidden_mounts[] = { "/mnt/cdrom", "/mnt/cdaudio", "/tmp", "/var", "/dev/shm", "/mnt/sambadir1", "/mnt/sambadir2" };
  unsigned int capacity;
  length = &capacity;
 
  for (unsigned int loop_i_mount = 1; loop_i_mount < sizeof (user_forbidden_mounts) / sizeof (user_forbidden_mounts[0]); loop_i_mount++)
    {
      capacity = loop_i_mount+1;
      array = malloc (capacity * sizeof(char*));
      array[loop_i_mount] = user_forbidden_mounts[loop_i_mount];
    }
  array[0] = user_forbidden_mounts[0];
   
  syslog (LOG_EMERG, "%s[%u]: read_forbidden_mounts successfully assigned all the values, length = %u, array[%u] = %s", __FILE__, __LINE__, *length, 6, array[6]);
 
  return 0;
}

static int
read_forbidden_volumes (char **array,
                         unsigned int *length)
{
  char *concat;
  syslog (LOG_EMERG, "%s[%u]: read_forbidden_volumes started", __FILE__, __LINE__);
 
  if (read_forbidden_mounts (array, length) == 0)
    {
      syslog (LOG_EMERG, "%s[%u]: read_forbidden_volumes successfully read %u forbidden mounts, array[%u] = %s", __FILE__, __LINE__, *length, 6, array[6]);
      for (unsigned int loop_i_volume = 0; loop_i_volume < *length; loop_i_volume++)
        {
          strcat (strcpy (concat, array[loop_i_volume]), " changed");
          *(array)[loop_i_volume] = *concat;
        }
      syslog (LOG_EMERG, "%s[%u]: read_forbidden_volumes successfully assigned all the values, length = %u, array [%u] = %s", __FILE__, __LINE__, *length, 6, array[6]);
      return 0;
    }
  else
    {
      syslog (LOG_EMERG, "%s[%u]: read_forbidden_volumes failed to read forbidden mounts", __FILE__, __LINE__);
      return 1;
    }
}

static void
update_volumes (void)
{
  char *user_forbidden_volumes[] = {""};
  char **ptr_user_forbidden_volumes = &user_forbidden_volumes[1];
 
  printf ("update_volumes started\n");
  unsigned int user_forbidden_volumes_length = 0;   
  unsigned int *ptr_user_forbidden_volumes_length = &user_forbidden_volumes_length;
    if (read_forbidden_volumes (ptr_user_forbidden_volumes, ptr_user_forbidden_volumes_length) == 0)
      syslog (LOG_EMERG, "%s[%u]: update_volumes successfully read %u forbidden volumes", __FILE__, __LINE__, user_forbidden_volumes_length);
    else
      syslog (LOG_EMERG, "%s[%u]: update_volumes failed to read forbidden volumes", __FILE__, __LINE__);
  printf ("update_volumes ended\n");
}

static void
update_mounts (void)
{
  char *user_forbidden_mounts[] = {""};
  char **ptr_user_forbidden_mounts = &user_forbidden_mounts[1];
 
  printf ("update_mounts started\n");
  unsigned int user_forbidden_mounts_length = 0;   
  unsigned int *ptr_user_forbidden_mounts_length = &user_forbidden_mounts_length;
    if (read_forbidden_mounts (ptr_user_forbidden_mounts, ptr_user_forbidden_mounts_length) == 0)
      syslog (LOG_EMERG, "%s[%u]: update_mounts successfully read %u forbidden mounts", __FILE__, __LINE__, user_forbidden_mounts_length);
    else
      syslog (LOG_EMERG, "%s[%u]: update_mounts failed to read forbidden mounts", __FILE__, __LINE__);
  printf ("update_mounts ended\n");
}

int
main (void)
{
  openlog ("abcd", LOG_PID, LOG_USER);
  update_volumes ();
  update_mounts ();
  closelog ();
  printf ("main ended\n");
  return 0;
}

Output
Code:
update_volumes started
update_volumes ended
update_mounts started
update_mounts ended
main ended

Syslog
Code:
pass_ptr_to_empty_array_through_two_funcs_and_return_full_array_back.c[35]: read_forbidden_volumes started
pass_ptr_to_empty_array_through_two_funcs_and_return_full_array_back.c[25]: read_forbidden_mounts successfully assigned all the values, length = 7, array[6] = /mnt/sambadir2
pass_ptr_to_empty_array_through_two_funcs_and_return_full_array_back.c[39]: read_forbidden_volumes successfully read 0 forbidden mounts, array[6] = _?f??
pass_ptr_to_empty_array_through_two_funcs_and_return_full_array_back.c[45]: read_forbidden_volumes successfully assigned all the values, length = 0, array [6] = _?f??
pass_ptr_to_empty_array_through_two_funcs_and_return_full_array_back.c[65]: update_volumes successfully read 0 forbidden volumes
pass_ptr_to_empty_array_through_two_funcs_and_return_full_array_back.c[25]: read_forbidden_mounts successfully assigned all the values, length = 7, array[6] = /mnt/sambadir2
pass_ptr_to_empty_array_through_two_funcs_and_return_full_array_back.c[81]: update_mounts successfully read 0 forbidden mounts

What am I doing wrong?

(was UPDATED-EDITED)
Back to top
View user's profile Send private message
GDH-gentoo
Guru
Guru


Joined: 20 Jul 2019
Posts: 484
Location: South America

PostPosted: Thu Oct 22, 2020 2:56 pm    Post subject: Reply with quote

halcon wrote:
pass_ptr_to_empty_array_through_two_funcs_and_return_full_array_back.h
Code:
#include <syslog.h>

void openlog(const char *ident, int option, int facility);
void syslog (int priority, const char *format, ...);
void closelog(void);

Hu already pointed this out: don't redeclare openlog(), syslog () and closelog(). Including <syslog.h> already makes them available.

With respect to the rest of the program, it does not what you think it does, and leaks memory.
halcon wrote:
pass_ptr_to_empty_array_through_two_funcs_and_return_full_array_back.c
Code:
static void
update_mounts (void)
{
  char *user_forbidden_mounts[] = {""};
  char **ptr_user_forbidden_mounts = &user_forbidden_mounts[1];
  // ...
}
The user_forbidden_mounts object declared in update_mounts() is an array of one element, which happens to point to an empty string (an array of char objects with a single element whose stored value is 0). ptr_user_forbidden_mounts points therefore to "one element past the end" of user_forbidden_mounts[] (a valid pointer value with special properties).
halcon wrote:
pass_ptr_to_empty_array_through_two_funcs_and_return_full_array_back.c
Code:
static int
read_forbidden_mounts (char **array,
                         unsigned int *length)
{
  // ...
}

// ...

static void
update_mounts (void)
{
  // ...
    if (read_forbidden_mounts (ptr_user_forbidden_mounts, ptr_user_forbidden_mounts_length) == 0)
    // ...
}
Object array in read_forbidden_mounts() is a function parameter of pointer type, initializing it with the value of ptr_user_forbidden_mounts simply makes it point to the same place. These are some of the program's objects when the function has just been called:
Code:
                                        +-----+   +-----+
user_forbidden_mounts @ update_mounts() | [0] |-->| [0] | = 0
                                        +-----+   +-----+
                                           O (one past the end)
                                           ^
                                           |
                                           |
                                        +--+--+
                                  array |     |
                                        +--+--+

After the first iteration of the for loop:
Code:
                                        +-----+   +-----+
user_forbidden_mounts @ update_mounts() | [0] |-->| [0] | = 0
                                        +-----+   +-----+
                                        +-----+
                (dynamically allocated) | [0] |--------------------+
                                        +-----+                    |
                                           ^                       |
                                           |                       |
                                           |                       |
                                        +--+--+                    |
                                  array |     |                    |
                                        +--+--+                    |
                                                   +-----+         V
   user_forbidden_mounts @ read_forbidden_mounts() | [0] |--> "/mnt/cdrom"
                                                   +-----+
                                                   | [1] |--> "/mnt/cdaudio"
                                                   +-----+
                                                   | ... |
Note that there are two objects named "user_forbidden_mounts", one defined in update_mounts(), and the other in read_forbidden_mounts(), an array of 7 elements. Every usage of identifier "user_forbidden_mounts" in read_forbidden_mounts() refers to the second one. array has been assigned the result of malloc(), so, assuming the call succeeded (which hasn't been checked, but oh well), it now points to a dynamically allocated object that is an array of 1 element.

After the second iteration of the for loop:
Code:
                                        +-----+   +-----+
user_forbidden_mounts @ update_mounts() | [0] |-->| [0] | = 0
                                        +-----+   +-----+
                                        +-----+
                (dynamically allocated) | [0] |--------------------+
                                        +-----+                    |
                                                                   |
                                                   +-----+         V
   user_forbidden_mounts @ read_forbidden_mounts() | [0] |--> "/mnt/cdrom"
                                                   +-----+
                                                   | [1] |--> "/mnt/cdaudio"
                                                   +-----+         ^
                                                   | ... |         |
                                        +-----+                    |
             (dynamically allocated) +->| [0] | = indeterminate    |
                                     |  +-----+                    |
                                     |  | [1] |--------------------+
                                     |  +-----+
                                     |
                                  +--+--+
                            array |     |
                                  +-----+
array now points to a second dynamically allocated object, that is an array of 2 elements. First element hasn't been initialized, so it has an indeterminate value; second one points to the same string that user_forbidden_mounts[1] does, because of the assignment to array[loop_i_mount]. But the first allocated object still exists (because free() has not been called), and is now unreferenced... so you have leaked memory.
Back to top
View user's profile Send private message
halcon
Apprentice
Apprentice


Joined: 15 Dec 2019
Posts: 279

PostPosted: Thu Oct 22, 2020 4:33 pm    Post subject: Reply with quote

Hi GDH-gentoo,

Thank you very much for your detailed reply!

GDH-gentoo wrote:
Hu already pointed this out: don't redeclare openlog(), syslog () and closelog(). Including <syslog.h> already makes them available.

Oh, sorry, my fault. I have switched to g_log in my current patch version, so haven't learned this lesson properly.

GDH-gentoo wrote:
it does not what you think it does, and leaks memory.

I would ask what it does and why it leaks, if it does not stray from the main issue.

GDH-gentoo wrote:
The user_forbidden_mounts object declared in update_mounts() is an array of one element, which happens to point to an empty string (an array of char objects with a single element whose stored value is 0). ptr_user_forbidden_mounts points therefore to "one element past the end" of user_forbidden_mounts[] (a valid pointer value with special properties).

Oh, sure! Thanks.
I changed array/pointer initializations in update_volumes to one line:
Code:
  char **ptr_user_forbidden_volumes = malloc (1 * sizeof(char*));

and in update_mounts:
Code:
  char **ptr_user_forbidden_mounts = malloc (1 * sizeof(char*));

GDH-gentoo wrote:
Note that there are two objects named "user_forbidden_mounts", one defined in update_mounts(), and the other in read_forbidden_mounts(), an array of 7 elements. Every usage of identifier "user_forbidden_mounts" in read_forbidden_mounts() refers to the second one.

This my change ^^ should fix that. There is only one "user_forbidden_mounts" object now.

GDH-gentoo wrote:
assuming the call succeeded (which hasn't been checked, but oh well)

I also added to update_volumes:
Code:
  if (ptr_user_forbidden_volumes == NULL) {
    printf ("Memory not allocated.\n");
    exit (0);
  }

and to update_mounts:
Code:
  if (ptr_user_forbidden_mounts == NULL) {
    printf ("Memory not allocated.\n");
    exit (0);
  }

GDH-gentoo wrote:
it now points to a dynamically allocated object that is an array of 1 element.

I hope, dynamically allocated objects can be used with pointers correctly?

My loop in read_forbidden_mounts is so now:
Code:
  for (unsigned int loop_i_mount = 0; loop_i_mount < sizeof (user_forbidden_mounts) / sizeof (user_forbidden_mounts[0]); loop_i_mount++)
    {
          capacity = loop_i_mount+1;
          array = (char **) realloc (array, capacity * sizeof(char *));
          array[loop_i_mount] = malloc (capacity * sizeof(char));
          strcpy (array[loop_i_mount], user_forbidden_mounts[loop_i_mount]);
    }


It compiles but segfaults, at exiting from read_forbidden_mounts :(

PS
How do you create these beautiful "ASCII-schemes"?

EDIT
https://github.com/halcon74/pass-ptr-to-empty-array
Back to top
View user's profile Send private message
Hu
Moderator
Moderator


Joined: 06 Mar 2007
Posts: 15985

PostPosted: Thu Oct 22, 2020 6:31 pm    Post subject: Reply with quote

As an initial note, for the case where the returned data will be exactly the same, you may be better off storing the data at static scope and returning a non-allocated pointer to it.
https://raw.githubusercontent.com/halcon74/pass-ptr-to-empty-array/5a6ab5925e7a8f91a756cad645168c34c8c5dab2/pass_ptr_to_empty_array_through_two_funcs_and_return_full_array_back.c:
    10     char *user_forbidden_mounts[] = { "/mnt/cdrom", "/mnt/cdaudio", "/tmp", "/var", "/dev/shm", "/mnt/sambadir1", "/mnt/sambadir2" };
C string literals are of type const char[]. Implicit conversion of a string literal to char * is permitted in C, but discouraged. It is forbidden in C++, so you would be better off avoiding it in both languages.

Also, as before, you should make user_forbidden_mounts a static const char *const user_forbidden_mounts[], since it is never mutated.
https://raw.githubusercontent.com/halcon74/pass-ptr-to-empty-array/5a6ab5925e7a8f91a756cad645168c34c8c5dab2/pass_ptr_to_empty_array_through_two_funcs_and_return_full_array_back.c:
    11     unsigned int capacity;
    12     length = &capacity;
This is not useful. capacity will go out of scope at the end of the function, and the caller will not have access to it. I think what you want here is:
Code:
static int read_forbidden_mounts(char **const array, unsigned int *const length)
{
   unsigned capacity = 0;
   /* code to use/update capacity */
   *length = capacity;
   return 0;
}
This writes the computed final capacity to a location chosen by the caller. Note that I made both pointers const in their local form since you want them to continue to point to where the caller intended. This breaks your use of realloc, which was wrong as written.
https://raw.githubusercontent.com/halcon74/pass-ptr-to-empty-array/5a6ab5925e7a8f91a756cad645168c34c8c5dab2/pass_ptr_to_empty_array_through_two_funcs_and_return_full_array_back.c:
    15       {
    16         capacity = loop_i_mount+1;
    17         array = (char **) realloc (array, capacity * sizeof(char *));
realloc may fail, and return NULL to indicate that no change was made. You need to detect the NULL return, treat it as an error condition, and preserve the last known value of array from before the call. That value will be valid, but will point to a region which was not reallocated, so you do not have capacity to add more elements to it.
https://raw.githubusercontent.com/halcon74/pass-ptr-to-empty-array/5a6ab5925e7a8f91a756cad645168c34c8c5dab2/pass_ptr_to_empty_array_through_two_funcs_and_return_full_array_back.c:
    18         array[loop_i_mount] = malloc (capacity * sizeof(char));
This is wrong. You need to allocate space for sufficient characters to copy. As written, you allocate one character for the first element, two for the second, and so on. This will lead to memory corruption if using the static strings defined on line 10.

Since you want to copy the string, you could use strdup instead of malloc, which will both allocate a correct amount of memory and fill it with a copy of the source string. You would then write:
Code:
      array = (char **) realloc (...);   // but make the fixes described above
      array[loop_i_mount] = strdup(user_forbidden_mounts[loop_i_mount]);
   }
Note the absence of strcpy in my version.
https://raw.githubusercontent.com/halcon74/pass-ptr-to-empty-array/5a6ab5925e7a8f91a756cad645168c34c8c5dab2/pass_ptr_to_empty_array_through_two_funcs_and_return_full_array_back.c:
    37         for (unsigned int loop_i_volume = 0; loop_i_volume < *length; loop_i_volume++)
As a minor optimization, you could copy *length into a local before starting the loop, so that the compiler is not required to repeatedly load *length on each pass of the loop.
https://raw.githubusercontent.com/halcon74/pass-ptr-to-empty-array/5a6ab5925e7a8f91a756cad645168c34c8c5dab2/pass_ptr_to_empty_array_through_two_funcs_and_return_full_array_back.c:
    39             strcat (strcpy (concat, array[loop_i_volume]), " changed");
This is wrong. concat was never initialized, so writing through it will cause undefined behavior, and probably a crash.
https://raw.githubusercontent.com/halcon74/pass-ptr-to-empty-array/5a6ab5925e7a8f91a756cad645168c34c8c5dab2/pass_ptr_to_empty_array_through_two_funcs_and_return_full_array_back.c:
    60     if (ptr_user_forbidden_volumes == NULL) {
    61       printf ("Memory not allocated.\n");
    62       exit (0);
    63     }
This is fine for testing, but when you have the code working properly, you should change this. Terminating the entire program is a severe overreaction to being unable to filter out unwanted strings.
https://raw.githubusercontent.com/halcon74/pass-ptr-to-empty-array/5a6ab5925e7a8f91a756cad645168c34c8c5dab2/pass_ptr_to_empty_array_through_two_funcs_and_return_full_array_back.c:
    65       if (read_forbidden_volumes (ptr_user_forbidden_volumes, ptr_user_forbidden_volumes_length) == 0)
You do not need a dedicated temporary variable here. You could instead write:
Code:
    if (read_forbidden_volumes (ptr_user_forbidden_volumes, &user_forbidden_volumes_length) == 0)
As a general design note, calling malloc and passing the result to a function that calls realloc requires some care, since the pointer in the caller could be stale when the call returns. To handle that, you need the called function to return to the caller the updated pointer value.
Back to top
View user's profile Send private message
halcon
Apprentice
Apprentice


Joined: 15 Dec 2019
Posts: 279

PostPosted: Thu Oct 22, 2020 6:50 pm    Post subject: Reply with quote

Hi Hu,

Thank you for the answers!!

I'm replying now to some most obvious errors, and will "investigate" the others later:

Hu wrote:
Also, as before, you should make user_forbidden_mounts a static const char *const user_forbidden_mounts[], since it is never mutated.

Of course! I remember that you told me about it, and I have already fixed it (in the main file, and in the patch).
I made this pass-ptr-... file only as a test, so didn't perfect every detail.
I was just going to test out the main structure of the work with pointers/arrays and then to reproduce it in the main file, and in the patch.

Hu wrote:
Code:
18         array[loop_i_mount] = malloc (capacity * sizeof(char));
This is wrong. You need to allocate space for sufficient characters to copy. As written, you allocate one character for the first element, two for the second, and so on.

OMG :oops: I copy-pasted it...
Sorry for so dumb error.

EDIT
Hu wrote:
Code:
    12     length = &capacity;
capacity will go out of scope
Code:
   *length = capacity;

A perfect explanation of the difference between ptr = &var and *ptr = var.
Back to top
View user's profile Send private message
halcon
Apprentice
Apprentice


Joined: 15 Dec 2019
Posts: 279

PostPosted: Thu Oct 22, 2020 11:24 pm    Post subject: Reply with quote

Hu,

Thanks to your instructions I've got a working, non-segfaulting version! All lacking changes to be followed...

Code:
pass_ptr_to_empty_array_through_two_funcs_and_return_full_array_back.c[30]: read_forbidden_volumes started
pass_ptr_to_empty_array_through_two_funcs_and_return_full_array_back.c[20]: read_forbidden_mounts successfully assigned all the values, length = 7, array[6] = /mnt/sambadir2
pass_ptr_to_empty_array_through_two_funcs_and_return_full_array_back.c[34]: read_forbidden_volumes successfully read 7 forbidden mounts, array[6] = /mnt/sambadir2
pass_ptr_to_empty_array_through_two_funcs_and_return_full_array_back.c[40]: read_forbidden_volumes successfully assigned all the values, length = 7, array [6] = /mnt/sambadir2 changed
pass_ptr_to_empty_array_through_two_funcs_and_return_full_array_back.c[64]: update_volumes successfully read 7 forbidden volumes
pass_ptr_to_empty_array_through_two_funcs_and_return_full_array_back.c[20]: read_forbidden_mounts successfully assigned all the values, length = 7, array[6] = /mnt/sambadir2
pass_ptr_to_empty_array_through_two_funcs_and_return_full_array_back.c[84]: update_mounts successfully read 7 forbidden mounts
Back to top
View user's profile Send private message
Hu
Moderator
Moderator


Joined: 06 Mar 2007
Posts: 15985

PostPosted: Fri Oct 23, 2020 1:42 am    Post subject: Reply with quote

Congratulations. Unfortunately, you have mixed two styles in a way that leads to new problems. ;) You changed the declaration of array to const char **const, which is (almost) appropriate for your change to use static const char *const user_forbidden_mounts[]. However, you are still allocating dynamic copies of the string (now through strdup), so you need to free those copies - and you will get a warning when you try to free them from that const char **. Additionally, since you allocated only 1 * sizeof (const char *) in the caller, you cannot safely write more than one pointer into the array - but you write one for every element of the forbidden list. This will likely cause memory corruption, and it is only by chance that the corruption did not crash your test program. My intent with the static forbidden list is that you would have:
Code:
static void
read_forbidden_mounts (const char *const **const array,
                         unsigned int *const length)
{
  static const char *const user_forbidden_mounts[] = { "/mnt/cdrom", "/mnt/cdaudio", "/tmp", "/var", "/dev/shm", "/mnt/sambadir1", "/mnt/sambadir2" };
  *array = user_forbidden_mounts;
  *length = sizeof (user_forbidden_mounts) / sizeof (user_forbidden_mounts[0]);
}
Since you don't need to modify the strings, you don't need to copy them at all. Just return a pointer to the pre-initialized static storage in which the pointers were placed at load time.

I suggest you install dev-util/valgrind and use it to analyze your program. Although its speed is poor, it is very effective at finding and reporting misuses of memory, including writing past the end of containers, failing to free allocated memory, freeing the same block multiple times, and so on.
Back to top
View user's profile Send private message
Display posts from previous:   
Reply to topic    Gentoo Forums Forum Index Portage & Programming All times are GMT
Page 1 of 1

 
Jump to:  
You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot vote in polls in this forum