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

ADM_coreSocket: prevent uninitialized use of *port in createBindAndAc… #152

Closed
wants to merge 1 commit into from

Conversation

sobukus
Copy link

@sobukus sobukus commented Jan 21, 2018

…cept

I noticed random failure (rather, random success) in startup of avidemux3_jobs_qt5,
which seems to relate to a random component in the port used for opening the
background connecton:

Directory /home/user/.avidemux6/ exists.Good.
Using /home/user/.avidemux6/ as base directory for prefs, jobs, etc.
[jobInit] 18:54:38-080 Initializing database (/home/user/.avidemux6/jobs.sql)
[ADM_jobCheckVersion] 18:54:38-081 Db version 3, our version 3
[ADM_jobCheckVersion] 18:54:38-081 Same version, continuing..
[jobInit] 18:54:38-081 Successfully connected to jobs database..
QStandardPaths: XDG_RUNTIME_DIR not set, defaulting to '/tmp/runtime-user'
[createBindAndAccept] 18:54:38-231 Binding on 127.0.0.1:2
[createBindAndAccept] 18:54:38-231 bind() failed
[close] 18:54:38-231 [ADMSocket]Error when socket shutdown -1 (socket 9)
[popup] 18:54:38-231 Cannot bind socket
Cleaning up
[jobShutDown] 18:54:38-234 Shutting down jobs database
[onexit] 18:54:38-234
Goodbye...

user@machine:~$ avidemux3_jobs_qt5


Avidemux v2.7.0


http://www.avidemux.org
Code : Mean, JSC, Gruntster
GFX : Nestor Di , nestordi@augcyl.org
Design : Jakub Misak
FreeBSD : Anish Mistry, amistry@am-productions.biz
Audio : Mihail Zenkov
MacOsX : Kuisathaverat
Win32 : Gruntster

Compiler: GCC 7.2.0
Build Target: Linux (x86-64)

Large file available: 1 offset
Directory /home/user/.avidemux6/ exists.Good.
Using /home/user/.avidemux6/ as base directory for prefs, jobs, etc.
[jobInit] 18:58:40-054 Initializing database (/home/user/.avidemux6/jobs.sql)
[ADM_jobCheckVersion] 18:58:40-055 Db version 3, our version 3
[ADM_jobCheckVersion] 18:58:40-055 Same version, continuing..
[jobInit] 18:58:40-055 Successfully connected to jobs database..
QStandardPaths: XDG_RUNTIME_DIR not set, defaulting to '/tmp/runtime-user'
[createBindAndAccept] 18:58:40-235 Binding on 127.0.0.1:-1006610672
[createBindAndAccept] 18:58:40-235 Socket bound to port 22288
[jobWindow] 18:58:40-235 Socket bound to 22288
*
*
*
*
*
[refreshList] 18:58:40-236 Found 5 jobs

The comments in the ADM_socket::createBindAndAccept() routine suggest
that the value for the port should be 0 when calling bind(). This change
ensures that.

Note: I tested this change on the 2.7.0 release and then went over to
github to hand-edit the trivial change in. I am not fully sure about the
intent of using the provided value of *port at all (even in the log message
before binding). Is it ever initialized? Anyhow, knowledgeable people
should figure out the correct thing to do quickly. I don't mind if you discard
this dirty pull request as long as the proper fix goes in;-)

[And: Is this github repo the official development platform for Avidemux now? I only found it mentioned as ‘mirror’ in the forum … next to the long-dead berios links …]

…cept

I noticed random failure (rather, random success) in startup of avidemux3_jobs_qt5,
which seems to relate to a random component in the port used for opening the
background connecton:

Directory /home/user/.avidemux6/ exists.Good.
Using /home/user/.avidemux6/ as base directory for prefs, jobs, etc.
 [jobInit] 18:54:38-080  Initializing database (/home/user/.avidemux6/jobs.sql)
 [ADM_jobCheckVersion] 18:54:38-081  Db version 3, our version 3
 [ADM_jobCheckVersion] 18:54:38-081  Same version, continuing..
 [jobInit] 18:54:38-081  Successfully connected to jobs database..
QStandardPaths: XDG_RUNTIME_DIR not set, defaulting to '/tmp/runtime-user'
 [createBindAndAccept] 18:54:38-231  Binding on 127.0.0.1:2
 [createBindAndAccept] 18:54:38-231  bind() failed  
 [close] 18:54:38-231  [ADMSocket]Error when socket shutdown  -1 (socket 9)
 [popup] 18:54:38-231  Cannot bind socket
Cleaning up
 [jobShutDown] 18:54:38-234  Shutting down jobs database
 [onexit] 18:54:38-234  
Goodbye...

user@machine:~$ avidemux3_jobs_qt5 
*************************
  Avidemux v2.7.0
*************************
 http://www.avidemux.org
 Code      : Mean, JSC, Gruntster 
 GFX       : Nestor Di , nestordi@augcyl.org
 Design    : Jakub Misak
 FreeBSD   : Anish Mistry, amistry@am-productions.biz
 Audio     : Mihail Zenkov
 MacOsX    : Kuisathaverat
 Win32     : Gruntster

Compiler: GCC 7.2.0
Build Target: Linux (x86-64)

Large file available: 1 offset
Directory /home/user/.avidemux6/ exists.Good.
Using /home/user/.avidemux6/ as base directory for prefs, jobs, etc.
 [jobInit] 18:58:40-054  Initializing database (/home/user/.avidemux6/jobs.sql)
 [ADM_jobCheckVersion] 18:58:40-055  Db version 3, our version 3
 [ADM_jobCheckVersion] 18:58:40-055  Same version, continuing..
 [jobInit] 18:58:40-055  Successfully connected to jobs database..
QStandardPaths: XDG_RUNTIME_DIR not set, defaulting to '/tmp/runtime-user'
 [createBindAndAccept] 18:58:40-235  Binding on 127.0.0.1:-1006610672
 [createBindAndAccept] 18:58:40-235  Socket bound to port 22288
 [jobWindow] 18:58:40-235  Socket bound to 22288
*
*
*
*
*
 [refreshList] 18:58:40-236  Found 5 jobs

The comments in the ADM_socket::createBindAndAccept() routine suggest
that the value for the port should be 0 when calling bind(). This change
ensures that.

Note: I tested this change on the 2.7.0 release and then went over to
github to hand-edit the trivial change in. I am not fully sure about the
intent of using the provided value of *port at all (even in the log message
before binding). Is it ever initialized? Anyhow, knowledgeable people
should figure out the correct thing to do quickly. I don't mind if you discard
this dirty pull request as long as the proper fix goes in;-)
@mean00
Copy link
Owner

mean00 commented Jan 22, 2018

bind.txt
Does that work for you ?

(single line patch attached as .txt)

@eumagga0x2a
Copy link
Collaborator

eumagga0x2a commented Jan 22, 2018

I'm sorry, I didn't notice the pull request and fixed the issue in the very same way (edit: as proposed by mean00) independently.

@sobukus
Copy link
Author

sobukus commented Jan 22, 2018

Ah, you're quick. I don't mind the fix either way. I did not test mean00's fix yet, but I suppose it has the same effect for the code path that affected my use case. My only gripe is that the comments in the function should be fixed, as they claim it always uses an arbitrary port assigned by the OS when in fact it relies on you initializing the port parameter accordingly.

@sobukus
Copy link
Author

sobukus commented Jan 22, 2018

OK, tested it. Seems fine. So we can close this pull request. I still suggest changing the
documentation for the function to indicate that ‘any port’ does not mean that the
port parameter is output only. Forgetting to initialize it could happen again.

@sobukus sobukus closed this Jan 22, 2018
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.

3 participants