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

The Con has now a "colored" flag #1

Closed
wants to merge 3 commits into from
Closed

The Con has now a "colored" flag #1

wants to merge 3 commits into from

Conversation

setzer22
Copy link

@setzer22 setzer22 commented Sep 2, 2015

- The flag is set whenever a Con's custom color is set with the color command.
- Whenever the color command is called, the default colors get copied in the
  colorclasses unspecified by the command.
- Instead of checking for color == 0 to see if the color is changed, the colored
  flag is used.

    - The flag is set whenever a Con's custom color is set with the color command.
    - Whenever the color command is called, the default colors get copied in the
      colorclasses unspecified by the command.
    - Instead of checking for color == 0 to see if the color is changed, the colored
      flag is used.
@setzer22
Copy link
Author

setzer22 commented Sep 2, 2015

I had forgotten to push some changes on the first commit.

@Airblader
Copy link
Owner

I don't really like this approach. Having a boolean flag sounds like duplication to me. The information should be whether there is a color set or not and if there is, use it. Also, this approach doesn't allow only overwriting certain color classes and not others.

I only wrote super ugly in my patch because I just compared one random value against 0 which of course has to be done in a much smarter way. It'd be possible to use a pointer and check it against NULL, for example.

We should discuss this more in the i3 ticket.

@setzer22
Copy link
Author

setzer22 commented Sep 3, 2015

I agree that the possible inconsistencies between the colors themselves and the flag might lead to trouble at some point, but I had already given this some thought and looked like the most clean alternative to me.

The thing is, that if we're to store pointers to colors instead of the colors themselves in the Con to properly check against NULL we should be thinking where the custom colors should be allocated. I don't think having them scattered across the heap is a good idea, that's why I proposed the boolean flag. Maybe some other allocation method could be used, but at this point this approach didn't seem that much of a bad idea, because having a "color-pool" just looks weird to me. Lastly, having both the colors and the pointer inside the Con would essentially be the same as having a boolean flag, or the equivalent to have many boolean flags, one per color, so I don't think it's a better alternative either.

The changes do allow overwritting some colorclasses only, the only problem is default colors are copied instead of referenced from the config. That could only be a problem when resetting i3 with a default color change in-between.

Anyway we should define what happens when i3 resets and the default colors change (or default colors change at runtime in any other way, not sure if there's any).

Even if these changes lead nowhere they helped me as an exercise in becoming familiar with i3 development, so no worries :D.

@Airblader
Copy link
Owner

Restarting i3 won't be a problem. I left a TODO in my patch saying that we need to dump the con's color properties and read it when we reload the layout. So that should work fine.

I think it makes sense if you work on a good PR (with the rest as discussed in the ticket) and we see what Michael says. There'll just be some iteration necessary, I believe, but that's how development works. :)

…n a colorclass

when some colors are unspecified those are copied like other colorclasses.
Also added some "helper" macros for the big macro so we get less duplicated code. I'm
not really sure it helps with that though so I might remove it.
Airblader pushed a commit that referenced this pull request Nov 21, 2016
Fix memory leaks when executing 'i3 --moreversion'.

=================================================================
==14852==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 159 byte(s) in 1 object(s) allocated from:
    #0 0x7fea40855602 in malloc (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x98602)
    #1 0x4c4c4a in smalloc ../../i3/libi3/safewrappers.c:24
    i3#2 0x4c3aee in ipc_recv_message ../../i3/libi3/ipc_recv_message.c:61
    i3#3 0x44dc2e in display_running_version ../../i3/src/display_version.c:94
    i3#4 0x472947 in main ../../i3/src/main.c:269
    i3#5 0x7fea3d0c982f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)

Direct leak of 39 byte(s) in 2 object(s) allocated from:
    #0 0x7fea40855602 in malloc (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x98602)
    #1 0x7fea3d11f7d7 in vasprintf (/lib/x86_64-linux-gnu/libc.so.6+0x767d7)

SUMMARY: AddressSanitizer: 198 byte(s) leaked in 3 allocation(s).
Airblader pushed a commit that referenced this pull request Sep 4, 2017
This fixes the following issue when having an error early in the config file:

==1562==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6220000180ff at pc 0x55c837edb1d3 bp 0x7ffee7534650 sp 0x7ffee7534648
READ of size 1 at 0x6220000180ff thread T0
    #0 0x55c837edb1d2 in start_of_line ../../i3/src/config_parser.c:238
    #1 0x55c837edc96f in parse_config ../../i3/src/config_parser.c:493
    i3#2 0x55c837edf527 in parse_file ../../i3/src/config_parser.c:1091
    i3#3 0x55c837ecf14b in parse_configuration ../../i3/src/config.c:65
    i3#4 0x55c837ed1ef4 in load_configuration ../../i3/src/config.c:230
    i3#5 0x55c837f0a8d0 in main ../../i3/src/main.c:539
    i3#6 0x7fb63ae042b0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202b0)
    i3#7 0x55c837e95eb9 in _start (/home/michael/i3/build/i3+0x4beb9)

0x6220000180ff is located 1 bytes to the left of 5165-byte region [0x622000018100,0x62200001952d)
allocated by thread T0 here:
    #0 0x7fb63e590cf8 in malloc (/usr/lib/x86_64-linux-gnu/libasan.so.3+0xc1cf8)
    #1 0x55c837f59aa6 in smalloc ../../i3/libi3/safewrappers.c:24
    i3#2 0x55c837edef45 in parse_file ../../i3/src/config_parser.c:1029
    i3#3 0x55c837ecf14b in parse_configuration ../../i3/src/config.c:65
    i3#4 0x55c837ed1ef4 in load_configuration ../../i3/src/config.c:230
    i3#5 0x55c837f0a8d0 in main ../../i3/src/main.c:539
    i3#6 0x7fb63ae042b0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202b0)
Airblader pushed a commit that referenced this pull request Jan 27, 2019
This fixes a crash produced with the following config:
    # i3 config file (v4)
    workspace 1 output $screen1
    workspace 2 output $screen2

    exec --no-startup-id "i3-msg workspace 1, open && i3-msg workspace 2 && xrandr --output $screen2 --off && xrandr --output $screen1 --auto --output $screen2 --auto --right-of $screen1 "

Which results in:
ERROR: AddressSanitizer: heap-use-after-free on address …
READ of size 8 at 0x614000001f48 thread T0
    #0 0x5563df6e73a8 in init_ws_for_output i3/src/randr.c:468
    #1 0x5563df6ef3b4 in randr_query_outputs i3/src/randr.c:940
    i3#2 0x5563df68dbe1 in handle_screen_change i3/src/handlers.c:450

… is located 264 bytes inside of 448-byte region …
freed by thread T0 here:
    #1 0x5563df634b0a in con_free i3/src/con.c:96
    i3#2 0x5563df7151e6 in tree_close_internal i3/src/tree.c:344
    i3#3 0x5563df7280fe in workspace_show i3/src/workspace.c:499
    i3#4 0x5563df6e7315 in init_ws_for_output i3/src/randr.c:457
    i3#5 0x5563df6ef3b4 in randr_query_outputs i3/src/randr.c:940
    i3#6 0x5563df68dbe1 in handle_screen_change i3/src/handlers.c:450

Which is similar to i3#3228, i3#3248.
@Airblader Airblader closed this Jun 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants