Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Acceptable use policy optional addition. #2535

Open
wants to merge 1 commit into
base: devel
Choose a base branch
from

Conversation

Nexarian
Copy link
Contributor

@Nexarian Nexarian commented Feb 5, 2023

WIP, don't review this yet!

@Nexarian Nexarian force-pushed the Nexarian/login-screen-aup-fix branch from 5d4b76a to 6783803 Compare February 5, 2023 04:52
@Nexarian Nexarian force-pushed the Nexarian/login-screen-aup-fix branch 6 times, most recently from 8ce244d to 868d74f Compare February 12, 2023 07:15
@Nexarian Nexarian force-pushed the Nexarian/login-screen-aup-fix branch 2 times, most recently from bde1721 to 38de36e Compare February 13, 2023 04:58
@Nexarian Nexarian force-pushed the Nexarian/login-screen-aup-fix branch from 38de36e to 11b17e1 Compare February 13, 2023 04:59
@Nexarian Nexarian changed the title Working on aup screen Acceptable use policy optional addition. Feb 13, 2023
@Nexarian
Copy link
Contributor Author

I recognize the changes to the xrdp.ini.in file can't be checked in (probably have to be checked in as an example template and/or with the documentation), but otherwise, this is ready to review.

The most important workflow change here is that it will now populate the password box with the password that's sent over the wire from the client, and the user could change it.

What do you all think?
Example AUP login screen

@Nexarian Nexarian marked this pull request as ready for review February 13, 2023 05:11
Copy link
Member

@matt335672 matt335672 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments for you

list_add_item(lines, (tintptr) pch);
pch = strtok(NULL, "\n");
}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Memory problems here I think. You're allocating memory with a single call to strdup(), and then expecting auto_free to fix it for you on list_delete(). However, you'll be calling free on addresses mid-way through the allocated block.

How about (in string_calls.c or list.c) :-

static const char *
append_fragment(const char *start, const char *end, struct list *list)
{
    unsigned int len = end - start;
    char *copy = malloc(len + 1);
    g_memcpy(copy, start, len);
    copy[len] = '\0';
    list_add_item(list, (tintptr)copy);
    return end + 1;
}

struct list *
split_string_by_newline(const char *str)
{
    struct list *result = list_create();
    result->auto_free = 1;

    if (str != NULL)
    {
        const char *p;

        while ((p = g_strchr(str, '\n')) != NULL)
        {
            str = append_fragment(str, p, result);
        }

        if (*str != '\0')
        {
            append_fragment(str, str + g_strlen(str), result);
        }
    }
    
    return result;
}

It's got a slightly different signature admittedly, but all memory is freed on list_delete()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, it behaves differently from strtok() for contibuous delimiters.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what happens when you've spent too much of your career in languages with garbage collectors and then go code in an unmanaged memory language when you're tired. You're right of course!

I think I'll split this part up into smaller PRs as always to make them easier to review.

file_path);
return 0;
}
char *str = (char *)g_malloc(sizeof(char) * (file_size + 1024), 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check malloc result?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants