Fix Segmentation fault if HOME not set #162

EyitopeIO · 2024-08-30T01:23:41Z

Fixes #154

EyitopeIO · 2024-08-30T01:38:21Z

If the user-specified cache directory had other content the user cared about, then clearing the cache would wipe that directory along with the non-httpdirfs content.

Perhaps the help should be updated with this comment?

fangfufu · 2024-08-30T08:56:15Z

Yeh, your previous pull request successfully address the stated GitHub issue, so I merged it. If you cherry-pick this on top of the current master, update the pull request saying that it fixes #154, I will merge it if it is up to standard like the previous one.

src/cache.c

fangfufu · 2024-08-30T14:31:19Z

You need to rebase your changes on top the head of the master branch. There are merge conflicts. The quickest way to resolve it is to rebase. I was happy with your previous commit, that's why I merged it. :)

EyitopeIOn · 2024-08-30T17:03:29Z

Commenting as @EyitopeIO

Good to know. Thanks.

I'm testing with $HOME non-existent, but got a crash. I'd do my best to keep the standard up and request a re-review as soon as it's ready.

EyitopeIOn · 2024-08-30T20:10:59Z

I think I don't have permissions to edit the PR title because I don't see the edit button.
I see you may be able to change the PR title at the point of merging.

src/cache.c

src/cache.h

src/main.c

src/cache.c

fangfufu · 2024-08-31T12:31:51Z

src/cache.c

+                home = path_append(user_home, xdg_default);
+            }
+        } else {
+            home = path_append(xdg_home, xdg_default);


Is this what we really want to do? When xdg_home == "XDG_CACHE_HOME"

src/main.c

fangfufu · 2024-08-31T12:33:36Z

In general you need to take more care about your variable names, I think.

fangfufu · 2024-08-31T23:10:01Z

src/main.c

+            lprintf(warning, "$HOME is unset\n");
+            char *cur_dir = realpath("./", NULL);
+            if (!cur_dir) {
+                lprintf(fatal, "Could not get config directory\n");


This should not be fatal, if the software cannot get the config directory, it should keep running. This should just be a warning.

fangfufu · 2024-08-31T23:14:11Z

src/cache.c

+        /*
+        *  "XDG_CACHE_HOME/HOME/.cache" > "HOME/.cache"  > "./.cache"
+        */
+        char *xdg = getenv("XDG_CACHE_HOME");


rename xdg to xdg_cache_home.

fangfufu · 2024-08-31T23:15:21Z

src/main.c

+{
+    const char *default_config_subdir = "/.config";
+    char *config_dir;
+    char *xdg = getenv("XDG_CONFIG_HOME");


rename this xdg to xdg_config_home.

fangfufu · 2024-08-31T23:27:56Z

src/cache.c

+        /*
+        *  "XDG_CACHE_HOME/HOME/.cache" > "HOME/.cache"  > "./.cache"
+        */


I am not sure if the comment reflects what's actually happening. The code is clear enough describing what's happening. Perhaps delete this?

fangfufu · 2024-08-31T23:32:51Z

src/cache.c

+        *  "XDG_CACHE_HOME/HOME/.cache" > "HOME/.cache"  > "./.cache"
+        */
+        char *xdg = getenv("XDG_CACHE_HOME");
+        if (!xdg) {


I would invert the this condition to make the whole thing clearer.

fangfufu · 2024-08-31T23:33:08Z

src/main.c

+    const char *default_config_subdir = "/.config";
+    char *config_dir;
+    char *xdg = getenv("XDG_CONFIG_HOME");
+    if (!xdg) {


I would invert the this condition to make the whole thing clearer.

fangfufu · 2024-08-31T23:35:03Z

src/cache.c

+        char *xdg = getenv("XDG_CACHE_HOME");
+        if (!xdg) {
+            char *user_home = getenv("HOME");
+            if (!user_home) {


I would invert this condition too.

fangfufu · 2024-08-31T23:36:20Z

src/main.c

+    char *xdg = getenv("XDG_CONFIG_HOME");
+    if (!xdg) {
+        char *user_home = getenv("HOME");
+        if (!user_home) {


I would invert this condition.

fangfufu · 2024-08-31T23:45:59Z

Please address all the code quality issues introduced by your change flagged up by the static analysers.

Also, in general, when you have a if statement, you should condition them in such a way that the most common condition occurs at the first branch, so the code is easier to read. This is what I want in this repository for new contributions.

For example, I prefer:

if (common_condition) {
// handle common condition
} else {
// handle unexpected condition
}

I don't like:

if (!common_condition) {
// handle unexpected condition
} else {
//deal with common condition
}

Anyways, this pull request is heading towards a good direction. I like it so far.

EyitopeIO · 2024-09-01T10:59:32Z

Comments on static analyser

Issues about realpath function are false-positives because the function is already called with a destination buffer set to NULL.
Issue about the $HOME variable being modified by an attacker is not applicable. We assume the user's system is already secure, so whatever is in$HOME is legit.

sonarcloud · 2024-09-01T11:41:27Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

fangfufu reviewed Aug 30, 2024

View reviewed changes

src/cache.c Outdated Show resolved Hide resolved

EyitopeIO force-pushed the main branch 4 times, most recently from 602e449 to 9fb6226 Compare August 30, 2024 12:40

EyitopeIO force-pushed the main branch 4 times, most recently from fc97fc7 to c2a2807 Compare August 30, 2024 20:03

EyitopeIO requested a review from fangfufu August 30, 2024 21:49

fangfufu changed the title ~~Add option to clear cache~~ Fix Segmentation fault if HOME not set Aug 30, 2024

fangfufu requested changes Aug 30, 2024

View reviewed changes

src/cache.c Outdated Show resolved Hide resolved

src/cache.h Outdated Show resolved Hide resolved

src/main.c Outdated Show resolved Hide resolved

EyitopeIO force-pushed the main branch from c2a2807 to 13fd26e Compare August 30, 2024 23:52

EyitopeIO requested a review from fangfufu August 30, 2024 23:56

EyitopeIO force-pushed the main branch from 13fd26e to 1496f64 Compare August 31, 2024 00:03

fangfufu requested changes Aug 31, 2024

View reviewed changes

src/cache.c Outdated Show resolved Hide resolved

fangfufu reviewed Aug 31, 2024

View reviewed changes

src/cache.c Outdated Show resolved Hide resolved

EyitopeIO force-pushed the main branch from 1496f64 to 0a91803 Compare August 31, 2024 11:30

EyitopeIO requested a review from fangfufu August 31, 2024 11:33

fangfufu requested changes Aug 31, 2024

View reviewed changes

EyitopeIO force-pushed the main branch 2 times, most recently from 0e5c191 to 452757a Compare August 31, 2024 12:55

EyitopeIO requested a review from fangfufu August 31, 2024 14:59

fangfufu requested changes Aug 31, 2024

View reviewed changes

EyitopeIO force-pushed the main branch from 452757a to 3484f4a Compare September 1, 2024 10:46

EyitopeIO force-pushed the main branch 4 times, most recently from 4b846e6 to c45ef51 Compare September 1, 2024 11:14

Fix segfault when HOME is not set

2976c29

EyitopeIO force-pushed the main branch from c45ef51 to 2976c29 Compare September 1, 2024 11:40

EyitopeIO requested a review from fangfufu September 1, 2024 11:46

fangfufu merged commit 09ebc82 into fangfufu:master Sep 1, 2024
3 of 5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Segmentation fault if HOME not set #162

Fix Segmentation fault if HOME not set #162

EyitopeIO commented Aug 30, 2024 •

edited

Loading

EyitopeIO commented Aug 30, 2024 •

edited

Loading

fangfufu commented Aug 30, 2024

fangfufu commented Aug 30, 2024

EyitopeIOn commented Aug 30, 2024 •

edited

Loading

EyitopeIOn commented Aug 30, 2024 •

edited

Loading

fangfufu Aug 31, 2024

fangfufu commented Aug 31, 2024

fangfufu Aug 31, 2024

fangfufu Aug 31, 2024

fangfufu Aug 31, 2024

fangfufu Aug 31, 2024

fangfufu Aug 31, 2024

fangfufu Aug 31, 2024

fangfufu Aug 31, 2024

fangfufu Aug 31, 2024

fangfufu commented Aug 31, 2024 •

edited

Loading

EyitopeIO commented Sep 1, 2024 •

edited

Loading

sonarcloud bot commented Sep 1, 2024

Fix Segmentation fault if HOME not set #162

Fix Segmentation fault if HOME not set #162

Conversation

EyitopeIO commented Aug 30, 2024 • edited Loading

EyitopeIO commented Aug 30, 2024 • edited Loading

fangfufu commented Aug 30, 2024

fangfufu commented Aug 30, 2024

EyitopeIOn commented Aug 30, 2024 • edited Loading

EyitopeIOn commented Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

fangfufu commented Aug 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fangfufu commented Aug 31, 2024 • edited Loading

EyitopeIO commented Sep 1, 2024 • edited Loading

sonarcloud bot commented Sep 1, 2024

Quality Gate passed

EyitopeIO commented Aug 30, 2024 •

edited

Loading

EyitopeIO commented Aug 30, 2024 •

edited

Loading

EyitopeIOn commented Aug 30, 2024 •

edited

Loading

EyitopeIOn commented Aug 30, 2024 •

edited

Loading

fangfufu commented Aug 31, 2024 •

edited

Loading

EyitopeIO commented Sep 1, 2024 •

edited

Loading